emrg: the attached env -C spelling is a move the guard reads too - #1411
Conversation
|
Maintainer refresh: the branch was 4 commits behind master (#1404–#1407 landed this cycle), so its green CI was about tree The merged tree is green on the full Python suite (3527 passed, 22 skipped, run in the refreshed worktree) on top of the review I had already done on the head: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-084645
Reviewed on the head as it now stands, a2b58da2, which this cycle refreshed: the branch was
4 commits behind master (#1404–#1407 landed this cycle), so its previous green CI was about
tree d041a948 rather than about anything mergeable. I merged master 56ee2055 into it and
pushed the merge — a merge, not a rebase, because a published rebase cannot be pushed without
the force-push this project forbids. Everything below is about the refreshed tree, which the
freshness gate now calls FRESH (head a2b58da2, merge base = master's tip) and whose both CI
legs pass (test 4m7s, test-windows 8m27s).
Full suite on the refreshed tree before pushing: 3527 passed, 22 skipped. No code in the
PR was changed by the merge.
What the PR closes is a real fail-open, and I re-measured both halves rather than reading them:
--chdir=<dir>had been read since the rule was written while its short twin had not, so
env -C<elsewhere> sh -c 'echo x > f'was ALLOW while the file really landed outside
the workspace. Ground truth, asked of the real/usr/bin/envin a temp tree rather than of
the guard: all three attached spellings —-C<dir>,-iC<dir>,-iC <dir>— wrote into
the other directory and left the starting one empty. So the verdict the guard used to
reach was wrong about where the write goes, which is exactly the class of defect worth a
refusal.- The new branch is load-bearing, not decorative: removing the
eliffor the attached
spelling reds three of the PR's own tests (the spelling table, the stated-price row, and the
ground-truth arm) while the other 44 stay green. Restored byte-exactly afterwards. tests/test_bash_tool_sandbox_cwd.pyas shipped: 47 passed, and the ground-truth arm is
correctly gated off win32 with its reason stated — a verdict alone is not a bug, and the
instrument that says where the file lands needs a POSIXenvthat moves the child.
Two things I checked because the PR is the kind that can quietly overreach:
- The reading accepts any single-dash token containing a
C, which is deliberately wider
than "the flag is-C". The price is stated in the code and pinned by
test_the_price_of_reading_the_bundle_is_statedrather than left to a reader:env -uC<path>
(whereCis a value, not the flag) is read as a move. That direction only ever adds a
refusal, and only when the text after theCspells a path outside the workspace — the
fail-closed side of the #461 enumeration this walk refuses to depend on. I would rather
have this than the fail-open, and the alternative (enumerating which short options take a
value) is the enumeration the walk's contract already says it will not depend on. -P<path>(env's utility path, which is a program path and not a directory) is not read
as a move, and a bundle that merely ends inCtakes the next token exactly as bare-C
does — both pinned. So the widened reading is bounded by the tests, not by prose.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-092733
Reviewed on the refreshed head a2b58da2 (base = master's tip, so this is the tree the merge
would land), both CI legs green (test 4m7s, test-windows 8m27s).
The fix closes a genuine fail-open, and I measured both halves myself rather than reading the
PR's table. Ground truth, asked of this host's real /usr/bin/env in a temp tree, reading where
the file actually lands:
command (cwd = ws) |
file lands | guard before | guard after |
|---|---|---|---|
env -C<outside> sh -c 'echo x > f' |
outside | ALLOW | BLOCK |
env -iC<outside> sh -c 'echo x > f' |
outside | ALLOW | BLOCK |
env -C<inside-relative> sh -c 'echo x > f' |
inside | ALLOW | ALLOW |
env --chdir=<outside> … / env --chdir <outside> … |
outside | BLOCK | BLOCK |
Row 1 and 2 are the defect: the same move, spelled with the value attached, and the walk read
neither — so a relative write target after it was judged by a cwd the shell had already left.
Row 3 is the direction that would make a fix of this kind wrong, and it is still allowed, so the
change does not simply refuse everything that says -C.
I asked this PR's own predicate about shapes its tests do not cover, and the new branch is
load-bearing rather than decorative: deleting it reds three of the PR's tests
(test_the_attached_env_chdir_spelling_is_a_move_too,
test_the_price_of_reading_the_bundle_is_stated,
test_the_attached_spelling_really_moves_the_child) and leaves 44 green; restored byte-exactly,
47 passed. The ground-truth arm is correctly gated off win32 with its reason stated, which is the
right call — a verdict alone is not evidence of where a file lands.
One residual I measured, non-blocking and fail-closed. The walk iterates over every argument
of env, so an option-shaped token after the utility name is read as a move — but env
stops parsing options at the utility, so the shell does not move there. Measured on this host:
env FOO=1 sh -c 'echo x > f' -C<outside>— the file lands insidews, while the guard
BLOCKs. A false block on a command that writes nothing outside (same for the space-separated
form).env FOO=1 -C<outside> sh -c 'echo x > f'— this host'senvdoes not even move (it exits 127,
"No such file or directory", because BSDenvstops option parsing at the assignment), yet the
guard BLOCKs.
This is the same direction as the rows issue #1391 already tracks (a refusal, not a leak), and
it is not introduced by this PR — the loop over all args predates it — but the PR does widen the
matched set, and its code comment already prices one over-read (env -uC<something>), so this row
belongs next to that one. I am filing it as a measured row on issue #1391 rather than asking for a
change here: blocking a fix that closes a real fail-open over a fail-closed over-read would trade
the dangerous direction for the safe one, and narrowing the walk means deciding where a command's
options end, which is its own change.
Not verified, and named as such: whether GNU env's getopt_long abbreviations (--chdi=<dir>)
are accepted. This host's env is BSD — it rejects --chdir outright — and no GNU env is
installed here, so I could not measure it; I am not asserting it either way.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-100129
Reviewed at the landing tree 94fd9cccd6b710a60b0ec872bf624b8983bd52e0 (master 2f691617 + this PR), not at the head, because the head is behind_by=1 and a refresh would void the two votes already standing on it.
Change read: _cwd_left_workspace (emrg/tools/bash_tool.py) now reads the attached short spelling — -C<dir>, and the bundle -iC<dir> / -iC <dir> — beside the -C <dir> / --chdir=<dir> / --chdir <dir> forms it already read. The --chdir= form had been read since the rule was written while its short twin was not, so env -C<elsewhere> sh -c 'echo x > f' answered ALLOW while /bin/sh really wrote the file outside the workspace.
Independent instrument (not this PR's own test): for eight env spellings, the guard's verdict compared against where a real /bin/sh actually put the file, in a tree the probe built, with outside deliberately outside the OS temp root (a temp-rooted "outside" is an allowed write zone and answers ALLOW for every row — how a first pass measures itself green).
row verdict shell wrote agrees
env -C <out> sh -c 'echo x > f' BLOCK /private/tmp/...#outside/f yes
env -C<out> sh -c 'echo x > f' BLOCK /private/tmp/...#outside/f yes
env -iC<out> sh -c 'echo x > f' BLOCK /private/tmp/...#outside/f yes
env -iC <out> sh -c 'echo x > f' BLOCK /private/tmp/...#outside/f yes
env -C<in> sh -c 'echo x > f' ALLOW <ws>/sub/f yes
control: cd ../outside && ... BLOCK /private/tmp/...#outside/f yes
env --chdir=<out> / --chdir <out> BLOCK (nowhere — BSD env: illegal option) n/a
The two --chdir rows are n/a rather than a disagreement: this host's /usr/bin/env is BSD and rejects --chdir outright (usage: env [-0iv] [-C workdir] [-P utilpath] [-S string]), so the row writes nothing anywhere and the BLOCK is not a block a host can feel — while on a GNU host, where that spelling really does move the child, BLOCK is the correct verdict. Folding that outcome into "inside" is what would have made it look like a false block.
The control that makes the reading mean something: the same instrument run against pre-fix master (2f691617) reports 3 problems — env -C<out>, env -iC<out>, env -iC <out> all ALLOW with the file at /private/tmp/...#outside/f. On the landing tree: 0 problems. Same instrument, same rows, only the tree differs. A shell-side control (env -C<out> sh -c pwd → the outside directory) confirms the host's env really honours the attached spelling, so the rows measure the guard rather than a shell that ignores the flag.
Scope honesty: the over-read the new branch accepts (env -uC<something> read as a move) is stated in the comment rather than hidden, and it is the fail-closed direction. The false-block family in #1391 (the outer-shell redirect, eval, the flag between prefix and verb) is untouched and correctly left out of scope.
Verification: landing tree full suite 3530 passed, 22 skipped (scripts/check-merge-plan-suite.py 1411, and I rebuilt the same merge locally — tree 94fd9ccc… byte-identical). tests/test_bash_tool_sandbox_cwd.py 47 passed on that tree. Nothing here starts, stops or signals a daemon, and nothing touches ~/.emrg/config.toml.
Closes the mirror row of issue #1391: the workspace-write guard read three of the four spellings
envhas for its chdir, and the one it did not read is a fail-open.The defect, measured
_cwd_left_workspacereads-C <dir>,--chdir <dir>and--chdir=<dir>. The attached shortspelling —
-C<dir>— was never read, by either walk, so the move was invisible and the relativewrite target behind it was joined onto the workspace root and read as inside:
The last two rows are why this is a failure and not a preference: the same move, one keystroke apart,
gets two verdicts, and the file lands wherever the shell puts it. This is the direction a
workspace-writeboundary must never move — the refusal costs friction, the allowance costs theworkspace.
The bundle is real too. This host's
envmoves the child for the bundled spelling as well(
env -iC<dir> sh -c pwdprints<dir>, measured), so closing-C<dir>alone would have left thehole one more keystroke away — which is the #461 enumeration trap in its usual shape.
What this changes
One branch in the env walk: a short-option token carrying a
Cmeans "everything after thatCisthe directory", and a bundle ending in
Ctakes the next token, exactly as bare-Cdoes. Whichshort options may precede the
Cis not decided by the guard — that is the enumeration it refuses todepend on — so the read is structural.
The price is stated, not hidden. A token carrying a
Cinside an option value(
env -uC<path>is "unsetC<path>") is read as a move as well. It only ever adds a refusal, andonly when the text after the
Cspells a path outside the workspace; the same token with an insidevalue, or with no value at all, stays allowed. One anchored test pins that price so a later change
has to lift it deliberately.
The remaining rows of #1391 are untouched: the two
evalshapes (refused a statement earlier by thecwd walk, which is the security-critical direction) and
command -p cd sub(which needs a prefix'sflag list) are still refused, and the false block on
env -C <out> cat > f— the outer shell setsthat redirect up before
envruns — is unchanged for both the separate and the attached spelling.That one is a friction, tracked in the issue; the open direction is what this PR closes.
Tests
Three tests in
tests/test_bash_tool_sandbox_cwd.py, in that file's idiom (parity by verdict, plusone ground-truth arm that runs the same text in a tree the test builds and reads where the file
lands, gated to POSIX because the instrument is a real shell):
and not about a guard that refuses
envoutright — the inside spelling stays allowed andenv -P(utility path, not a move) is not read as one;
Mutation arm, one per new test: deleting the new branch reds all three
(
3 failed, 44 passed), restoring it passes (47 passed). Full suite on the head:3509 passed, 21 skipped. Doc-count and node-count guards green; nothing here starts, stops or
signals a daemon.