Repository navigation
(sandbox): protect repositories below a symlinked Additional Directory - #538
3 commits merged into
Conversation
… protected find does not descend into a starting point that is a symbolic link, so a repository or a .claude below an Additional Directory given as a link is never found, and its .git/config, hooks and settings stay writable.
…hen protecting below it find does not descend into a starting point that is a link; a trailing slash makes it enter the directory the link points to.
devsuitup
left a comment
There was a problem hiding this comment.
Review at 4fc768f — approve (2 minor, non-blocking).
Verified
- Red-first in CI: test-only 81cf86b, run 38039845996, fails test (20/22, ubuntu) on exactly
a repository and a .claude below an Additional Directory that is a link are protected(.../link/sub/.git/config must stay read-only, expected '--ro-bind', actual '--bind'), 1 of 5121. Head 4fc768f, run 38040407529: green, test unchanged between the two commits, and it runs in required jobs. - The fix (
find "$d"→find "$d/",scripts/claude-sandbox.sh:615) makes GNU find enter a starting point that is a link; a plain directory yields the same paths as before. - Main vs PR script under Git Bash with a recording fake bwrap: absolute link, relative link and a chain
l2 -> l1 -> other—.claude,.git/config,.git/hookswritable on main, read-only with the PR,.claude/worktreesstill writable; a link into the project is read-only at both paths; a bind already ending in/gives//paths but stays consistent and read-only. Missing target, link to$HOMEor a parent, or into.git/.claudeare rejected earlier, unchanged. - Changelog under Unreleased / Fixed, no comments added,
docs/sandbox.mdalready promises this protection.
Minor
CHANGELOG.md:8leaves out a second visible change: a symlinked Additional Directory holding a nested.claudethat is itself a link (or a nested repo git cannot read) now refuses to start with status 125, where main started it unprotected (refusing to launch: /lo/c/.claude is a symbolic link). The refusal is right — it is what a plain directory already does — worth one sentence in the entry.- The regression test checks the recorded bwrap argv only; the suite already runs real-bwrap tests on ubuntu (e.g.
from inside a real sandbox, job 114179348966). Adding a symlinked Additional Directory to the existing real-sandbox persistence test would pin the actual write.
Merge before #483, which carries these two commits.
Not run here: real bwrap / Linux (Git Bash and a fake git are an approximation).
…de a real sandbox Also name in the changelog the refusal a linked Additional Directory now shares with a plain one.
|
Addressed at 5680fee.
|
devsuitup
left a comment
There was a problem hiding this comment.
Re-review at 5680fee — approve. Both minors from the 4fc768f review are closed; behaviour is unchanged (git diff --stat 4fc768f 5680fee: CHANGELOG.md 1 line, test/sandbox-wrapper.test.js +19/-1, scripts/ untouched).
| Previous minor | Status |
|---|---|
| Changelog should mention the status-125 refusal | closed — CHANGELOG.md:8 now says a .claude that is itself a link, or a repo git cannot read, refuses to start "as a plain directory already did" |
| Real-bwrap test | closed — test/sandbox-wrapper.test.js:1278-1292, :1340-1344 bind linked -> other through SWITCHBOARD_SANDBOX_BINDS and, from inside a real sandbox, try git config core.fsmonitor, a write to .claude/settings.json and a plain file under $linked/sub; on the host the config stays unset, settings.json stays {}, and file holds ok, proving writes do reach below the link |
The real test runs, not skipped: run 38068130533, ok 3112 … from inside a real sandbox … in test (20, ubuntu) job 114259856193 and test (22, ubuntu) job 114259856370, both with SWITCHBOARD_REQUIRE_REAL_BWRAP=1 and the "gets them, not a skip" guard passing. All checks green on 5680fee, merge state clean.
Optional: no run shows this real test failing with the fix reverted; it very likely would (without find "$d/" nothing below the link is made read-only). The red-first gate for the fix itself is met by 81cf86b (run 38039845996) → 4fc768f.
Merge before #483.
8b12045
…Additional Directory)
Split out of #483 at the maintainer's request, so CI shows the regression test failing before the fix.
The bug (on main).
bind_writable_dirprotects every repository and.claudebelow a bound directory withfind "$d" …. When an Additional Directory is given as a symbolic link,finddoes not descend into the starting point (GNU find:find link -mindepth 1prints nothing,find link/lists the contents). A repository or.claudebelow such a directory is then never found: its.git/config,.git/hooksand.claude/settings.jsonstay writable from inside the sandbox, and a hook written there runs outside the sandbox later.Commits.
.claudebelow an Additional Directory that is a link; the recorded bwrap arguments must leave.git/configand.claude/settings.jsonread-only. CI is expected red on this commit.find "$d/", so the starting point is entered when it is a link; a Fixed changelog entry.Not run: a live sandbox launch; the test checks the recorded bwrap argument vector.
🤖 Generated with Claude Code