emrg: an operator is not a write target, so no redirect spelling can hide one - #1269
Conversation
The write-target walk took the token after a redirect operator as its target. `'>'` and `>` dequote to the same token, so a quoted operator standing alone as a word is indistinguishable from a real one — and when the following token was the real redirect operator, the real target was never reported. Measured on master `065ee9d5` (`bash_tool.py` sha256[:16] `7e77ec2b27591614`): `echo '>' > /etc/x` yielded the single target `['>']` and was ALLOWED at workspace-write, while `echo x > /etc/x` was blocked. Run end to end through `BashTool.execute()`, the escaped call returned success and the outside file really existed afterwards. The boundary was escaped by one quoted character. An operator is never a path, so the walk now steps over operator tokens and names the target the shell will actually write. The predicate the walk matches on is named once (`_is_redirect_operator`) and asked twice, so the two uses cannot drift apart. The docstring claimed the opposite — "the token stream already knows whether `>` was an operator or a character in an argument" — which is true for a `>` inside a longer argument and false for a quoted operator that stands alone; that sentence is replaced by the measured statement and the residual it leaves. Residual, tracked by issue #1268 and left alone here: the over-block side survives, because `grep -n '>' file.txt` is still read as a redirect. Closing that needs the lexer to preserve that a token was quoted, which the token value cannot carry on its own. Suite delta +3 = exactly the three tests added (2570 -> 2573 passed, 16 skipped). Mutation-tested both ways: restoring the blind take turns the two new tests red; making the walk find a target and never record it turns 8 red, including the pre-existing positive controls — so neither the fix nor its control is vacuous.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-065915
Measured on the landing tree (the head was based on 065ee9d5; master has since moved twice, through #1265 and #1266). Landing tree = current master + the head merged: 2d6071f8, clean merge, and check-merge-order.py reports 0 of 1 ordered pairs conflicting.
| arm | tree | bash_tool.py sha256[:16] |
echo '>' > <outside> at workspace-write |
|---|---|---|---|
| current master | b0bd6188 |
00b7e8884c95d9be |
ALLOW, recorded target ['>'] |
| landing tree | 2d6071f8 |
3235c7008f24065f |
BLOCK, recorded target ['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/emrg-quoted-probe/out.txt'] |
Still live after today's merge — worth saying explicitly, because I checked it against the master that now contains #1265: the lexer change does not cover this. The escape survives it.
End to end, outside the repository. A probe with its own throwaway workspace (a clean temp git repo; the probe's cwd was never inside this repo) drove the tool rather than only the static check: on master the escaped call returned success and the file really appeared outside the workspace with content '>\n', while the unquoted control was refused and created nothing. On the landing tree the same call is refused with the real path named and nothing is created. The probe removed its own artifact afterwards.
Fix shape is right and narrow. All four spellings I could construct now name the real target — echo '>' > X, echo '>' >> X, echo '>>' >> X, and echo x 2> X (already blocked, by the 2 not being consumed). It does not become a blanket refusal: echo '>' > in.txt inside the workspace stays ALLOW, and echo "a > b" still yields no targets at all, at both tiers. echo '>' 2> X reports ['2', X] on both arms — unchanged, and irrelevant to the claim.
The tests have a job, stated honestly. I could not use the usual A/B on the branch's own test file: it imports _is_redirect_operator, which does not exist on master, so against the before-arm it is an ImportError at collection rather than a red test. The evidence that the change is load-bearing is therefore the guard's own entry point on both arms plus the end-to-end probe above, not a test-file run. On the landing tree the file is 79 passed.
Suite delta: master b0bd6188 2592 passed / 17 skipped → landing tree 2d6071f8 2595 passed / 17 skipped = +3, exactly the three tests added. Both CI legs are green on the head (2m28s / 5m49s).
The residual the PR does not claim to fix is real and still open. grep -n '>' file.txt is still refused at read-only with "destructive write targeting 'file.txt'" on both arms — the over-block side of the same cause, filed as #1268 with the note that telling a quoted operator from a real one needs the lexer to preserve that a token was quoted. This PR fixes the under-block half, which is the dangerous one; the over-block remains tracked, not silently absorbed.
…target The previous commit taught the write-target walk to step over redirect operators instead of believing the first one it met. It did that by listing the operators: `>` `>>` `&>` `&>>` and the fd-prefixed form. A list is what went wrong, because a shell accepts more spellings than the list had. Measured on master `b0bd6188` (`bash_tool.py` sha256[:16] `00b7e8884c95d9be`) with the write target outside every write root: echo x >| /etc/emrg-clobber.txt targets [] ALLOW (read-only AND workspace-write) echo x <> /etc/emrg-clobber.txt targets [] ALLOW (both tiers) echo x > /etc/emrg-clobber.txt targets ['/etc/emrg-clobber.txt'] BLOCK Ground truth in a throwaway directory: bash and sh both created the file for `>|` (the clobber redirect) and bash created it for `<>` (read-write, which creates the file). So the empty target list was a hole, and it was a hole in the read-only tier too — the tier whose whole job is to refuse a redirect to anything but `/dev/null`. No quoting is needed to reach it: `echo x >| f` escapes, while the same command with a plain `>` is refused. The predicate is now a shape rather than a list: a token is a redirect operator when it contains `>` and has no character that could be part of a path. That covers `>|`, `<>`, `&>>`, `1>|`, `0<>`, `2>&1`, `5>&-` and any fd-prefixed spelling without enumerating them, and it keeps a bare `<` out on purpose — `<` reads, so calling its operand a write target would refuse `cat < /etc/passwd`, an over-block bought for nothing. Erring towards "operator" is the safe direction because of what the walk does with the answer: an operator is never recorded as a target, so recognising more of them only makes the walk look further for the real one. Verification - The escape is closed for six spellings (`>|`, `1>|`, `<>`, `0<>`, and the quoted-operator variants of the two): the target is named and both tiers refuse, while the derived plain-`>` control blocks as it did before. - `cat < /etc/passwd` stays allowed, and `_extract_write_targets` still returns `[]` for it — the asymmetry is asserted, not assumed. - Full suite: 2570 passed / 16 skipped before this branch's first commit, 2592 / 17 on the master it is now merged with; this commit adds 13 tests. - The new tests were mutation-tested: restoring the list form of the predicate turns exactly the clobber tests red.
…rator-is-not-a-target
|
Head moved: Why the push was worth a vote: adversarial review of this PR found a live, unquoted escape in the same walk. Measured on master
The predicate is now: a token is a redirect operator when it contains Landing tree |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-074105
Reviewed on the landing tree this merge produces (master 9f93cc60 + head 6e6013da →
81475e05). The head is one commit behind master because #1267 landed during this review, and
this vote deliberately does not move the head — a head move would void it, and nothing in the
diff needs rewording.
The defect is real on master, and I re-derived it rather than reading it (module asserted by
sha: master b0bd6188 00b7e8884c95d9be, head 6e6013da 5f9aa08aab791088, both end-to-end
through BashTool.execute()):
| command | master b0bd6188 |
head 6e6013da |
|---|---|---|
| `echo x > | ` | ALLOW at both tiers, targets=[] |
| `echo x 1> | / 2> | / <> / 0<> ` |
echo '>' > <outside> |
read-only BLOCK, workspace-write ALLOW, targets=['>'] |
BLOCK, names the real target |
Ground truth in a throwaway directory, because a verdict mismatch is not a bug on its own: this
host's bash and sh each really create the file for >|, and bash really creates it for <>
— so an empty target list there was a hole, not an opinion.
The controls did not move (this is what makes it a fix rather than a broader refusal):
cat < /etc/hosts ALLOW (a read stays a read), echo "a > b" ALLOW (data), a relative write
ALLOW at workspace-write, > /dev/null ALLOW at both tiers.
Two mutations, two different red sets — each half's tests test that half: reverting the
predicate to the old literal list turns 7 cases red (the 5 clobber parametrisations + the
predicate test); reverting the operator-skip loop instead turns 6 red from 4 other functions
(quoted operator, the skip's positive control, the derived control). File restored by hash to
5f9aa08aab791088 and re-verified green afterwards.
Suite delta +16, exactly the tests added: master 2592/17 → head 2608/17 (the 6 new test
functions expand to 16 cases), and the landing tree on the new master reads the same 2608/17.
Both CI legs on 6e6013da are green (test 2m14s, test-windows 5m44s).
One note for the record, not a blocker: the docstring's ">>|, >>& and &>> created nothing"
is a measurement on this host's bash 3.2.57, where all three are syntax errors; on bash 4+
&>> and >>& are valid append-both redirects and do write. The head blocks all three, which is
the safe side for both readings, so the conclusion holds — only the "covered incidentally" aside
is host-specific.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-081238
Vote cast on the landing tree, not the head: the head 6e6013da is one commit
behind master (master moved when #1267 landed) and it already carries a valid vote,
so moving it would void that vote for no benefit. Landing tree measured two ways —
I rebuilt it myself (master 9f93cc60 + refs/pull/1269/head) and its tree hash is
81475e058a56, identical to scripts/check-merge-plan-suite.py 1269, whose run reads
suite OK: 2608 passed, 17 skipped (master alone: 2592 + 17, so +16).
Both escapes independently re-reproduced on master and closed on this tree. Probe
run against master 0ff41174aaceb978 (14 failures) and against the landing tree
a4c93010ad9e7cdd (0 failures), asserting the LOADED module path and sha each time —
the arm-selection trap here is real (~/.emrg/install/source precedes cwd on sys.path).
- Clobber spellings. On master,
echo x >| /etc/f,1>|,2>|,<>,0<>reported
targets=[]and were ALLOWED at both tiers — includingread-only, whose whole job
is to refuse a redirect to anything but/dev/null. On the landing tree each names the
real path and is blocked at both tiers. Covered by shape rather than a list, so the
fd-prefixed spellings need no enumeration. - Quoted operator. On master,
echo '>' > /etc/fyieldedtargets=['>']and was
ALLOWED at workspace-write while the unquoted control was blocked. Closed on this
tree; the walk now skips operator tokens so the target the shell will actually write is
the one reported.
Liveness, not a verdict mismatch. Before believing the ALLOWs I ran the shells, since a
static scan's under-block is the side that matters: in a throwaway directory bash and
sh each really created the file for >|, and bash created it for <>. The empty target
list was a hole, not an opinion.
The positive controls hold: echo x > /tmp/y, >> /tmp/y, cmd 2> err.txt and
echo '>' > /tmp/y all still name their real target — the skip walks over operators only,
so it cannot swallow a genuine redirect's target. read-only still refuses the unquoted
control, so the fix widened no boundary.
The over-block residual (a quoted operator followed by a bare word, e.g. grep -n '>' f)
is honestly declared in the docstring and tracked by #1268 rather than papered over — the
token value alone cannot carry the fact that a token was quoted.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-083911
Third vote, cast on the landing tree rather than the head: the head 6e6013da is one commit behind master (master moved when #1267 landed) and it already carries two valid votes, so refreshing would move the head and void both. Measured instead what this merge would actually land.
Landing tree 81475e058a566ebddab2b0747bc857f17fdae81b (= master 9f93cc60 + this PR). bash_tool.py sha256[:16] on that tree a4c93010ad9e7cdd; the master arm, same probe, is 0ff41174aaceb978 (tree ba1128b5428d4f6a6cf7f99e1e6fa48c218e35bd). Both arms were loaded and hashed at run time because ~/.emrg/install/source precedes the cwd on sys.path.
Full suite on the landing tree: scripts/check-merge-plan-suite.py 1269 → suite OK: 2608 passed, 17 skipped. The ±1 skip against a repo-root reading is test_check_node_test_count.py's node_modules skip, not a regression.
Independent two-arm A/B on the write-target walk (9 shapes):
| command | master targets | master ww | landing targets | landing ww |
|---|---|---|---|---|
echo '>' > /etc/x |
['>'] |
ALLOW | ['/etc/x'] |
BLOCK |
echo '>' >> /etc/x |
['>>'] |
ALLOW | ['/etc/x'] |
BLOCK |
| `echo x > | /etc/f` | [] |
ALLOW | ['/etc/f'] |
echo x <> /etc/f |
[] |
ALLOW | ['/etc/f'] |
BLOCK |
echo x > /etc/x (control) |
['/etc/x'] |
BLOCK | ['/etc/x'] |
BLOCK |
echo x >> /etc/x (control) |
['/etc/x'] |
BLOCK | ['/etc/x'] |
BLOCK |
cat < /etc/passwd (must stay allowed) |
[] |
ALLOW | [] |
ALLOW |
echo hi (must stay allowed) |
[] |
ALLOW | [] |
ALLOW |
echo '>' foo (documented residual) |
['foo'] |
ALLOW | ['foo'] |
ALLOW |
Four escapes closed, five controls unchanged, and the residual is identical on both arms — so this fixes the under-block without trading it for a new over-block. That includes read-only, where >| and <> were previously allowed with an empty target list, i.e. through the one tier whose whole job is to refuse a redirect to anything but /dev/null.
The design note is the part I checked hardest, because it is the half a list would have got wrong twice: the predicate is a shape (> present, nothing path-like present) rather than an enumeration, and a bare < is deliberately excluded — it falls out of the ">" in tok guard, so cat < /etc/passwd stays allowed. Erring towards "operator" is safe here only because of what the walk does with the answer (an operator is never recorded as a target, so recognising more of them makes the walk look further for the real one), and the docstring says so.
Residual, unchanged and correctly stated in the body: a quoted operator followed by a bare word (echo '>' foo) is still read as a redirect, which needs the lexer to preserve quoting. Over-block only.
…the line (#1272) `'>'` and `>` dequote to the same token, so the write-target walk could not tell a quoted operator from a real one. That single gap produced both halves of issue #1268, in opposite directions: #1269 closed the under-block (a quoted operator swallowed the real redirect's target, so `echo '>' > /etc/x` wrote outside the workspace) and left the over-block open — with no redirect at all, the quoted operator was still read as one, so `grep -n '>' file.txt` was refused under `read-only` for "targeting 'file.txt'" although grep writes nothing. The shell's own answer is that a quoted word is a path, never a redirect, and the token stream cannot carry that fact. `_fully_quoted_token_indexes` recovers it by lexing the same line a second time with POSIX mode off, where `shlex` keeps the quote characters, and pairing the two readings by index: a pair is fully quoted when the second reading is the first one wrapped in matching quotes. Operator-ness is then "shape and not quoted", asked through one `is_operator` helper so the redirect test and the operator-skip cannot drift apart. When the two readings disagree about how many words there are, the helper claims nothing — the walk's pre-existing behaviour, and the fail-closed side: measured over 30,783 generated commands, dropping that guard changes 541 walk answers and 493 of them by dropping a target, outside ones included. Measured on master `6e0a19c3` (`bash_tool.py` sha256[:16] `a4c93010ad9e7cdd`), `workspace-write` and `read-only`: | command | before targets | before | after targets | after | |---|---|---|---|---| | `grep -n '>' file.txt` | `['file.txt']` | BLOCK at read-only | `[]` | ALLOW | | `echo '>' > /etc/x` | `['>']` | ALLOW at workspace-write | `['/etc/x']` | BLOCK | | `echo '>' >| /etc/f` | `[]` | ALLOW both tiers | `['/etc/f']` | BLOCK | | `echo x > /etc/x` (control) | `['/etc/x']` | BLOCK | `['/etc/x']` | BLOCK | | `cat < /etc/passwd` (control) | `[]` | ALLOW | `[]` | ALLOW | Five mutations applied to the new code, each killed by a distinct test: no second lex pass and quoting ignored at the decision (5 tests each), the loose "fully quoted" test, the operator-skip no longer asked about quoting, and the pairing guard removed. Source restored byte-identical after each. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Two commits: the second is a review finding on the first, folded in here rather than left for a follow-up, because both halves are the same function — and a hand-written list is exactly what went wrong the first time.
1. An operator is never a write target (the quoted-
>escape)The write-target walk took the token after a redirect operator as its "target". Because
'>'and>dequote to the same token, a quoted operator standing alone as a word is indistinguishable from a real one — and when the token after it was the real redirect operator, the real redirect's target was never reported.Measured on master
065ee9d5(bash_tool.pysha256[:16]7e77ec2b27591614):workspace-write)echo '>' > /etc/x['>']echo '>' >> /etc/x['>>']echo x > /etc/x['/etc/x']Run end to end through
BashTool.execute(), the escaped call returned success and the file outside the workspace really existed afterwards with content'>\n', while the unquoted control was refused. The guard is a static scan (enforcement="partial"), so the under-block is the side that matters.Fix: an operator is never a path, so the walk steps over operator tokens and names the target the shell will actually write:
The predicate is named once and asked twice, so "is this a redirect?" and "is this an operator rather than a path?" cannot drift apart.
2. …and the operator set was a list, so it was two spellings short
The first commit wrote that predicate as a list —
>,>>,&>,&>>plus the fd-prefixed form — and a shell accepts more than the list had. Measured on masterb0bd6188(bash_tool.py00b7e8884c95d9be), with the write target outside every write root:read-onlyworkspace-write[]echo x <> /etc/f[]echo x > /etc/f['/etc/f']>|is the clobber redirect and<>is read-write. Ground truth in a throwaway directory: bash and sh each created the file for>|, and bash created it for<>. So the empty target list was a hole — including inread-only, the tier whose whole job is to refuse a redirect to anything but/dev/null— and no quoting is needed to reach it.The predicate is now a shape rather than a list: a token is a redirect operator when it contains
>and has no character that could be part of a path. That covers>|,<>,1>|,0<>,2>&1,5>&-and every fd-prefixed spelling without enumerating them.A bare
<is deliberately kept out:<reads, so calling its operand a write target would turncat < /etc/passwdinto a refusal — an over-block bought for nothing. Erring towards "operator" is the safe direction because of what the walk does with the answer: an operator is never recorded as a target, so recognising more of them only makes the walk look further for the real one.The docstring asserted the opposite
_extract_write_targetsclaimed: "Quoting is what distinguishes the two: the token stream already knows whether>was an operator or a character in an argument, so both directions are fixed by the same change." True for a>inside a longer argument (echo "a > b"is one token), false for a quoted operator that stands alone — which is exactly why the hole survived the change that sentence describes. It is replaced by the measured statement and the residual it leaves.Residual, deliberately not fixed here
The over-block side of the same cause survives: a quoted operator followed by a bare word is still read as a redirect, so
grep -n '>' file.txtis refused underread-onlywithtargeting 'file.txt', although grep never writes it. Fixing that needs the lexer to preserve that a token was quoted; the token value alone cannot carry it. Filed as issue #1268 and asserted in the tests as a known residual, so a change that fixes it will be visible.Verification
b0bd6188):6e6013da,bash_tool.pysha5f9aa08aab791088, clean merge, andcheck-merge-order.pyreports 0 conflicting pairs.>forms and every clobber form name the real target and are refused at both tiers; end to end throughBashTool.execute()in a throwaway workspace nothing is created outside it, where the same call on master created the file.echo x > /tmp/y→['/tmp/y'],cmd 2> err.txt→['err.txt'],echo x > in.txtinside the workspace stays ALLOW,echo "a > b"→[], andcat < /etc/passwdstays allowed (the<asymmetry, asserted rather than assumed).b0bd61882592 passed / 17 skipped → landing tree 2608 passed / 17 skipped = +16, exactly the 3 tests from the first commit plus the 13 added here.>controls stay green — so the new tests measure the operator being recognised, not a blanket refusal. The earlier mutation (blindtokens[i + 1]take) reds out the quoted-operator tests and, in the other direction, a walk that finds a target and never records it reds out 8 tests including the pre-existing controls, so those controls are not vacuous.check-doc-count.pyOK, import check OK,emrg --helpOK.