fix: restrict ~/.openwiki ACLs on Windows where chmod is a no-op#367
Open
Wenyu Chiou (WenyuChiou) wants to merge 1 commit into
Open
fix: restrict ~/.openwiki ACLs on Windows where chmod is a no-op#367Wenyu Chiou (WenyuChiou) wants to merge 1 commit into
Wenyu Chiou (WenyuChiou) wants to merge 1 commit into
Conversation
The OpenWiki home directory is created with mode 0o700 and chmod'd to 0o700 wherever it is ensured, but on Windows Node's chmod only toggles the read-only attribute - the directory holding ~/.openwiki/.env (every provider API key and OAuth refresh token in plaintext), the sqlite chat checkpoints, and raw connector dumps simply inherits whatever ACL its parent grants. restrictDirToCurrentUser mirrors the existing 0o700 owner-only intent with icacls: grant full control to the current user and SYSTEM (by well-known SID, language-independent), inheritable so new children are covered, then remove inherited ACEs. The grant runs before the inheritance reset so a failed grant can never lock the user out, and the whole helper is best-effort (returns false, never throws) exactly like the surrounding chmod calls. No-op on non-Windows platforms. Wired into ensureOpenWikiHome and saveOpenWikiEnv, the two places that express the 0o700 intent on the home directory. Verified on a real Windows 11 machine: a fresh temp directory carrying a dozen inherited modify-rights ACEs was reduced to exactly NT AUTHORITY\SYSTEM and the current user with (OI)(CI)(F). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
restrictDirToCurrentUser(src/windows-acl.ts) and wires it into the two places that express the home directory's 0o700 owner-only intent —ensureOpenWikiHomeandsaveOpenWikiEnv:win32, runsicacls <dir> /grant:r <user>:(OI)(CI)F *S-1-5-18:(OI)(CI)F, thenicacls <dir> /inheritance:r— full control for the current user and SYSTEM (well-known SID, language-independent), inheritable so newly created children (.env,openwiki.sqlite,connectors/**) are covered, inherited ACEs removed.falseinstead of throwing (mirroring the surroundingchmodIfExistssemantics) — a machine without workingicaclsnever fails a run.Why
On Windows, Node's
fs.chmodonly toggles the read-only attribute and themodeoption onmkdir/writeFileis ignored — so the existing 0o700/0o600 calls protect nothing there, and~/.openwiki(plaintext.envwith every provider key and the ChatGPT refresh token, sqlite chat checkpoints, raw connector dumps) simply inherits its parent's ACL.Verified on a real Windows 11 machine (machine name redacted). A fresh directory before/after the helper:
Scope notes, stated explicitly: pre-existing files keep the ACEs they already inherited (no recursive
/Tpass — new children inherit the restricted ACL; a recursive apply on a largeconnectors/rawtree could be slow and is easy to add later if wanted). Administrators lose their inherited ACE on the directory but can still take ownership — the same way root bypasses 0o700 on POSIX.How tested
test/windows-acl.test.ts(3 tests, mockednode:child_process, runs on any platform incl. ubuntu CI): non-win32 no-op; exact grant + inheritance-reset argument sequence with grant ordered first; grant failure short-circuits without removing inheritance (the lockout guard).icaclsoutput before/after shown above.pnpm run format,pnpm run lint,pnpm test: 298/300 pass locally on Windows; the 2 failures are the pre-existing win32test/env-behavior.test.tsplatform checks (they reproduce on pristinemain) — notably, this PR is the change that makes the intent of those 0600 expectations real on Windows at the ACL layer.Fixes #366
🤖 Generated with Claude Code