emrg: a destination an option names is read as a write target - #1400
Conversation
…ions-are-write-targets
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-040013
Reviewed at head abeb4f6d, and re-measured the fix myself rather than taking the PR
body's word for it.
The change reads what it claims to read. _OPTION_DESTINATION_VERBS plus
_option_destination_values derive the destination from an option's value, which is
the one place this walk had no rule for: the target list came back empty, and an empty
list never enters the loop that judges targets, so both tiers allowed by construction.
Reading the value in every spelling getopt accepts — spaced (-o FILE), attached
(-oFILE) and both long forms (--output FILE, --output=FILE) — and dropping a value
of exactly - (the documented way these options mean stdout) is the right shape: it
widens refusals without narrowing any allowance, which is the direction a sandbox guard
has to err in.
Independently run at this head (fresh worktree of abeb4f6d, not the working tree):
tests/test_bash_tool_option_destinations.py → 71 passed, 1 skipped (the skip is
wget, absent on macOS).
The mutation arm is real, not decorative. Dropping the "curl" row from the
destination table → 14 failed, 57 passed, 1 skipped, with the failures spread across
the attached-token reader, the long = spelling and the end-to-end refused-writes-nothing
row (the one that carries its own positive control). Restoring the table → back to 71
passed. So the rows are load-bearing rather than retellings.
CI on this exact head: run 35389714700, test 3m0s and test-windows 8m38s, both
pass. Merge state MERGEABLE.
The two deliberate non-coverages are the reason I am comfortable rather than merely
satisfied: tar is absent because -f/-C mean different things under -c and -x
(naming -C would falsely refuse tar -cf out.tgz -C /etc .), and the curl -so<dir>
cluster is left unnamed with its ground truth recorded because splitting it needs a
grammar this walk does not have, and the two ways of guessing are not equally bad —
naming a read would be a false block. Both are pinned as measured holes rather than
left to be rediscovered, and enforcement="partial" stays asserted so a later change
cannot quietly upgrade the claim.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-042531
Re-reviewed at head abeb4f6d (unchanged since the previous review, and both CI legs green there).
What this closes. _extract_write_targets derived targets from operands, redirects and a verb
list, so a destination named as an option's value — curl -o <f>, wget -O <f>,
sort -o <f>, unzip -d <d> — produced an empty target list, and an empty list never enters the
loop that judges targets, so both tiers allowed it by construction. Under read-only (the tier the
dirty-tree guard downgrades a task into, issue #979) that meant a task could write wherever the
process can reach.
Independently run at this head (fresh worktree of abeb4f6d):
tests/test_bash_tool_option_destinations.py → 71 passed, 1 skipped (the skip is wget, absent
on macOS).
A fresh mutation arm, deliberately a different piece than the one I ran previously. Disabling
the attached-token reader (_leading_short_option_value returning None) → 11 failed,
60 passed, spanning the attached-spelling test, the long = spelling test and the
cluster-spelling residual; restored → 71 passed. So the attached spelling is genuinely load-bearing
rather than incidentally covered.
The direction of the change is what I most wanted to confirm, and it holds: it widens refusals
without narrowing allowances — writes aimed inside the workspace, stdout-only shapes and reads all
keep their verdicts — which is the direction a sandbox guard must err in. The two deliberate
non-coverages are recorded with their ground truth rather than left implicit (tar, because
-f/-C mean different things under -c and -x; the curl -so<dir> cluster, because the two
ways of guessing are not equally bad and naming a read would be a false block), and
enforcement="partial" stays asserted so no later change can quietly upgrade the claim.
|
Independent verification of Staged the head tree from git objects (no Two arms, plus a control
Twenty-two spellings asked through The census is complete for
|
| cluster | targets | read-only |
workspace-write |
pinned as a residual? |
|---|---|---|---|---|
curl -so OUT/f https://x |
[] |
ALLOW | ALLOW | yes |
wget -qO OUT/f https://x |
[] |
ALLOW | ALLOW | no |
unzip -qd OUT a.zip |
[] |
ALLOW | ALLOW | no |
sort -k1o OUT/f x |
[] |
ALLOW | ALLOW | no — and correctly so |
Each control (the same destination spread over two tokens) names the target and blocks at both tiers, so the cluster really is the difference. sort -k1o is the one that belongs outside the table, for the reason the docstring already gives: -k takes an argument, so the remainder is not the destination and OUT/f is a file to read — the walk allowing it is right.
wget -qO <dir>/f and unzip -qd <dir> <zip> are the other shape: the leading letter takes no argument, so the option letter is the destination letter and the next token is the value. unzip -qd I measured on this machine (no Docker needed — BSD unzip parses the cluster the same way):
unzip -qd <dir> <zip> rc=0 landed=['outside/a.txt']
unzip -q -d <dir> <zip> rc=0 landed=['outside/a.txt'] (the control)
so it really writes into <dir> while the walk returns an empty target list. wget is absent here (the PR's own suite skips it), so for that row I can only report what I did measure — the predicate's empty list — and the ground truth is inherited from the Docker measurement of the two-token wget -O, whose cluster parsing -qO shares. Declared rather than glossed, since the conclusion differs for the two rows.
Why it is worth adding them is the table's own tripwire. Its docstring says a future change that reads one of these families makes the row red and "must move it into OPTION_DESTINATIONS deliberately" — an assertion per row is what makes that fire. With wget -qO and unzip -qd unpinned, a later change that starts reading them would red nothing here, and the hole would close silently while the table still read as the complete census of what this walk cannot see. Two rows, no behaviour change, and the count of unnamed writers becomes the real one (six rather than four).
Nothing here suggests reading the clusters: naming a read is a false block, and both guesses are not equally bad — the reasoning in the docstring is the right call, and this PR closes a fail-open in the direction that matters.
test and test-windows are green on this head (run 35389714700).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-044243
Re-reviewed at head abeb4f6d, independently re-run, and the PR is now the third
standing approval.
The defect and the direction of the fix. _extract_write_targets derived targets
from operands, redirects and a verb list, so a destination named as an option's value
(curl -o <f>, wget -O <f>, sort -o <f>, unzip -d <d>) yielded an empty target
list — and an empty list is allowed at both tiers by construction, because the loop that
judges targets never runs. Under read-only, the tier the dirty-tree guard downgrades a
task into (issue #979), that meant a task could write anywhere the process can reach.
The change widens refusals without narrowing allowances — writes inside the workspace,
stdout-only shapes and reads keep their verdicts — which is the direction a sandbox
guard has to err in.
Independently run at this head (fresh worktree of abeb4f6d):
tests/test_bash_tool_option_destinations.py → 71 passed, 1 skipped (the skip is
wget, absent on macOS).
Fresh mutation arm, a third distinct piece. The PR drops an option value of exactly
- because that is the documented spelling for stdout, and a bare - left in a target
list would make every block message name a token that is not a path. Removing that filter
(return out) → 1 failed: test_a_stdout_destination_names_nothing_at_all, restored
to 71 passed. Together with the two earlier arms (dropping the "curl" row; disabling the
attached-token reader) that is three separate load-bearing claims, each reddening its own
named test rather than a whole file.
CI on this exact head: run 35389714700, test 3m0s and test-windows 8m38s, both
pass. Merge state MERGEABLE/CLEAN.
The class
_extract_write_targetsderives a command's targets from operands, redirects and averb list.
curl -o <f>,wget -O <f>,sort -o <f>andunzip -d <d>name theirdestination as an option's value, so no operand rule reaches it and the walk named
no target at all — and an empty target list is allowed at both checked tiers by
construction, because the loop that judges targets never runs.
Measured while verifying #1399's landing tree (master + the everyday-writer fix), in one
geometry whose target lay outside every allowed root (
workspace, the OS temp root,~/.emrg/evolution/.emrg):read-onlyworkspace-writecurl -o <outside>/f <url>wget -O <outside>/f <url>sort -o <outside>/f xunzip <zip> -d <outside>truncate -s 0 <outside>/ftee <outside>/fcp x <outside>/fread-onlyis the tier the dirty-tree guard downgrades a task into (community issue#979), and its documented promise is stronger than its tag, so a task that could
curl -oanywhere the process can reach was not read-only in any useful sense.Ground truth — measured on GNU, not inferred
macOS/BSD does not carry these flags, so the semantic question was settled in Docker
(
debian:bookworm-slim, colima). The sources are read back off disk in a directoryoutside every allowed root:
curl -o <d>/f,curl -o<d>/f,curl --output <d>/f,wget -O <d>/f,sort -o <d>/f,tar -xf -C <d>(-C <d>and-C<d>),tar -cf <d>/a.tgz,unzip -d <d>,unzip -d<d>.curl --output=<d>/fexits 2 andunzip --directory=<d>exits 11. Both spellings are still read — reading themrefuses a command that was going to fail anyway, while not reading them would miss a
real write on
sort/wget, which accept theirs. The asymmetry is recorded in thereader's docstring rather than left to be re-derived.
-means stdout(
wget -O - <url>,sort -o - xboth leave the directory empty), so the readerdrops it. Pinned directly, because a bare
-in a target list would make everyblock message name a token that is not a path.
What the change is
One table (
_OPTION_DESTINATION_VERBS) plus one reader, and one branch in the walkplaced after
find— deliberately away from #1399's region. Every spelling getoptaccepts is read: spaced (
-o FILE), attached (-oFILE) and both long forms(
--output FILE,--output=FILE).What it deliberately does not cover, and why
taris the case that proves why there is no per-verb flag grammar here:-f'svalue is a write under
-c/-r/-uand a read under-x/-t, and-Ciswhere files land when extracting but only a directory to collect from when
creating — so
tar -cf out.tgz -C /etc .writes nothing outside and a rule naming-Cwould falsely refuse it. Only verbs whose destination option means one thingregardless of the other flags are listed (the emrg: 修复 p12 私钥校验单复数匹配 — identities imported #461 class is about exactly this).
curl -so<d>/fis left unnamed and pinned as a measuredresidual with its ground truth (measured: it really does write into
<d>). Splittingit needs a grammar this walk does not have, and the two guesses are not equally bad:
the remainder is the value for
curl -so<d>but the next token is the value forsort -ko out.txt(wherektookoas its value andout.txtis an operand toread) — and naming a read is a false block. A future change that closes it flips
that assertion deliberately.
enforcement="partial"stays: an interpreter still writes wherever it likes. Thetests assert the label so a later change cannot quietly upgrade the claim.
One existing expectation moved, and why
tests/test_bash_tool_sandbox.py::test_containment_allows_legitimate_commandscarriedcurl -s -o /tmp/out.json <url>in a list that asserts both tiers allow. It waspassing for a reason unrelated to containment:
-onamed no target. With thedestination read,
read-onlyrefuses it (every write except/dev/null) andworkspace-writejudges the path —/tmpis the temp root only on Linux, and thissuite also runs on macOS and Windows. The row is removed with a comment saying why it
cannot come back, and the coverage moves to the new file, where the tier semantics are
asserted with the reader that produces them.
Tests
New file
tests/test_bash_tool_option_destinations.py— 72 tests, self-containedfixtures:
writes;
allowed (a guard that refuses the workspace it guards is worse than the hole);
read-onlyrefuses an inside-workspace option destination too — its contract, not aslip;
cannot be flipped is not a claim: dropping the verb from the table, disabling the
attached-token reader, pruning the long name from the table — and each arm is checked
not to be a blanket off-switch (the spaced rows survive it);
BashTool.executein a directory the testcreates: the refused form writes nothing outside and its control really writes, so
the refusal is the difference rather than an empty directory;
tar,rsync,split,csplit,git clone, the-socluster) are pinned as a measured hole: each reaches
workspace-writewith anempty target list, and the row carries the verdict it really gets —
git cloneisrefused under
read-onlyby the git-mutator rule, which is not this walk, so aseparate test names where that block comes from. When one of those families is read,
the row reds and has to be moved deliberately. (A docstring pointing at these rows
said "all ALLOW at both tiers" before this commit; measuring it is what found the
git cloneexception.)tmp_pathsits inside the OS temp root, whichworkspace-writelegitimately allows, sotempfile.gettempdiris patched to a name no directory here has — without it therefusal would be about the temp root and would pass for the wrong reason.
Verification
uv run pytest tests/ -v— 3354 passed, 18 skipped (1 skipped iswget, absent onmacOS; its rows are still pinned by the pure-predicate tests)
scripts/check-merge-pairs.py 1399 1400— both ordered pairs clean and healthy(2 of 2, 0 blocked by a conflict), measured at this head; both PRs edit the same walk
chain, so the composition is measured rather than assumed
uv run python -c "from emrg.client.app import run_client"— OKuv run python -m emrg --help— OKRefs #1398 (the verb-list class this is a sibling of: #1399 covers the everyday verbs,
this covers the option-valued destinations).