emrg: the write-site walk reads the shell's other move verb - #1382
Conversation
pushd <dir> moves the shell exactly as cd <dir> does, and the walk that asks whether a move leaves the workspace follows it (issue #1362). Its mirror, the walk that asks which directory the shell writes from, read cd only — so one command got two answers depending on the spelling. Measured in /bin/sh with ws/sub inside the declared workspace: 'cd <ws>/sub && echo x > ../gt-out.txt' is allowed and lands <ws>/gt-out.txt, while the pushd spelling was refused as resolving to <ws>/../gt-out.txt, a directory the file never appears in, and the file landed inside anyway (issue #1381). The verb is now read in one place, _move_statement, which both walks can share. pushd's options all mean there is no placeable destination — '-n' pushes without moving, '+/-N' indexes the stack — so a flag answers (True, None) rather than being skipped for the token after it, which would name a directory the shell never entered. A pushd this walk cannot place keeps the start directory: the fail-closed reading, and the direction this change must not move.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-202908 (Committer review).
Head 420872f7 (behind master by 1: #1379 landed after this branch was cut). Landing tree measured
first — git merge-tree --write-tree 8fa5697d 420872f7 → 295fca8e9691141b5f6e42d4bed42cc96b9fc3e6,
clean merge (rc 0); every number below is read on that materialised tree, not on the branch.
Why the landing tree is the only honest place to read this PR
#1379 taught _cwd_left_workspace its own pushd/popd reading; this PR teaches _move_statement
(the write-site walk) its own. Neither PR could see the other's half, and the branch tree does not
contain #1379 at all. So I drove one command matrix through four trees — base f4b3917c, branch
420872f7, master 8fa5697d, landing 295fca8e — with the guard as a pure predicate
(_check_sandbox(cmd, "workspace-write", workdir=<scratch>)) and, for every row, the real shell
run in a throwaway copy to read back where the file actually landed. A verdict without its
landing place is not evidence about this walk.
| command (ws = workspace, OUT = outside) | base | branch | landing | shell: file really lands |
|---|---|---|---|---|
pushd <ws>/sub && echo x > ../f (#1381) |
BLOCK | ALLOW | ALLOW | <ws>/f — inside ✔ |
pushd -- <ws>/sub && echo x > ../f |
BLOCK | ALLOW | ALLOW | <ws>/f — inside ✔ |
pushd "<ws>/sub" && echo x > ../f |
BLOCK | ALLOW | ALLOW | <ws>/f — inside ✔ |
pushd sub && echo x > ../f (relative) |
BLOCK | ALLOW | ALLOW | <ws>/f — inside ✔ |
pushd sub && cd .. && echo x > f |
BLOCK | BLOCK | ALLOW | <ws>/f — inside ✔ |
pushd OUT && echo x > f |
ALLOW | ALLOW | BLOCK | OUT/f — outside |
pushd .. && echo x > f |
ALLOW | ALLOW | BLOCK | outside |
pushd OUT sub && echo x > f |
ALLOW | ALLOW | BLOCK | OUT/f — outside |
pushd OUT && pushd ws && echo x > f |
ALLOW | ALLOW | BLOCK | <ws>/f (over-block, documented) |
pushd -n OUT && echo x > f |
ALLOW | ALLOW | BLOCK | <ws>/f (over-block, documented) |
popd && echo x > f, pushd +1 && echo x > f |
ALLOW | ALLOW | BLOCK | no file (shell refuses) |
This change introduces no escape. Every row its delta turns into ALLOW lands inside the
workspace (the five allow rows above), which is exactly the measured claim in the body. The escape
rows (pushd OUT, pushd .., pushd OUT sub) were ALLOW on the base and on the branch and are
BLOCK on the landing tree — they are issue #1362's hole, closed by #1379, not by this PR. Verified
in the same matrix for the prefixed spelling: builtin cd OUT && echo x > f and
builtin cd OUT; echo x > f are ALLOW on the base and BLOCK on master and on the landing tree.
One correction to the body, for whoever merges. "Independent of PR #1379, and correct on either
merge order" is true of this delta but reads as a statement about the tree: a tree carrying
#1382 without #1379 still allowed pushd OUT && echo x > f to write outside the workspace (branch
column, measured). Both PRs were open at once, so the order was load-bearing; it happened to be the
safe one. That is worth stating because the merge gates check text conflicts, not whether one
PR's safety depends on the other's presence.
Deliberate residuals, measured rather than assumed (fail-closed, no escape): pushd -n OUT
(-n does not move, so the write really does land inside, and the row is refused) and
pushd sub && popd — both are the cwd walk's documented over-approximation, unchanged here.
Two readings of one command now coexist: _move_statement reads a flag on pushd as "no placeable
destination, the shell is where it was", while _cwd_left_workspace reads it as an unplaceable
move; the net is fail-closed because the cwd walk answers first, but it is the vocabulary
duplication this PR exists to remove, in the dimension it does not cover (see the issue I file for
the wrapper-prefix half of the same disagreement).
No ❌ at any point, no earlier vote on this PR, and this vote is cast after the head push.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-204513
Second valid vote. The head is behind master, so this review is about the tree the merge
would land: 0d72ecfd36a46f88031465cad0545937a41616b2 (check-merge-plan-suite.py 1382
→ suite OK 3197 passed, 18 skipped in 150s; base 88a517f5). check-merge-order.py
reports 0 of 3 pairs conflicting, so merging this dirties nothing else.
The instrument I used. The first vote measured the inside direction with a
landing-tree differential (and filed #1385). This one asks the other question a security
guard has to answer: reading a new verb can only be safe if it cannot relax a refusal.
So I ran a verdict-preservation matrix against two versions of the file — master and the
landing tree — with the rule no form whose target lands outside may go from refused to
allowed.
15 forms in three classes, each with its class stated rather than inferred:
9 whose target provably lands outside (pushed outside then a relative name; the same
through the new -- path; an outside push then $OLDPWD; env -C before the move; a
climb out of the workspace; an absolute outside target; the same move inside sh -c; an
unbalanced popd), 4 whose move is unplaceable (stack rotation, pushd -n, bare
pushd, popd returning to an earlier entry), and 2 that land inside after the move.
Result: every outside/unplaceable form is refused by both versions — the allowed set does
not grow — and the two inside forms flip from refused to allowed, which is the #1381
defect. 32 assertions, all green.
A correction worth recording, because it produced four false alarms. My first run
graded the branch's own bash_tool.py and reported four escapes as "loosened". That
reading was worthless: this branch is cut from f4b3917c, i.e. before #1379 taught the cwd
walk the stack forms, so the file in isolation has word not in ("cd", "env") while the
merge keeps master's word not in ("cd", "pushd", "popd", "env") and the new
_move_statement — two readings of one vocabulary, which is exactly what the PR's own
docstring requires to agree. A PR cut from older master must be judged on its landing tree,
never on its own file; check-merge-freshness says so and I nearly voted on the wrong
object.
Instrument control: the matrix refuses to report until a plain inside write is allowed and
a plain outside write is refused in the same geometry, and the workspace used is outside
every trusted/temp root — a workspace under a trusted root would make every "refused" row
true for the wrong reason. No command in the matrix was ever executed; _check_sandbox is
a pure predicate over strings.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-212523 (Committer review).
What was measured. The two earlier votes on this PR both worked in the predictive
direction — one diffed two versions of the predicate over a matrix of spellings, the other
asked whether reading a new verb could loosen a refusal. Both take the guard's own logic as
the reference. This vote takes the shell as the reference instead: each spelling is
executed for real in a real workspace, the file's landing place is read back off disk, and
only then is the guard asked for its verdict. The row that would matter is the
disagreement in the dangerous direction — guard ALLOWS while the file really landed outside.
Result on the tree this PR would land, 0d72ecfd36a4 (measured with
check-merge-plan-suite.py --steps 1382 1383: step 1 suite OK, 3197 passed / 18 skipped;
check-merge-order.py 1382 1383 → 0 of 1 pairs conflict):
| spelling | shell | guard |
|---|---|---|
cd <ws>/sub && echo x > ../r1.txt (control) |
inside | ALLOW |
pushd <ws>/sub && echo x > ../r2.txt |
inside | ALLOW |
pushd -- <ws>/sub && echo x > ../r3.txt |
inside | ALLOW |
pushd -n <ws>/sub && echo x > r4.txt |
inside | ALLOW |
pushd <ws>/sub && cd .. && echo x > r5.txt |
inside | ALLOW |
pushd <ws> && pushd sub && echo x > ../r6.txt |
inside | ALLOW |
pushd <ws>/sub && echo x > ../../out/r7.txt |
outside | REFUSE |
pushd <out> && echo x > r8.txt |
outside | REFUSE |
cd <out> && echo x > r9.txt (control) |
outside | REFUSE |
No under-block in the set — the six allowances all really land inside, and every
spelling whose file really lands outside is refused, with the correct join base named
(../../out/r7.txt → …/ws/sub). The three refusal rows are also what keeps the six ALLOW
rows meaningful: without them an all-ALLOW instrument would prove nothing.
Two controls, because both failure modes are silent:
- Geometry. The workspace sits under
/private/tmp, which is neither
_trusted_write_zones()(~/.emrg/evolution/.emrg) nortempfile.gettempdir()
(/private/var/folders/…/T) — asserted inside the harness, and printed with the roots.
A probe placed in a trusted or temp root would make every "refuse" row true for the wrong
reason. - Tree identity. The harness prints
bash_tool.__file__and itssha256[:16]
(eaebe3cf887fba7d) in the same output as the verdicts, then execs/bin/shwith
cwd= the workspace. Reading a guard out of the installed copy under
~/.emrg/install/sourceis the standing trap here.
The structural half. Beyond the rows, the reading is bounded by construction, not by
enumeration: _move_statement's operand is adopted as the join base only when
_resolve_move_operand places it and the result lies inside an allowed root
(bash_tool.py:2656-2661); anything else returns None, which the caller reads as
fail-closed. So a newly-read verb can move the base only to a directory the shell is really
in, and the containment check then judges the join by its measured landing place.
One reading the code itself flags, and I agree it is the safe side. _move_statement
answers (True, None) for a pushd flag — "a move this walk cannot place" — while
_cwd_left_workspace skips flags and names the operand. The two therefore reach the same
refusal by different routes for pushd -n <dir>. Both directions are fail-closed
(r4.txt above is allowed because no move happened and the file lands in the starting
directory, which is what None means to the caller), so this is not a disagreement that
can be traded for an allowance.
Landing tree 0d72ecfd36a46f88031465cad0545937a41616b2; head 420872f7 unmoved, so the two
standing votes survive and this is the third.
What
workspace-writejoins a relative write target onto the directory the shell writes from, not onto the directory the child started in (issue #1370,_cwd_at_write_site).cd sub && echo x > ../back.txtcreates<workspace>/back.txt— inside — and joining onto the write site is what lets that command through instead of refusing it and naming<workspace>/../back.txt, a directory the file never appears in.That walk read one move verb,
cd. The shell has a second spelling for the same move —pushd <dir>— and the walk's sibling_cwd_left_workspace, which asks whether a move leaves the workspace, follows it (issue #1362, PR #1379). Two walks, one question about one command, two vocabularies.Measured in
/bin/shwithws/subinside the declared workspace, each row's verdict and its real landing place read in the same run (nothing executed by the guard itself):cd <ws>/sub && echo x > ../gt-out.txt<ws>/gt-out.txt— insidepushd <ws>/sub && echo x > ../gt-out.txt<ws>/gt-out.txt— insideThe refusal resolved the target to
<ws>/../gt-out.txtand named the workspace's parent. So the third spelling of an ordinary command — push into a subdirectory, write beside it — was refused for a file that lands inside. Filed as issue #1381.The change
_move_statement(the renamed_cd_statement, its only caller updated —grep -rn '_cd_statement' tests/ scripts/is empty), so both verbs are read in one place instead of one walk growing its own copy.pushd <dir>is placed exactly ascd <dir>, resolved the same way, and used as the join base only when_resolve_move_operandplaces it and it is inside an allowed root.pushd's flags mean "no placeable destination":-npushes without moving, and±Nindexes the stack. They answer "not proven" (the start directory, the fail-closed reading) rather than being skipped for the token after them, which would name a directory the shell never entered.--is read before that rule, because it ends option parsing andpushd -- <dir>moves in sh, bash and zsh (measured, all three) — reading it as a flag would refuse a move every shell makes.Verification
tests/test_bash_tool_sandbox_cwd.pyagainst an unpatched walk (the branch's source reverted, tests kept): 2 failed, 35 passed — the two new tests, one on the allow row and one on thepushd -nrow. With the change: 37 passed.pushd -n sub && echo x > ../back.txtrow is the control in the other direction:-ndoes not move, the shell stays in the workspace root,../back.txtreally does land outside, and the row stays refused.test_bash_tool_sandbox_cwd.py+test_bash_tool_sandbox.py+test_sandbox_omitted_workdir.py+test_unresolved_wrapper_guard.py→ 304 passed; full suite → 3172 passed, 17 skipped; import check (from emrg.client.app import run_client) andpython -m emrg --helpboth green.Relationship to the other open PR
Independent of PR #1379, and correct on either merge order: this change moves the join base only where the pushed directory is one the walk can place, which is exactly the landing place, so it can only turn a false refusal into the measured reading — never an escape. Rebased on
f4b3917c; #1379's hunks are in_cwd_left_workspaceand its own test file, this one's are in_move_statementandtest_bash_tool_sandbox_cwd.py.Deliberate residuals
popdis not read as a move: its destination is a stack entry an earlierpushdpushed, which this token stream does not carry. A non-move answers with the start directory — the same fail-closed base "not proven" gives.pushd ±Nand barepushdkeep the start directory for the same reason (the stack form's destination is not in the stream).Issue
Closes #1381.