Skip to content

fix(sandbox): --sandbox read-only was not read-only on Linux - #256

Merged
oratis merged 1 commit into
mainfrom
test/linux-sandbox
Aug 9, 2026
Merged

fix(sandbox): --sandbox read-only was not read-only on Linux#256
oratis merged 1 commit into
mainfrom
test/linux-sandbox

Conversation

@oratis

@oratis oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Stacked on #252. Retargets to main when that merges.

Closes the 仍未做 item in docs/THREE_WAY_REVIEW.md §7: "Linux (bwrap) 侧只共享了 sandbox 模式解析,没有在 Linux 主机上做过 #226 那样的实测。"

Writing the missing test found a real bug on the first run.

The bug

buildLinuxBwrapArgs ends with:

// cwd is rw by default
args.push('--bind', cwd, cwd);

unconditionally, after the allowRead loop. bwrap applies binds in order and the last one wins, so under read-only:

  1. sandboxConfigForMode sets allowWrite: [] and puts cwd in allowRead — correct;
  2. the loop emits --ro-bind-try <cwd> <cwd> — correct;
  3. this line then emits --bind <cwd> <cwd>, overriding it.

The mode resolved correctly. The arguments were generated correctly by their own tests. Between two correct halves, deepcode --sandbox read-only let a command write to the workspace on Linux.

macOS never had this — buildMacOsProfile grants writes only from allowWrite, which read-only leaves empty.

Why it survived

#226 introduced the --sandbox axis and live-verified it on macOS, where it caught the mirror-image bug: a (deny default) profile with no cwd rule, so an enabled sandbox could not read the project directory. Linux got the shared mode resolution and nothing else.

The existing bwrap-integration.test.ts only ever exercised the legacy { enabled: true } shape. No test had spawned bwrap in a named mode to see what a command could actually do — and that is the only kind of test that could have caught this, since both halves pass in isolation.

Change

  • read-only ro-binds cwd instead of rw-binding it. An absent mode keeps the read-write bind, so callers on the legacy enabled: true shape are unaffected.
  • Integration tests across the mode axis: read-only readable, read-only not writable, workspace-write readable+writable, workspace-write still blocked outside, danger-full-access produces no bwrap at all, defaultMode applies when config names none, and a library caller with no mode anywhere stays unsandboxed.
  • Argument-level regression tests for the bind itself.

No silent skips

Linux CI sets DC_REQUIRE_BWRAP=1, turning a missing binary into a failure. These are the only tests that observe what the sandbox does rather than what it builds, and they self-skip — a green suite that skipped every real enforcement check is how this survived in the first place.

Verification

typecheck, lint, format, docs clean; full suite green through the pre-commit hook. The bwrap integration tests cannot run on macOS, so the behavioural half is verified by this PR's Linux CI job, not locally — the argument-level assertions are what I could run here, and they fail against the old code.

🤖 Generated with Claude Code

@oratis

oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Review — approve, no changes

This is the best kind of finding: two halves that each pass their own tests, and a bug that only exists between them. sandboxConfigForMode put cwd in allowRead, buildLinuxBwrapArgs emitted the --ro-bind-try, and then overwrote it two lines later. Nothing was wrong in isolation, which is why argument-level tests could never have caught it — bwrap's last-bind-wins is a property of bwrap, not of the argument list.

The framing is also right: --sandbox read-only on Linux was not read-only, and the mode axis was live-verified on one platform only. The macOS mirror-image failure from #226 (a sandbox that could not read the project) is a good foil, because it shows the same "verified on one platform" gap producing the opposite symptom.

Checked:

  • if (config.mode === 'read-only') and not !== 'workspace-write' — an absent mode keeps the read-write bind, so a caller on the legacy {enabled: true} shape is genuinely unaffected. That is the difference between a fix and a silent capability change for embedders.
  • DC_REQUIRE_BWRAP: ${{ runner.os == 'Linux' && '1' || '0' }} evaluates correctly ('1' on Linux, '0' elsewhere), runner is available in step env, and the Linux job already apt-get install -y bubblewrap — so turning the skip into a failure will not fail the job for want of a binary. The green Linux check on this PR is the proof that matters.
  • The self-skip → hard-fail change is the general lesson here. A skipped suite reads as a passing one, and that is how a sandbox stayed writable through a release.

--sandbox read-only letting a command write to the workspace is a correct 🔒 Security classification.

@oratis
oratis changed the base branch from fix/git-env-test-isolation to main August 9, 2026 15:38
`buildLinuxBwrapArgs` ended with an unconditional `--bind <cwd> <cwd>`. bwrap
applies binds in order and the last one wins, so under `read-only` the
`--ro-bind-try` that `sandboxConfigForMode` had correctly asked for was
overwritten a few arguments later. The mode resolved right, the profile said the
right thing, and a command could still write to the workspace.

macOS never had this: `buildMacOsProfile` grants writes only from `allowWrite`,
which read-only leaves empty. #226 introduced the mode axis and live-verified it
on macOS — where it caught the mirror-image bug, a profile that denied reads of
the project directory. This is the half nobody looked at, which is exactly what
THREE_WAY_REVIEW recorded under 仍未做.

Found by writing the missing test rather than by reading the code: the bwrap
integration suite only ever exercised the legacy `enabled: true` shape, so no
test had spawned bwrap in a named mode to see what a command could actually do.
The mode resolution was unit-tested and the arguments were argument-tested, and
between those two correct halves the behaviour was wrong.

An absent mode keeps the read-write bind, so callers on the legacy shape are
unaffected.

Linux CI now sets DC_REQUIRE_BWRAP=1. These are the only tests that observe what
the sandbox does rather than what it builds, and they self-skip — a green suite
that skipped every real enforcement check is how this survived.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@oratis
oratis force-pushed the test/linux-sandbox branch from 7c12cee to 3902beb Compare August 9, 2026 16:00
@oratis
oratis merged commit d901879 into main Aug 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant