emrg: a brace list is not a path, on either side of the guard - #1397
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-010456
Reviewed #1397 against master 6667fba7 (its base, so CI run 35374532701 judges the tree
that would land: test 3m27s, test-windows 8m41s, both green).
The premise, re-measured this cycle rather than inherited. Two worktrees side by side —
master 6667fba7 and the head 567b2a2c — with the guard's predicate called on synthetic
paths whose OUTSIDE directory is outside every allowed write root (the fixture asserts that
itself, so the rows cannot pass for the wrong reason). One geometry, nothing executed:
| command | master | head |
|---|---|---|
cd {../emrg-1396-outside,sub} && cat > f |
ALLOW | BLOCK |
rm -rf {../emrg-1396-outside/doomed,inside} |
ALLOW | BLOCK |
tee {../emrg-1396-outside/written,inside} |
ALLOW | BLOCK |
cat > {../emrg-1396-outside/t,inside} |
ALLOW | BLOCK |
cd {../emrg-1396-outside,{a,b}} && cat > f |
ALLOW | BLOCK |
cp x {../emrg-1396-outside/c,inside} |
ALLOW | BLOCK |
mv sub {../emrg-1396-outside/m,sub} |
ALLOW | BLOCK |
sed -i s/a/b/ {../emrg-1396-outside/f,f} |
ALLOW | BLOCK |
The other direction, which is the one that matters for a guard. Every spelling master
already refused still is refused: cat > ../emrg-1396-outside/f (BLOCK/BLOCK),
rm -rf ../emrg-1396-outside (BLOCK/BLOCK), mv {sub,sub2} ../emrg-1396-outside/
(BLOCK/BLOCK). And every legitimate form the join already placed keeps its verdict:
rm -rf sub, cat > f, a{b}c > f (one literal word to the shell too — the class is a
comma or a .. inside a pair) and echo {1..3} are ALLOW on both sides. This is a strict
widening of refusals, not a narrowing.
The price is stated, not discovered. rm -rf {sub,sub2} and cp a {b,c} are now refused
even though every spelling stays inside, because which spelling the shell takes is exactly what
the text does not say; the docstring carries it, the test file pins it, and the work-around
every refusal in this file names (spell them out) applies. A redirect carrying a brace target
cannot escape in practice (bash answers "ambiguous redirect"), and it is refused with the rest
rather than special-cased — the right call for a walk that has no wish to depend on which verb
it was handed.
Not a claim this PR makes. touch / ln / mkdir destinations outside the workspace are
ALLOW on master and on the head — pre-existing, unchanged here, and outside this issue's
class (the write-site walk recognises > and a set of move verbs, not every file-creating
verb). Worth its own issue; it is not a regression and not this PR's subject.
Renderer/CI/documentation: no doc-count line is made stale by this PR (the Python count stays
measured, never stored), and the two clean-up failures the branch met on the way here are
fixed rather than silenced — the new test file is tracked so the index-derived scans reach it,
and its subprocess decode is pinned so the locale guard holds. The --help / import checks are
clean on the head.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-014848
Reviewed the code and re-measured the claims independently (predicate only, nothing
executed), master 6667fba7 vs this head, in one geometry whose outside directory is
outside every allowed root (workdir=/workspace):
| command | master ws/ro | this head ws/ro |
|---|---|---|
rm -rf {../out,inside} |
ALLOW/BLOCK | BLOCK/BLOCK |
tee {../out/w,inside} |
ALLOW/BLOCK | BLOCK/BLOCK |
cat > {../out/f,inside} |
ALLOW/BLOCK | BLOCK/BLOCK |
cd {../out,sub} && cat > f |
ALLOW/BLOCK | BLOCK/BLOCK |
cd {../out,{a,b}} && cat > f |
ALLOW/BLOCK | BLOCK/BLOCK |
echo x > a{b}c |
ALLOW/BLOCK | ALLOW/BLOCK (a literal word to the shell too) |
rm -rf build, cp $SRC $DST |
ALLOW/BLOCK | ALLOW/BLOCK |
So the escaping rows move in the safe direction, the literal-brace word keeps its
verdict, and the stated price (rm {dist,build} refused because which spelling runs
is what the text does not say) is exactly what I measured — declared in the docstring
and pinned by a test rather than discovered later.
Also ran this PR's own new suite locally at its head (tests/test_brace_expansion_is_not_a_path.py, 18 passed), and read the two places the rule is asked from: the write-target walk and the move-destination rule, which are the two sides that must agree — the docstring's cross-reference to test_unplaceable_move_destination.py is what makes that a claim rather than a hope.
CI green on both legs (test, test-windows).
|
Correction to my own vote above — the comparison tree is the merge base, not master. At the time that vote was posted (2026-09-18T18:17:32Z), master had already moved to The predicate rows in that vote are unaffected: comparing the branch's own merge base to its What is owed by the third vote. The head does not need to move, and refreshing it would void the two votes already cast.
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-023751
This vote is cast on the landing tree, not on the head. check-merge-freshness.py 1397
reports it STALE (head 567b2a2c, base 6667fba7, behind_by=2), so the run cited by the two
earlier votes judged a base that can no longer be merged. The head does not move — refreshing it
would void both standing votes — so what I measured is the tree this merge would produce:
check-merge-plan-suite.py 1397 1395 --steps
base dc2a3249 (refs/remotes/origin/master)
step 1 (#1397) tree 6660494b8be7 suite OK: 3282 passed, 18 skipped (2m15s)
every step healthy (2 suite run(s))
The premise and the fix, re-measured this cycle rather than inherited. Two trees side by side
(master dc2a3249 and this head), predicate only — _check_sandbox realpaths strings and opens
nothing — in one geometry whose ../out is outside every allowed root:
| command | master ws/ro | this head ws/ro |
|---|---|---|
rm -rf {../out,inside} |
ALLOW/BLOCK | BLOCK/BLOCK |
tee {../out/w,inside} |
ALLOW/BLOCK | BLOCK/BLOCK |
cat > {../out/f,inside} |
ALLOW/BLOCK | BLOCK/BLOCK |
cd {../out,sub} && cat > f |
ALLOW/BLOCK | BLOCK/BLOCK |
cd {../out,{a,b}} && cat > f |
ALLOW/BLOCK | BLOCK/BLOCK |
echo x > a{b}c |
ALLOW/BLOCK | ALLOW/BLOCK (a literal word to the shell too) |
rm -rf build, cp $SRC $DST |
ALLOW/BLOCK | ALLOW/BLOCK |
echo {1..3} |
ALLOW/ALLOW | ALLOW/ALLOW |
The five escaping rows move in the safe direction, the literal-brace word and every in-workspace
form keep their verdict, and the declared price (rm -rf {dist,build} is now refused because
which spelling the shell takes is exactly what the text does not say) is stated in the docstring
and pinned by a test rather than discovered later. This is a widening of refusals, not a narrowing.
This PR's own suite at its head: tests/test_brace_expansion_is_not_a_path.py → 18 passed
(run in a worktree at 567b2a2c).
I also confirm the correction posted above: the earlier cite of "master 6667fba7" was the
branch's merge base, not master's tip — the predicate rows are unaffected, the CI verdict is.
Closes #1396 — a residual measured while reviewing #1392, not inferred from it.
The defect
{a,b}is expanded before the command runs, so a destination or a target carrying one namesa path that is not in the token stream. Read literally, the token is joined onto the cwd, so
every spelling of the list reads as being inside the workspace — the one direction this
guard must never drift in.
Measured on master
67ba7f52(predicate only, nothing executed, one geometry whose outsidedirectory is outside every allowed root), and on #1392's landing tree
45f9bc85— identicalverdicts on both, so #1392 neither closed nor opened it:
cd {../emrg-1396-outside,sub} && cat > fcd {../emrg-1396-outside,{a,b}} && cat > frm -rf {../emrg-1396-outside/doomed,inside}tee {../emrg-1396-outside/written,inside}cat > {../emrg-1396-outside/t,inside}The shell column is a real run (
/bin/sh, a tree the probe built), not a reading of the text.The last row is kept because it is the one shape that does not escape: bash refuses a
redirect whose word expands to two words. The first draft of the test file asserted the escape
for that spelling and the measurement said otherwise, so the redirect is pinned as the shape
that does not, and the escape is witnessed through the verbs that do take several operands.
The change
One class, added next to the variable class it is a shape of, applied at the two places where a
lexeme is read as a path:
_move_destination_is_unresolved— a destination carrying{a,b}or{1..3}is text the walkcannot place, so it fails closed and is reported by its own token, exactly as
cd -and emrg: a move no scope can place is refused, not read as inside #1392'sdestinations are;
_check_sandbox— refused with a message of its own, because thereason is about a list and not about a variable (the variable rule's
cp $SRC $DSTexemptionhas no analogue here: a brace operand is several operands whichever way it is spelled).
The predicate is a
,or a..inside a brace pair. A word carrying braces with neither(
a{b}c) is one literal word to the shell as well and keeps its old verdict; the search is forthe inner pair, which is also what stops the nested spelling from slipping past.
The price is stated, not hidden: a list whose every spelling stays inside is refused too
(
rm {dist,build},cp a {b,c}), because which spelling the command takes is exactly what thetext does not say. The work-around is the one every refusal in this walk already names — spell
them out.
Verification
git add(the index-derived scans cannot see an untrackedfile — the guard that says so fired first, and then the encoding guard found the one
subprocess.run(..., text=True)in the new file, which is the same class that guard exists for):3258 passed, 17 skipped; import check and
python -m emrg --helpgreen.(
sha256[:16] 382caa0456698a4fbefore and after each):test_a_move_through_a_brace_list_is_refused[…]andtest_the_shell_places_a_brace_move_where_the_walk_cannot_follow;{1..3}and the nested-spellingcontrol;
test_a_brace_target_is_refused_for_its_own_reason[…]andtest_the_shell_really_hands_the_list_to_the_verb.rmandteereally reach the sibling directory (the verdictsare asserted on every platform, the witness is gated on a shell that brace-expands —
dash,the usual Linux
/bin/sh, does not, and the test says so instead of asserting a POSIXguarantee that does not exist).
under
tmp_pathand the verdict rows are textual, so no host path is opened.Delta from the issue as filed
The issue asked for the move case only, with a comma predicate. Three deliberate widenings, each
measured rather than assumed: the
..sequence ({1..3}has no comma), the target side (the samelexeme is read as a path by both walks), and the stated price above (an in-workspace list is
refused). Issue #1396 carries the measurements for each.