fix(agent): expand shell variables before the workspace fence scans - #527
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the remaining variable and cd forms that bypass the workspace fence must be handled.
I reviewed the full diff plus the ExecTool callers, the permission policy's shell parsing, DirectExecutor and Boxlite execution, and the relevant history. I also checked AGENTS.md, CLAUDE.md, and CONTEXT-MAP.md; the branch/commit/source/test naming rules are satisfied and no new domain term needed definition. The tests are additive rather than weakened, and I checked backward compatibility on both direct and sandboxed call paths.
Local verification: uv run --frozen pytest -q tests/test_shell_workspace_fence.py tests/test_shell_comments.py tests/test_sandbox_unit.py passed with 166 tests. Direct probes against _check_workspace_restriction reproduced each inline bypass while confirming the advertised direct $HOME case is blocked. Because these commands still reach outside the configured roots through the exact shell features this change now interprets, this revision is not ready to merge.
a214cf3 to
1022f0e
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: wrapped shell payloads, nested fallback expansions, and platform-inaccurate percent expansion still need correction.
I re-reviewed the complete diff and the six follow-up commits, including the permission-policy helpers, DirectExecutor/Boxlite callers, relevant history, backward compatibility, and the repository rules in AGENTS.md, CLAUDE.md, and CONTEXT-MAP.md. The existing tests were not weakened; the new tests cover the four reported reproducers, and I independently confirmed the brace-trim and cd -- fixes, replied to their threads, and resolved them.
The nested-shell and Windows threads remain open with concrete wrapper/platform reproducers, and one new nested-parameter finding is inline. Nonblocking follow-up: _steps_outside resolves every cd from the initial cwd, so cd subdir; cd ..; ls is refused even though it returns to the workspace; this has a straightforward compound-command workaround and does not add another merge blocker.
Local verification passed: uv run --frozen pytest -q tests/test_shell_workspace_fence.py tests/test_shell_comments.py tests/test_sandbox_unit.py (202 passed) and uv run --frozen pytest -q tests/test_permissions_gate.py tests/test_shell_approval.py (467 passed). git diff --check github/main...HEAD also passed.
1022f0e to
59dd715
Compare
|
All three blocking findings are fixed, each in its own commit, and answered in
The branch was rebased onto the current base after your review, so the shas in my Verification at the new head: 39 tests and 98 cases in the fence file, all One disclosure that is not a finding of yours. A second full-suite failure, |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: cd state tracking must not assume skipped or failed directory changes succeeded.
I re-reviewed the complete diff and the four new fix commits, including the shell-policy helpers, DirectExecutor and sandbox callers, relevant history, backward compatibility, and the repository rules in AGENTS.md, CLAUDE.md, and CONTEXT-MAP.md. I verified the wrapper, platform-percent, and nested-brace fixes against their reproductions, replied to the author's explanations, and resolved all three prior threads.
The remaining blocker is new in the latest directory-state change and is marked inline. I reproduced it end to end with a temporary workspace: the fence returned allowed and DirectExecutor printed the contents of a sibling file outside the workspace after the preceding cd failed.
Local verification otherwise passed: uv run --frozen pytest -q tests/test_shell_workspace_fence.py tests/test_shell_comments.py tests/test_sandbox_unit.py tests/test_permissions_gate.py tests/test_shell_approval.py (693 passed), and git diff --check github/main...HEAD passed. The tests are additive, but the new semicolon cd case does not create its target directory, so it currently asserts allowed for the same failed-cd sequence that escapes in a real shell.
59dd715 to
ecf397e
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The standing directory-state blocker is fixed: the separator-aware walk now refuses failed/skipped cd, ||, pipeline, background, subshell, and pushd escapes while retaining proven && walks. The original thread is resolved.
Named follow-up: conditional state before a cd is still not represented. test -d subdir && cd subdir && echo ready; cd ..; cat outside.txt is allowed and reads the parent when the test fails. This is nonblocking at this round because target main already has no directory-state scan; the revision substantially reduces that pre-existing class rather than introducing it.
I covered the repository rules, the complete diff against github/main, this revision's delta and history, the separator parser's existing callers, backward-compatibility of the token-only wrappers, and whether tests were weakened. The changed semicolon expectation corrects an unsafe test; the proven && cases remain covered. Verification: uv run pytest -q tests/test_shell_workspace_fence.py tests/test_shell_comments.py tests/test_sandbox_unit.py tests/test_permissions_gate.py tests/test_shell_approval.py passed (707 tests), manual fence-plus-bash reproductions confirmed both the fixes and the named follow-up, and git diff --check github/main...HEAD passed.
|
Not a blocker -- an A stands beside this, and the change is a clear improvement. But the one test The fence stops looking past four nested shells, and allows. Measured on this head, wrapping
This is NOT a regression, and that is why it is a note. The same probe on the merge-base What is this PR's is the test. tests/test_shell_workspace_fence.py:551-559, which is the complete range of So the control proves the suite reaches the recursion -- nine tests die when it is removed -- and the The rest of the file is strong: reverting both production files to base leaves 60 of its 112 failing, Smallest thing that would close it: assert the exact answer for that input, whichever the intended |
The fence extracted candidate paths from the command as typed, then called
os.path.expandvars on what it had extracted. Extraction wants a "/" sitting on
a word boundary, and "$HOME/" puts a letter there, so a variable spelling
produced no candidate at all and the expansion never saw the text that hid the
path. "cat /etc/shadow" was refused while "cat $HOME/.ssh/id_rsa" ran.
An unset name is the sharpest spelling: the shell drops it, so
"$NOPE/etc/shadow" is /etc/shadow, and there is no literal form to fall back
on. Nothing else was looking either, because "cat" is read-only and the
permission gate tiers a read-only command "allow"; for a read the fence was the
only thing in the way.
Expand first, with the shell's own rules, then scan:
- an unknown name expands to nothing, as the shell does;
- the environment that decides is the child's allowlisted baseline, not
os.environ, so a name the command will never be given reads as unset;
- single quotes expand nothing, so "echo 'keys go in $HOME/.ssh'" stays a
sentence;
- PWD resolves to the directory the command runs in, because a shell sets it
from there whatever this process inherited.
A brace body stands beside the value rather than being applied to it, since
which one the shell uses depends on a value this cannot read. Its word
operator becomes a space so the word starts on a boundary; the operators that
reshape a value instead of offering a word are left alone, so ${TERM%-256color}
keeps working.
Stepping out is the same escape as reaching out, so a cd out of the workspace
is refused as well. Only the destinations the scan cannot see needed this:
"/" has nothing after it, ".." is not absolute, and a cd with no argument names
$HOME by saying nothing. "cd /etc" was already refused, because /etc is a path.
The fence returns before any of this when restrict_to_workspace is off, which
it is by default, so an install that did not raise it runs none of the new
code.
Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
…ation `cd -- /; cat etc/passwd` was allowed. The argument walk dropped `-L` and `-P` and then took the first remaining word as the destination, so `--` itself was read as a directory name, resolved inside the workspace, and passed. The bare `/` after it is invisible to the path scan for the reason the scan already documents, so nothing else was looking. Past the terminator every word is an operand: a `-` there names a directory rather than $OLDPWD, and no word at all means what a bare `cd` means. The flag skip becomes a leading-only walk for the same reason, so `cd -- -L` names its directory instead of losing it. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`sh -c 'cat $HOME/secret'` was allowed. The single quotes suppress expansion in the outer shell and the fence was right to leave them alone -- but what they enclose is the inner shell's program, and the name expands there. Reading only the outer shell left the payload unscanned while the outer quoting looked handled, and a nested `cat` is a read, which the permission gate tiers `allow`. The payload is now scanned with the same rules, recursively, bounded by the policy's own embedded-shell depth. The payload is found with the policy's helper rather than a second parser, so the fence and the deny list agree on what counts as a nested shell. An escaped `$` now yields the bare character. It is a literal `$` that this level does not expand, but it still travels on, and a nested shell handed it does expand it; keeping the backslash hid two-deep nesting from the scan, because the payload no longer looked like it named anything. Every other escape keeps both characters, since only `$` and a backtick decide whether something expands. Segmenting moves up to the caller so one lexer pass serves the directory check and the payload search. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
… cannot
`cat "${PWD%/*}/outside.txt"` was allowed. Removing a suffix was read as inert
text standing beside the value, so the scan saw the in-root `$PWD` and never
the parent the shell opens. A trim is not decoration on a value; it walks a
path UP, which is exactly how one leaves the workspace.
`%`, `%%`, `#` and `##` are now applied, with the doubled forms taking the
longest match and the single ones the shortest, so `${HOME##*/}` resolves to a
bare name and the command that uses it keeps running rather than being refused
for the value it no longer carries.
That leaves the posture, which the four bypasses found in one round argue for
more than any single one of them: every spelling the fence did not model was
allowed, so each gap was silent and the next one would be too. A parameter
expansion is now resolved faithfully or the command is refused. The set of
spellings is finite, so the residue is closed; substitution, case folding,
indirection and offsets each refuse with the construct named. A refusal is
visible and can be argued with, which is what the silent version could not be.
Command substitution stays a declared limit rather than joining the refusal: it
holds an arbitrary program, no textual guard can resolve it, and refusing it
would take `echo "built at $(date)"` with it. Literal paths inside one are
still scanned, as before. A count and an arithmetic result are exempt for a
reason that does not depend on modelling them: both yield a number, and a
number cannot name an absolute path.
Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`DirectExecutor` runs the platform shell, so on Windows the ordinary spelling of an outside path is `%USERPROFILE%\.ssh\id_rsa`, not `$HOME/...`. The name pattern knew only the POSIX form and the Windows path pattern wants a drive prefix the unexpanded text does not carry, so both halves of the scan looked past it. The environment allowlist names USERPROFILE, APPDATA and their siblings, so the fence was always meant to hold there. A name is substituted only when the child is given it. That is cmd.exe's own rule for an undefined name, which it leaves standing as written, and it is what keeps this out of the way on a POSIX host: the Windows names do not exist there, so nothing is rewritten and `date +%Y%m%d` survives. The check sits ahead of the quote branches because cmd.exe has no quoting that suppresses it; hiding a run it would expand behind POSIX quoting rules would under-block on the platform the rule is for. Verified by asserting the expanded text rather than the verdict: a Windows path is not absolute to a POSIX Path, so a verdict test would turn on the host running the suite instead of on the expansion. The end-to-end refusal on Windows is not reproduced here. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
…not run
The directory check opened by stripping a leading `(` or `{` from the first
token. It never fires: the policy's splitter lists `(` and `)` as operators and
treats a standalone `{` as a word operator, so `(cd /; x)` arrives as its own
segment with the bracket already gone. Dead code in a guard reads as handling
that is not happening, which is worse than the gap it pretends to close. A
brace-group case now pins the behaviour it was pretending to provide.
Four branches had no test and would have been taken on trust: the two
destinations the fence deliberately does not guess at (`$OLDPWD` and an empty
operand), a destination that cannot be resolved at all, and the recursion cap.
The cap is asserted as "it still answers" rather than by its verdict, because
asserting which way it answers at the bottom of the nesting would be asserting
a hole rather than a behaviour.
Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
… ruff wants N818. Caught by the linter locally rather than by CI, which runs the same rule in its python lint job. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`command sh -c 'cat $HOME/secret'` and `env sh -c '...'` were allowed. The payload search read the segment as written, while the permission policy runs `_unwrap_command_wrappers` over a segment before asking what it executes. Reusing the payload helper without the unwrap that precedes it there reused half the agreement, and a wrapper in front of the nested shell put its program back out of view. Both readers of a segment now unwrap first. That also closes `command cd /`, which named the same builtin behind the same wrapper and was refused only when spelled bare -- found by writing the wrapper cases out rather than by the report, which named the `sh -c` forms. Assignments, `env`, `sudo` and `command` are covered because the policy's helper covers them; keeping one list rather than a second is the point of calling it. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
…gnore case Two directions, both mine from the previous revision. The pass ran unconditionally, so on POSIX `echo '%HOME%/notes'` was refused while `sh` prints that text literally. The reasoning given for running it everywhere was wrong: it claimed the Windows names do not exist on a POSIX host, but HOME, PWD, USER and TMPDIR are on the executor's allowlist on both. The test offered as proof used `date +%Y%m%d`, which passes only because `Y` and `m` happen to be unset, so it never exercised the direction it was cited for. `%VAR%` is cmd.exe syntax and is now read only where cmd.exe is the shell. The other direction: cmd.exe resolves an environment name without regard to case, so the exact-key lookup left `%UserProfile%` standing and the Windows path scan then saw no drive prefix. The lookup now folds case, and a name the child does not have is still left as written, which is what cmd does. The platform answer is a class attribute rather than a call to `os.name`, so the Windows branch can be driven from a POSIX host. Both directions are tested; the end-to-end refusal on Windows still is not, for the reason the earlier commit gave. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
… cannot parse
`cat ${UNSET:-$HOME/secret}` was allowed. A body is emitted after the walk has
passed the position it lands in, so nothing read it again; the shell does read
it, and expands the `$HOME` inside. Trim patterns had the same shape, so
`${PWD%$PWD}/etc/passwd` compared against an unexpanded pattern and matched
nothing.
This was the one spelling the resolved-or-refused contract claimed and did not
cover: neither expanded nor refused, handed through instead. The body now goes
back through the same pass, so a parameter in it is expanded or refused on the
same terms as one anywhere else, and a fallback that resolves inside the
workspace still runs.
The brace pattern stops at the first `}`, so nesting is something this cannot
parse rather than something it resolves. Under the same contract that is now a
refusal instead of a fall-through to text, which is what `${UNSET:-${X:-/etc/
shadow}}` relied on.
Recursion terminates because a body is strictly shorter than the text holding
it, and a value is substituted rather than re-read. Checked at 200 levels of
nesting and 500 parameters in one body: both answer without a RecursionError.
Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`cd subdir; cd ..; ls` was refused. Every `cd` resolved from the directory the command started in, so the second was read as if the first had not happened and a walk back out of a subdirectory looked like leaving. The sequence ends where it began. Carrying the directory forward is off wherever a bracket appears. A `cd` inside a subshell does not outlive it, and the splitter has already dropped the bracket that said so, so carrying past one would read `(cd subdir); cd ..` as returning to the workspace while the shell sits a level above it. Where the structure cannot be seen the stricter reading holds, which is the same rule the expansion contract follows. The bracket test is crude on purpose: a literal bracket in an argument costs only strictness. A `../` anywhere is still refused by the traversal rule before any of this runs, so the sequences here are spelled as repeated `cd ..`; the shorter spelling would pass for a reason unrelated to where the walk starts. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`cd missing; cd ..; cat outside.txt` read the file. The walk advanced to workspace/missing on the `cd`, so the `cd ..` after it looked like a return to the workspace; the real shell's `cd` failed, the `;` carried on from where it stood, and the `cd ..` left the fence. Checked against bash rather than reasoned about: five spellings of this reach a file outside the workspace, and the fence allowed every one. Only `&&` proves the command before it succeeded, because it runs its right side *because* the left one returned zero. After every other separator the move may not have happened at all: the directory may not exist, `||` runs only when it did not, and a pipe, a background `&` or a bracket runs the `cd` in a subshell that takes its directory with it. One position could not hold that. `cd a && cd b; cd ..; cd ..` needs both the place an unproven `cd` would have left and the place it would not, because the escape is the combination of the two: keeping either alone allows it. The walk now carries the places the shell can be standing in, converging on `&&` and holding both readings after anything else, and refuses when any of them leaves the workspace. `_split_on_operators` grew a sibling that yields each piece with the operator that terminated it, and `_command_segments` the same. Both originals are thin wrappers over the new ones, so no existing caller reads anything new. An existing assertion changed meaning, and is called out rather than left in the diff. The test that kept a walk back from being refused carried `cd subdir; cd ..; ls` and `cd a; cd b; cd ..; cd ..; ls`. The fixture's workspace is empty, so `subdir` never exists, and those spellings are the escape rather than the round trip they were written for. They move to the refusal test, the `&&` spellings stay, and the refusal test gains `&&` cases of its own -- the only ones that reach the converging branch at all. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`pushd ..; cat outside.txt` read the file. The walk asked whether a segment's first word was `cd`, so the builtin that moves the shell exactly as far, and is on every interactive user's fingers, stepped over the fence untouched. Its destination is now checked the same way `cd`'s is. The stack is what the walk cannot follow. `popd` and a bare `pushd` take their destination from entries an earlier push put there, and each of those was cleared on the way in, so the shallowest place either can land is where the command began. The walk adds that place rather than guessing which one, which is the same answer a separator that proves nothing already gets. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
`test -d subdir && cd subdir && echo ready; cd ..; cat outside.txt` read the file. The `&&` after the `cd` was taken as proof it succeeded, which it is -- but only if the `cd` ran, and the condition standing before it in the same chain can skip it. The walk converged on that unproven `&&` and dropped the reading where the shell never moved, so the `;` after the chain ran from a place it was no longer holding. Checked against bash, in a workspace with and without `subdir`, rather than reasoned about: four spellings of the guard reach the parent, and the fence allowed all four. A chain stops at its first failure, so what follows one inherits the position after any prefix of it -- none of it, some of it, all of it. The walk now carries those prefixes and unions them where the chain ends, which is at the separator of any segment rather than of a `cd`: the reproducer leaves its chain on `echo ready;`, and a rule that only looked at `cd` separators never saw it. The per-segment move is lifted into `_moved` so the chain bookkeeping reads as one thing, and because the branch that ends a chain has to run for every segment, not only the ones that return early. Both directions are held. Never ending a chain turns 10 cases red -- every escape class, including these four. Ending it at every separator turns 6 red -- the proven walks that must keep running. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
ecf397e to
741716b
Compare
|
The named follow-up is fixed rather than carried, at 741716b. Reproduced first, against bash, in a workspace with and without You located it exactly. The The correction is that the chain, not the step, is the unit the walk carries. A The part I had wrong twice is where a chain ends. It ends at the separator of Re-measured at the new head over fourteen shapes, each in both fixture states: Held from both sides rather than one. Never ending a chain turns 10 cases red, One thing I am not claiming to have closed: a directory that appears between the |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The named guard-chain follow-up is fixed. I reproduced all four forms from the author’s note: each is now refused, while the proven && walks remain allowed. Tracking the chain’s prefix positions and ending it on separators from every segment addresses the specific state loss without changing the token-only policy callers. The separately recorded recursion-cap note remains nonblocking and is not duplicated here.
I covered the repository rules, the revision delta and full PR file set, the separator walker and its callers, commit history, backward compatibility, test expectations, and the existing architecture boundary. The new tests add coverage for the reported leak and do not weaken prior assertions. Verification: uv run pytest -q tests/test_shell_workspace_fence.py tests/test_shell_comments.py tests/test_sandbox_unit.py tests/test_permissions_gate.py tests/test_shell_approval.py passed (711 tests); manual fence checks passed for the four blocked guards and three allowed walks; git diff --check github/main...HEAD passed. All review threads I opened are resolved.
|
Not a blocker -- but it is a behaviour regression against both The escape it closes is real and the fix is the right shape. What follows is the The chain collapse fires at
|
Summary
restrict_to_workspaceis an operator's promise that the agent cannot reach outsideits workspace. It kept that promise only for paths spelled literally.
ExecTool._check_workspace_restrictionextracted candidate paths from the command astyped, then called
os.path.expandvarson what it had extracted. Extraction wants a/sitting on a word boundary, and$HOME/puts a letter there, so a variablespelling produced no candidate at all and the expansion never saw the text that hid
the path. The expansion sat downstream of the extraction it needed to feed.
An unset name is the sharpest spelling: the shell drops it, so
$NOPE/etc/shadowis/etc/shadow, and there is no literal form to fall back on. Nothing else was lookingeither, because
catis read-only anddefault_tieranswersallowfor a read-onlycommand without asking; for a read the fence was the only thing in the way.
Measured with the fence on, before this branch and after it:
cat /etc/shadowcat $HOME/.ssh/id_rsacat "$HOME/.ssh/id_rsa"cat $NOPE/etc/shadowcat ${NOPE:-/etc/shadow}cat ${UNSET:-$HOME/secret}cat ${PWD%/*}/outside.txtcat ${PWD%$PWD}/etc/passwdcd /; cat etc/shadowcd -- /; cat etc/passwdsh -c 'cat $HOME/secret'env sh -c 'cat $HOME/secret'command cd /; cat etc/shadowcat $PWD/notes.txtecho 'keys go in $HOME/.ssh'cd subdir; cd ..; lscd missing; cd ..; lscd subdir || cd ..; lscd subdir | cat; cd ..; lscd subdir & cd ..; lspushd ..; lstest -d subdir && cd subdir && echo r; cd ..; lscd subdir && cd .. && lscd subdir && cd ..; cat notes.txtThe fix expands first, with the shell's own rules, then scans. Each rule is taken
from the shell rather than invented:
os.environ,so a name the command will never be given reads as unset;
echo 'keys go in $HOME/.ssh'stays a sentence;$yields the bare character, which this level does not expand but anested shell handed it does;
or a trim pattern is read on the same terms as one anywhere else;
%NAME%is read only where cmd.exe is the shell, and there without regard to case,because that is what cmd does and
%VAR%means nothing tosh;PWDresolves to the directory the command runs in, because a shell sets it fromthere whatever this process inherited.
A nested shell's payload is scanned in the shell that runs it, recursively, through
the policy's own embedded-shell helper and depth bound. Both readers of a segment
unwrap
env,sudo,commandand leading assignments first, through the policy'sown helper, so the fence and the deny list agree on what a segment runs rather than
keeping two lists.
Stepping out is the same escape as reaching out, so a
cdout of the workspace isrefused as well. Only the destinations the scan cannot see needed this:
/hasnothing after it,
..is not absolute, and acdwith no argument names$HOMEbysaying nothing.
cd /etcwas already refused, because/etcis a path like anyother. Each
cdstarts from where the last one landed, but only where a separatorproves it got there:
&&runs its right side because the left one returned zero.After anything else the move may not have happened, so both readings are held and a
later step that leaves from either is refused.
The posture
Bypasses of one family kept appearing: everything the fence did not model was
allowed, so every gap was silent and the next one would be too. The contract is now
stated on
_check_workspace_restriction:A parameter expansion is resolved faithfully or the command is refused. The set
of spellings is finite, so the residue is closed. Substitution, case folding,
indirection, offsets and a brace this cannot parse each refuse with the construct
named. A refusal is visible and can be argued with; the failure it replaces could
not be.
Command substitution is a declared limit, not a gap. It holds an arbitrary
program, so no textual guard can resolve it, and refusing it would refuse
echo "built at $(date)"along with everything else. Paths written literally insideone are still scanned. Two constructs are exempt from the refusal for a reason that
does not depend on modelling them:
${#NAME}and$((...))yield numbers, and anumber cannot name an absolute path.
The same rule decides what the directory walk may carry. A
cdthe shell may nothave run cannot be carried, and the separator is what says whether it ran: a bracket
or a pipe puts it in a subshell,
||runs it only when the one before failed,;continues whether it failed or not. Only an
&&chain is knowable, and the chain isthe unit rather than the step -- it stops at its first failure, so what follows one
inherits the position after any prefix of it. The walk carries those prefixes and
unions them where the chain ends.
The directory walk took three rounds, and the second and third findings were both in
code written to close the one before. Recorded because the shape repeats: the first
version advanced the walk on every
cd, so acdinto a directory that does notexist left the walk a level deeper than the shell and the
cd ..after it read as areturn. The second read the separator after each
cd, which closed that and fourmore spellings of it (
||, a pipe, a background&, a bracket) but treated&&asproof the
cdran -- and a condition earlier in the same chain can skip it. The thirdmakes the chain the unit: it stops at its first failure, so what follows inherits the
position after any prefix of it.
Every one of these was measured against bash rather than reasoned about, in a
workspace with and without the directory the command names, because which branch
leaks depends on that.
pushdwas found the same way and is the one finding herethat came from neither review round.
Widening the fence from "no outside path is named" to "and the command does not walk
out" is the one deliberate contract change beyond the posture. Without it the rest is
bypassed by
cd /etc; cat shadow.Checked and deliberately not changed:
allowand asks nobody. Thatis why this defect mattered, but it is a separate control that applies to every
install including the ones with the fence off, so changing it is not in scope for
making the fence keep its own promise.
cat linknames no absolutepath, so there is nothing for a textual scan to refuse. Both sides of the comparison
already resolve physically, so a named path cannot be laundered through a symlink.
The sandbox executor is the boundary for both of these, which is what the code says
about itself.
Blast radius:
_check_workspace_restrictionreturns before any of this whenrestrict_to_workspaceis off, which is the default, so an install that did not raisethe fence runs none of the new code.
Type
Verification
Branch scoped with three dots against the rebased base:
git rev-list --left-right --count origin/main...HEADreports0 14.COLUMNS=200 uv run --frozen --python 3.12 --all-extras pytest -q2 failed, 23826 passed, 112 skipped in 636.80s under coverage. A second run of
the same head gave 1 failed, 23803 passed.
The one failure is
test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for("node without npm provisions a runtime"). It is not introduced here: it fails
identically with this change removed, and it fails the same way when its file is run
alone, so it is neither this branch's nor an ordering artifact.
A second failure is intermittent and its attribution is left open rather than
claimed:
test_agents_research_tools.py::test_a_thin_dataset_card_falls_back_to_the_metadata_the_table_needs(
KeyError: 'served_url'). It appeared in two of three full-suite runs of thisbranch and not in the one full-suite run taken with this change removed, which is a
single baseline sample and not enough to rule the branch out. Against that: the test
passes when its file runs alone both with and without this change, and it passes
when its file and the new one run together in a single process, so the new tests do
not reach it directly. The mechanism that does fit is in that file: it selects a
process-wide
ContextVar29 times and never restores the token, so its state leaksto whichever tests share its xdist worker, and the default
--dist loaddecidesthat per run rather than by a fixed split. Adding tests changes that distribution
without changing any code the test touches. Not changed here, since the isolation
defect is that file's own.
uv run --frozen --python 3.12 --all-extras pytest tests/test_shell_workspace_fence.py -q43 tests, 116 cases, all passing. Every case added for a reported bypass was watched
failing before its fix; the rest are over-blocking guards, which pass on both sides
by design and are there to fail if a fix reaches too far.
Reverting only the first commit's hunk in
raven/agent/tools/shell.py, with the newtests left in place, turned 13 of the then 38 cases red: every case written for the
defect, and none of the over-blocking guards. The tree was restored afterwards and
git diff HEADconfirmed empty, since a three-way reverse-apply stages what itapplies.
make coverage-diff(the gate that failed on the first revision):Diff coverage: 97.89% (186/190 executable changed lines).Recursion through brace bodies terminates because a body is strictly shorter than
the text holding it and a value is substituted rather than re-read; checked at 200
levels of nesting and 500 parameters in one body, both answer without a
RecursionError.Fence verdict against a real bash, fourteen shapes, each run in a workspace with and
without
subdir:holes: 0 over-blocks: 0. A hole is allowed-and-leaks; anover-block is refused-while-neither-branch-leaks, which is why both fixture states
are needed to claim either.
The walk's separator rule is held from both sides. Never ending an
&&chain turns10 cases red, every escape class; ending it at every separator turns 6 red, the
proven walks that must keep running. Dropping
pushd/popdrecognition turns 3red; recognising
popdwithout its worst-case reset turns 1.pytest tests/test_*shell*.py tests/test_permissions*.py tests/test_*exec*.py tests/test_sandbox*.py-- 958 passed.ruff checkandruff format --checkclean on both changed files.scripts/check_commit_messages.py,scripts/check_large_files.pyandscripts/check_source_language.pyoverorigin/main..HEAD: all pass.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
The docs box is unchecked because nothing documents the fence's path semantics. The
only mention outside the code lists
restrictToWorkspaceas a config field, whichthis does not change; there was no published claim describing the old behaviour.
Not reproduced here: the end-to-end refusal on Windows. A Windows path is not absolute
to a POSIX
Path, so a verdict test would turn on the host running the suite ratherthan on the expansion. The percent-expansion tests assert the expanded text and drive
the Windows branch through a class attribute instead.
Risk
With the fence on, commands that name an outside path through a variable, that
cdout of the workspace, or that use a parameter expansion the fence cannot resolve are
now refused where they previously ran. The first two are the control doing what its
name says. The third is the posture, and it is the one that can refuse a command that
would have been harmless:
echo ${NAME^^}names no path and is refused becausenothing here can prove it does not.
That trade is deliberate. An over-block is visible to the operator and can be argued
with or reported; an under-block is silent, and eight of them were found across two
review rounds on this branch before it was ready.
One shape that was refused mid-branch now runs: a
cdsequence chained with&&that returns to the workspace, such as
cd subdir && cd .. && ls. The;spellingof the same walk is refused, and correctly: when the directory does not exist the
cdfails, the;carries on from where the shell stood, and thecd ..leaves theworkspace. Measured against bash, it reads the parent.
Nothing changes for a default install, where the fence is off and this code does not
run.
Over-blocking was the failure mode guarded against throughout, and one instance of it
was introduced and then removed within this branch: reading
%VAR%on POSIX refusedecho '%HOME%/notes', whichshprints literally. Workspace-relative$PWDuse,single-quoted text containing a
$,cdinside the workspace and back,cdinto anoperator-added extra directory,
cd -- subdir, a wrapper in front of inside work, anested payload that stays inside, a fallback that resolves inside, a trim that
resolves to a basename or matches nothing,
${#NAME},$((...)),date +%Y%m%d,printf '%s%s'andcut -d/style delimiter arguments all still run, each with atest.
Rollback is reverting these commits; the fence returns to its previous behaviour with
no migration and no persisted state involved.
Related Issues
N/A