A / write root (Linux) or read root (macOS) no longer switches off the denies beneath it - #533
Open
ronleizrowice-ant wants to merge 5 commits into
Open
Conversation
"/" is a legal entry in the write allowlist — normalization keeps it, and the allow loop binds it writable — but isWithinAnyAllowedWritePath, the one gate deciding whether a write-deny destination lies inside that allowlist, spelled containment as an `allowedPath + '/'` string prefix. For "/" that prefix is "//", which matches nothing, so every denyWrite entry and every mandatory deny (.git/hooks, .git/config, .bashrc, .mcp.json, .claude/commands, ...) that no other allow entry happened to cover was logged "not within allowed paths" and got neither a read-only bind nor a placeholder. The deny list failed open on top of a root the allow loop had already bound writable. findSymlinkInPath compared the same way, so a symlinked ancestor of a deny path was never seen to be inside a writable root either, and the /dev/null mask that stops that component being deleted and recreated as a real directory was never emitted. Both now use the root-aware isAtOrUnder helper. For any directory other than "/" the helper is the same comparison the code spelled inline, so a configuration without "/" in the write allowlist renders exactly as before. Also corrects the example on isAtOrUnder's docstring, which had the pair backwards: "/x" does not start with "/xy" under a plain prefix test either, so it demonstrates nothing; "/xy" against "/x" is the pair that shows the segment boundary.
Seatbelt is last-match-wins, so a literal denyRead sitting inside a literal allowRead directory is re-emitted after the allow block or the allow wins. That containment test was a `dir + '/'` string prefix, so an allowRead entry of "/" — which re-opens every denied path — matched nothing and no deny was re-emitted at all. Use the module's own root-aware isStrictlyUnder for the test, and have that function delegate to the shared helper in sandbox-utils so the root rule has one home instead of two copies to keep in step.
…ifiable
New cases, each of which fails without the fix: with allowWrite ["/"], a
user denyWrite file and directory each get a real --ro-bind, an existing
mandatory deny gets one too, an absent one gets its placeholder, a
symlinked ancestor of a deny path gets the /dev/null mask, and — where
the host can run bwrap namespaces — a write to the denied file fails on
a read-only mount while a write elsewhere still succeeds.
The two existing "/" cases could not fail. Both located the deny-side
root bind with lastIndexOf('--ro-bind / /'), which falls back to the base
root mount that spells it identically, so "> -1" and the ordering
assertions held even with the deny-side bind gone. They now count whole
argv triples and expect exactly two. The runtime arm's "the write is
denied" check asserted only a non-zero exit, which a bwrap startup abort
or a spawn timeout satisfies just as well; it now asserts the
read-only-mount failure itself.
…h deny is skipped allowOnly ['/'] with denyWithinAllow ['/'] and nothing else makes '/' a recorded covering deny directory that no veto reaches: no allowed write path lies strictly beneath it and there is no read-deny tmpfs anywhere. So every other deny — the mandatory binds and the creation-blocking stubs alike — is skipped as already covered, and the deny-side --ro-bind / / emitted after the allow's writable --bind / / is the whole protection. The shape is only reachable now that the within-allowlist predicate is root-aware, and nothing covered it. The test also pins the first veto in either direction: a second allow entry lies strictly beneath '/', and a single read-deny directory is the re-application's trigger, so either one brings every per-path deny back.
This was referenced Sep 12, 2026
/ write root no longer switches off every write deny/ write root (Linux) or read root (macOS) no longer switches off the denies beneath it
…eny bind The end-to-end arm under a "/" write root opened with `if (!BWRAP_CAN_NAMESPACE) return`, so on a host without user namespaces it passed with no assertions at all and the runner called it green -- while it is the only place the change is executed rather than read off an argv. `it.skipIf` reports a skip instead. - The two runtime arms carried a verbatim copy of the same `run` closure. One now sits beside `wrap`. - `expectDeniedByReadOnlyMount` dropped the `expect(status).not.toBe(0)` it replaced, so a command that never ran satisfied the rest. Put it back inside the helper. - `/` allowed and denied whole pinned an argv where a single `--ro-bind / /` after `--bind / /` is the entire protection, with nothing executing it. A runtime arm now boots that sandbox (payload prefixed with `echo BOOTED`), reads the file through the bind, and shows the write failing and leaving the host file unchanged. Checked by removing the deny: the write then succeeds and the arm fails. - Comment: the pre-pass's lockstep note now names the one exception its own re-check documents -- a symlink appearing between the two passes makes the loop mask that component and emit no bind for the directory.
antdres
approved these changes
Sep 12, 2026
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.
Summary
If
/is inallowWrite, the Linux sandbox silently drops every write deny.denyWriteentries and the built-in protections (.git/hooks,.git/config, shell rc files,.mcp.json,.claude/commands, …) get no read-only bind, over a root that has just been bound writable. This PR makes the two checks responsible treat/as containing every path. macOS has the same slip forallowRead: ['/']and gets the same fix.Example
{ "filesystem": { "allowWrite": ["/"], "denyWrite": ["/home/u/.ssh"] } }--bind / /and nothing for the denies~/.sshand<cwd>/.git/hooksare writable. Each deny is logged "not within allowed paths", at debug level only.--bind / /, then a read-only bind (or placeholder) for each denyCause
"Is this deny path inside the write allowlist?" was written as
For
allowed === '/'the prefix is//, which no path starts with. Two places used that spelling:isWithinAnyAllowedWritePathdecides whether a deny gets a bind at all.findSymlinkInPathdecides whether a symlinked ancestor of a deny path gets the/dev/nullmask that stops it being swapped for a real directory.#502 added root-aware helpers (
isAtOrUnder,isStrictlyUnder) and moved five sibling checks in the same function onto them. These two were missed.Change
isAtOrUnder.denyReadnested inside anallowReaddirectory (Seatbelt is last-match-wins) also compared witha + '/', soallowRead: ['/']re-opened every denied path and no deny was re-emitted. It now usesisStrictlyUnder, and the macOS module's private copy of that helper delegates to the shared one, so the checks that were wrong now share one helper.Who sees a difference
/inallowWrite(Linux) orallowRead(macOS): nothing changes. For any other directory the helper is the same comparison the code spelled inline.allowWrite: ['/']: write denies now apply, including the built-in ones for the current directory. That is the documented behaviour. For built-in denies whose file does not exist, bubblewrap creates an empty placeholder on the host for the life of the command, as it already does under any other write root.filesystem.disabled: true: unaffected. In that mode the manager passes no write config to the backend, so this code does not run and the built-in protections stay off, as documented.Configurations that now refuse to start
Turning the denies back on also turns on a start-up failure that
mainalready has for every other write root. Until now it could not be reached under/, because every deny was skipped.allowWrite: ['/'], plus adenyWritedirectory that contains the current directory and has some allowed write path beneath it. ThroughSandboxManagerthe default write paths (~/.claude/debug,~/.npm/_logs,/tmp/claude) always count as allowed paths, sodenyWrite: ['$HOME']ordenyWrite: ['/']is enough. No read policy is needed.<cwd>/.gitconfig,<cwd>/.mcp.jsonand friends) still get placeholders, bubblewrap cannot create a placeholder inside a read-only mount, and it exits withCan't create file at <cwd>/.gitconfig: Read-only file system(the first placeholder it reaches). Every command fails./itself for a non-root user): the placeholders cannot be created and the sandbox does not start.Merge order
#514, #503 and #524 rework the same function, so please merge this first: it is small, and it deliberately avoids the lines they change. #503 and #524 still carry the old comparison and pick the fix up when they merge
main. #514 already uses the same root-aware rule. It also goes one step further for a root that is both allowed and denied (it treats the paths beneath as covered, where this PR alone emits placeholders bubblewrap cannot create), so when #514 mergesmainit updates the second and third blocks of this PR's last/test to match.Testing
Linux, bubblewrap 0.11.2.
mainand passing here:denyWritefile and directory that no other allow entry covers get their binds under a/root;/root: the ones that exist get binds, the absent ones get placeholders;denyReadnested underallowRead: ['/']is re-emitted./assertions could not fail: they searched for--ro-bind / /, which the base read-only root mount always matches, so they passed whether or not the deny's own bind of/was emitted. They now count whole<flag> <source> <dest>triples, and were checked by removing the deny-side bind: both fail without it.SandboxManager:mainstarts with no deny bind at all, this branch aborts, a cwd outside the denied directory starts on both, and a denied directory with no allowed path beneath it starts with its absent built-in denies skipped as covered.allowWrite: ['/']+denyWrite: ['/']with no read policy now also runs under bubblewrap: it boots, a read through the read-only root succeeds, and a write fails and leaves the host file unchanged.allowWrite: ['/']+denyWrite: ['/']with no read policy is pinned: exactly--ro-bind / /,--bind / /,--ro-bind / /, every per-path deny skipped, and the per-path denies come back as soon as a second allow entry or one read-deny directory exists.tsc,eslintandprettierclean on the changed files. Fullnpm testhas the same failures as a cleanmaincheckout on the same host (all environment-dependent), with five more tests passing. CI is green on all eight jobs.