Keep the default write paths out of a read-denied directory - #506
Open
ronleizrowice-ant wants to merge 2 commits into
Open
Keep the default write paths out of a read-denied directory#506ronleizrowice-ant wants to merge 2 commits into
ronleizrowice-ant wants to merge 2 commits into
Conversation
~/.npm/_logs and ~/.claude/debug are added to every write allowlist as a convenience; under a denyRead that covers the home they were bound back over the deny on Linux, readable and writable, and writable on macOS. getDefaultWritePaths now drops a default that lies at or under a read-denied directory; a caller who wants it lists it in allowWrite.
getDefaultWritePaths compares each recommended path, as listed and as normalizePathForSandbox spells it, against the normalized denyRead entries through one at-or-under helper instead of four inline clauses. wrapWithSandbox computes the effective denyRead (configured entries unioned with credential deny paths) once, ahead of the write config, and hands that to getDefaultWritePaths rather than spelling the customConfig/config fallback a second time.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Could getDefaultWritePaths() account for denyRead globs too? On Linux, ~/.npm/* is expanded later to a concrete _logs deny, but this helper ignores non-trailing globs and still adds _logs to allowOnly, which can bind it back over the read deny. That seems to leave the same fail-open path for glob-based policies.
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
2 participants
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.
getDefaultWritePaths()adds~/.npm/_logsand~/.claude/debugto every write allowlist as a convenience. When a caller read-denies the home (denyRead: ["~"], ordenyRead: ["/root"]for a root caller), the Linux deny loop mounts a tmpfs over it and then binds every allowed write path beneath it back — including those two, which the caller never asked for — so both directories were readable and writable inside a sandbox whose policy hides the whole home, and persist across commands. Reproduced with/root/.npm/_logs(present wherevernpm install -gran as root).Fix:
getDefaultWritePaths(denyRead)drops a convenience path that lies at or under a read-denied directory (compared afternormalizePathForSandbox, so~, trailing slashes and symlinked homes match; glob entries are ignored). The three call sites insandbox-manager.tspass the activedenyRead. A caller who wants such a path writable lists it inallowWriteexplicitly, which still works. Applies on macOS too, where the same entry made the path writable (not readable) under a denied home.Tests:
test/sandbox/default-write-paths.test.ts— defaults present with no denies; both dropped under a denied home (plain,~/, and the exact directory); kept for a sibling that only shares a string prefix; glob denies ignored.Blast radius: sandboxes whose
denyReadcovers the home lose implicit write access to~/.npm/_logsand~/.claude/debug(npm falls back to not writing its debug log; nothing in this repo writes to~/.claude/debugfrom inside a sandbox). No change for anyone else. Not behind a flag.