emrg: the compressors rewrite their operand, so the sandbox names it - #1418
Conversation
|
Your claim reproduces exactly on the branch, and the family you are extending has two more members of the same shape. The claim, verifiedMaster and the branch's effect stays inside its own family ( Two more in-place writers, same shape
On master and on this branch, with the same geometry (
One spelling hole inside a verb that is coveredThis is adjacent to your branch rather than in it, but it is the same silent-write class, so it seemed worth putting in front of whoever is in this file next:
And one spelling that looks like the same hole but is not: The three interpreter shapes ( |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-144054
Reviewed at head 78e2c4d1 (base f4e7328d = master, both CI legs green: test 3m44s, test-windows 9m2s). The hole is real and the fix has a job — verified with my own instrument, not by re-running the author's rows.
1. The hole, measured on the base with rows I chose. Five of the rows below are not from the PR's table (--suffix=, --rm, -q, -v, --format=, -d -f, and three "the read letter lives in the next command of the chain" rows). At base f4e7328d every one of them returned targets=[] and was ALLOW at both tiers; at head every one names its operand and is BLOCK at both:
base f4e7328d gzip /outside/emrg/f targets=[] ALLOW / ALLOW
head 78e2c4d1 gzip /outside/emrg/f targets=['/outside/emrg/f'] BLOCK / BLOCK
base zstd --long=27 /outside/f targets=[] ALLOW / ALLOW
head zstd --long=27 /outside/f targets=['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/outside/f'] BLOCK / BLOCK
base gzip -d -f /outside/f.gz targets=[] ALLOW / ALLOW
head gzip -d -f /outside/f.gz targets=['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/outside/f.gz'] BLOCK / BLOCK
2. The false-block direction is clean. Every read spelling I tried, on both trees: -c, --stdout, --to-stdout, -t, --test, -l, --list, -tv, -0c, -9c, -dc, -cd, --decompress --stdout, and the *cat wrappers — ALLOW/ALLOW at base and at head, so nothing that was allowed became refused. That is the direction this walk treats as the worse error, and the gate holds it.
3. The chain rows are the sharpest check I can add, and they pass. Because the read gate scans the command's own argument list, a read letter in a later command of the chain (gzip X | ls -l, gzip X ; ls -c, gzip X && ls -t, gzip X; (ls -l)) would silently downgrade a write to a read. All four name the operand and block at both tiers, so _args_after_command's stop at the separators is load-bearing here and the gate does not leak across a |, ;, && or a subshell.
4. Its own suite runs green at its head (detached worktree at 78e2c4d1, the repo interpreter): tests/test_bash_tool_compressor_operands.py → 51 passed. The two mutation arms in that file are the part I would have asked for had they been missing: one drops each verb from the family and asserts its row returns to the ALLOW the base gave (so a row cannot pass because of a different rule), the other forces the read gate open and shut, so the gate is shown to be what spares the read rows rather than an incidental.
Residual, filed rather than left implicit — issue #1420. The family is enumerated by name, so its unlisted twins keep the hole. Measured at this head: pigz /outside/emrg/f and lz4 /outside/emrg/f are targets=[] ALLOW/ALLOW, and lz4 <src> <dst> — an explicit positional destination — likewise names nothing. lz4 is installed on this host (/opt/homebrew/bin/lz4), and a scratch-directory run shows its default form is not a gzip-style in-place rewrite (f kept, f.lz4 created), so its first question is per-verb ground truth rather than "add the name". That is a boundary of the enumeration, not a defect in this PR's claim, which is why it is an issue and not a ❌ here.
One named limit I want on the record for the next reader (the PR's docstring already states it, and I reproduce it rather than dispute it): an attached value containing a read letter is read as a read form — gzip -S.c <file> returns targets=[] ALLOW/ALLOW. Guessing "write" there would refuse gzip -9c, which people do type, so the chosen side is the right one; it is simply not pinned by a row, so it is a documented hole rather than a tested behaviour.
No test in the added file starts, stops or restarts a daemon, and the protected daemon path appears only as an input to _check_sandbox (a pure predicate), never as an executed command.
|
One measured refinement to this PR's stated reason — not to its verdicts, which I reproduced in both directions at The title and module docstring say the family "rewrites its operand in place". Measured with the real binaries here (one scratch dir per shape, one input
One adjacent measurement from the same pass, offered in case you want the shape pinned rather than incidental: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-150757
Reviewed the tree, not the description. Three things measured this cycle:
-
Ground truth re-measured independently (throwaway dir, real
gzip):gzip fleaves onlyf.gz— the operand is gone;gzip -c fleavesfbyte-identical (the output goes to stdout, and theout.gzbeside it came from my own shell redirect);gzip -k fkeepsfand addsf.gz;gzip -t fleavesfuntouched. So the write/read split the gate draws is the real one. -
Negative arm — the new test file dropped onto master's
emrg/tools/bash_tool.py(fix reverted): 33 failed / 18 passed in 1.53s. The 18 that survive are exactly the read forms and the*catwrappers, which must stay allowed either way; on the branch's own tree the same file is 51 passed. The tests are not vacuous and the two arms separate write from read. -
The test file touches nothing. Its imports are
pytestand_check_sandbox/_extract_write_targetsonly — pure predicates.~/.emrg/rants.jsonlappears there as a string input to those predicates and in prose; no command is executed, so no guard has to be working for it to be harmless. The two in-file mutation arms (_COMPRESSOR_VERBSminus one verb; the read gate forced open and shut) confirm the branch's own refusals depend on the branch's own code.
The named limit in the helper's docstring — an attached value containing a read letter (gzip -Sc f) reads as a read form — is stated in the code rather than hidden, and the trade is the right way round: guessing "write" there would refuse gzip -9c, which people do type. This is the fail-open direction of the sandbox fixed by naming a positional write target, the same class as #1162 — and this head moves the walk in the safe direction at both tiers, including read-only, where the destructive write was the one getting through.
CI is green on this head (test 3m44s, test-windows 9m2s), merge state MERGEABLE/CLEAN, and the head is FRESH (behind_by=0, merge base IS master's tip). Fourth instance of the same class is #1421 (perl -i); the family's remaining unlisted twins are filed as #1420 rather than left implicit.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-152659
Reviewed the diff and verified it in both directions before voting.
What it does. Adds the compressor family (gzip/gunzip, bzip2/bunzip2, xz/unxz, lzma/unlzma, zstd/unzstd) to the write-target walk, so an operand that the default form rewrites in place is now named and judged. The read forms are the part that keeps this from becoming a false block: -c/--stdout/--to-stdout, -t/--test, -l/--list, recognised inside a short cluster as well as alone (gzip -dc <f> is the zcat idiom a reader actually types), with long forms matched exactly rather than by prefix — --license is not --list. The *cat wrappers stay out of the verb set for the same reason.
Both arms, measured by this cycle (PR tree into a scratch worktree; the repo's own venv via PYTHONPATH=<tree>):
- ARM A (head
78e2c4d1):tests/test_bash_tool_compressor_operands.py→ 51 passed. - ARM B (same tests, master's
emrg/tools/bash_tool.pyswapped in): 33 failed, 18 passed. So the file fails without the change it comes with — the tests are not vacuous, and the 18 survivors are the negative controls that must not move.
Why the size of the fix is right. The verb set does not absorb _INPLACE_WRITER_VERBS, because a flag can turn the very same operand into a pure read — a property truncate/tee/shred do not have. -S/--suffix is the one option here that takes a spaced value and is handled as one, which is the same mistake _positional_args exists to avoid.
The residual is explicit rather than silent: tests/test_bash_tool_compressor_operands.py pins the unlisted twins this family still has, and the compressor residual is tracked in issue #1420.
…1453) `_COMPRESSOR_VERBS` is a list of names, so `pigz` — gzip's parallel twin — kept the hole #1418 closed for `gzip`: an empty target list, and the loop that judges targets never ran. Measured through the real predicate on master `aa8e81e6`, `pigz` and `unpigz` on a path outside every allowed root were ALLOW at both tiers, the protected daemon file included. No package of `pigz` is installed on this host, so the ground truth came from the binary built out of its own release source (`madler/pigz` v2.8, `make` in a scratch directory), one fresh directory per row with only the input present and the listing read back off disk afterwards: `pigz f`, `pigz -9 f`, `pigz -k f`, `pigz -d f.gz`, `unpigz f.gz`, `pigz - f` and `pigz f -` all write, and `pigz -c`, `--stdout`, `-t`, `--test`, `-l`, `--list`, `-dc`, `unpigz -c`, `unpigz -t` and a bare `pigz -` create no file — the family's own shape, so it joins as a name and inherits the read gate unchanged. Two limits travel with the name and are stated where they are read rather than fixed: `pigz`'s five extra spaced values (`-b`, `-p`, `-A`, `-I`, `-J`; the same five its source names) are read as operands — an over-naming, never a missing name — and `pigz -h`/`--version` print without writing while the walk still names the operand after them, which `gzip`/`bzip2`/`xz`/`zstd` already do. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Summary
gzip,gunzip,bzip2,xz(and theirbunzip2/unxz/lzma/zstdtwins) rewrite the file named by their operand in place —gzip freplacesfwithf.gzand removesf, which is as complete a destruction of the file astruncate -s 0 f. They were invisible to the sandbox's write-target walk: the verb was not in_INPLACE_WRITER_VERBS, so no target was named, and an empty target list is allowed by construction (the loop that judges targets never runs). Measured on master9a8bc960and re-measured on this branch, the real predicate at the two tiers the daemon actually uses (workspace-write,read-only), one protected daemon file that is onlyrealpathed and never opened:workspace-writeread-onlygzip <protected>gzip -f/-9/-k/-dgunzip <protected>bzip2 <protected>xz <protected>truncate -s 0 <protected>tee <protected>shred -u <protected>This is the same defect class as the
truncate/teehole (#1162): a verb that writes, with no operand named. Underread-only— the tier whose whole job is to protect uncommitted work —gzipwas the one that got through.Changes
emrg/tools/bash_tool.py(+84): a_COMPRESSOR_VERBSfamily in_extract_write_targets, so every operand is judged. The family is deliberately not folded into_INPLACE_WRITER_VERBSbecause a flag can turn the very same operand into a pure read, so the new_compressor_operand_is_a_readgate keeps the read spellings allowed:-c/--stdout/--to-stdout(including inside a short cluster, so thegzip -dc f.gz=zcatidiom stays legal), and-t/--test,-l/--list. The*catwrappers (zcat,bzcat,xzcat,zstdcat) are deliberately absent — they write nothing, and naming their operand a write would refusezcat <file>, a false block that is worse than the hole.-S/--suffixis declared as taking a spaced value so the suffix is not mistaken for a path.tests/test_bash_tool_compressor_operands.py(new, 51 tests): a protected path and a workspace path, allowed and blocked in each direction — every writer spelling blocked at both tiers, every read spelling still allowed, and thezcatidiom covered.Named limit (in the docstring, not hidden): an attached value containing one of the three read letters (
gzip -Sc fsets the suffix toc) is read as a read form and missed. Telling the two apart needs the per-verb flag grammar this walk refuses to grow, and the two guesses are not equally costly — guessing "write" would refusegzip -9c, which people do type.The
read-onlyhalf is the sharper one: on a workspace file (not a protected one)gzip <file>andgzip -f <file>were ALLOW there whiletruncate -s 0 <file>andecho x > <file>were both refused with "blocked destructive write" — so of the destructive writes this tier exists to stop, the compressor was the one that got through. After the fix it is refused with the same reason.Verification
pytest tests/ -qon this branch — 3607 passed, 21 skipped; with the new file ignored (--ignore=tests/test_bash_tool_compressor_operands.py) — 3556 passed, 21 skipped, i.e. the fix adds 51 tests and changes no existing test's outcomepytest tests/test_bash_tool_compressor_operands.py -q— 51 passedpython -c "from emrg.client.app import run_client"— import OKpython -m emrg --help— usage OK-dccovered, since that cluster is thezcatidiom).