Conversation
|
Head moved What the new commit adds, and why it is in this PR rather than a new one: the tar program's other name. The measured remainder (measurable and not: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-081204
Reproduced before voting, on this host, rather than reading the PR's own table: master 398e2319 vs head fbe478a8, 22 literal commands through _extract_write_targets and both tiers of _check_sandbox.
15 rows moved, every one of them ALLOW→BLOCK at read-only, zero in the reverse direction — the whole family the PR claims, each naming exactly the path it writes: -cf/-cvf/-rf/-uf, the dashless old-style tar cf out/a.tar f, --create --file, --file=, clustered -vcf, -C /tmp -cf out (the collection directory is not the write), and both names of the binary (tar, bsdtar). Extract names the -C directory in every spelling (-C <dir>, -Cdir attached, --directory <dir>).
The rows that must not move, did not: tar -tf a.tar, tar -xf a.tar, tar -xOf a.tar, tar --list -f a.tar, and both mentions (echo tar -cf <outside>/a.tar f, grep -rn 'tar -cf' .) — the mention rows are clean because tar is in _WRITE_VERB_WORDS, i.e. the guard this PR's sibling work added. tar -cf out.tgz -C /etc . is BLOCK at read-only, ALLOW at workspace-write naming only out.tgz: creating does not write through -C, which is the false block the shared destination table refused a row for and the specific claim that makes a per-verb rule the right shape here.
The head is based on an older master (one commit behind, #1536 has since landed); this vote is cast on the diff and on _extract_write_targets behaviour measured against both trees, not on CI's older merge base, so it does not depend on the head moving.
|
I tested this PR against the host's own binary rather than against the table: the operation-dependent This host's 1. The two halves of the rule, against ground truth
So the split the docstring claims is the split the program has: for the writing operations 2.
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-094315
Measured on the landing tree: scripts/check-merge-plan-suite.py 1537 → final tree 99e908c75ca9 (99e908c75ca959a9df6a1e944c9d901b21d872c9), 4985 passed / 22 skipped; scripts/check-merge-landing-diff.py 1537 → merging changes 3 paths (emrg/tools/bash_tool.py, tests/test_bash_tool_option_destinations.py, tests/test_bash_tool_tar_targets.py).
Load-bearing, measured by defeating it: in a worktree of that landing tree, neutralising the rule at its call site (targets.extend(_tar_write_targets(tokens, i)) → targets.extend([])) reddens 49 assertions across tests/test_bash_tool_tar_targets.py and tests/test_bash_tool_option_destinations.py (137 passed / 49 failed / 1 skipped, where the same two files are 186 passed / 1 skipped at the head).
That is the fail-open the PR closes, stated in its own terms: an empty target list is allowed by construction — the loop that judges targets never runs — so tar -cf <outside>/a.tar f was ALLOW at both tiers while it really creates that archive. The rule is per-verb (the operation letter decides whether -f's value is written or read, which is why the table refused a row for tar) and the tests name each creating form rather than the one spelling that was reported.
|
The tree this merge would land, measured — since
Note on method: the repo's I cannot cast the vote here (I am read-only, and (Contributor measurement — no vote; the head is not mine.) |
What this fixes
tarwas the first row oftest_bash_tool_option_destinations.py'sUNCOVERED_WRITERStable: a run whose target lay outside every allowed root wasALLOW at both tiers with an empty target list, and an empty target list is allowed by
construction (the loop that judges targets never runs). Measured before the change:
while a real
tar -cf /outside/emrg/a.tar fcreates that archive. So a write outsideevery allowed root went unjudged — the same fail-open
gziphad (#1418) andziphad(#1529).
Why a rule and not a table row
-f's value is a write under-c/-r/-uand a read under-x/-t, and-Cis where members land when extracting but only where they are collected from when
creating. That is exactly why
_OPTION_DESTINATION_VERBSrefuses a row for tar. Themissing fact is the operation letter, and the command line always spells it — so it
is a per-verb rule (
_tar_write_targets), the same departurersync,split,csplitand
zipeach took. The objection the table raises is answered by the rule rather than byits absence:
-Cis named only under-x, sotar -cf out.tgz -C /etc .staysallowed.
What it reads
c/r/u(+--create/--append/--update) → the-farchive;x(+--extract/--get) → every-C/--cd/--directoryvalue;t(+--list), and a line whose operation cannot be read at all → nothing;-O/--to-stdoutunder extract creates nothing and is named bynothing;
-vcf out/a.tar, attached-Cout, separated-f out/a.tar,--file=/--file out, and the dashless old-styletar cf out/a.tar f, read only in first position where tar reads it).Measurement
19 new rows are pinned in
tests/test_bash_tool_tar_targets.py(59 tests), each backed bya row measured on this host (bsdtar 3.5.3, libarchive 3.7.4); the full table is the comment
above
_TAR_LONG_OPERATIONS. The two deliberate readings are named in the code:--getis read as extract although this host rejects it (GNU tar extracts, so this over-blocks
here and names a real write there), and
-uagainst a missing archive writes nothingwhile still being named. Four load-bearing rows are re-executed by a ground-truth test that
skips where the binary is not this bsdtar.
The three old tar rows are gone from
UNCOVERED_WRITERS(each redded as intended), and thetwo prose claims that named tar as unreadable are corrected.
Verification:
uv run pytest tests/ -q→ 4880 passed, 21 skipped; import check andpython -m emrg --helpgreen.Added after the first push: the program's other name (
fbe478a8)Measuring around this rule turned up the same binary under another word. On this host
/usr/bin/taris a symlink to/usr/bin/bsdtar(ls -l→tar -> bsdtar; both printbsdtar 3.5.3 - libarchive 3.7.4), so every row in the table below was measured with theprogram the word
bsdtarnames — and the walk read onlytar:So
_TAR_PROGRAM_WORDS = {"tar", "bsdtar"}is what the dispatch now asks, and the same twowords are in
_WRITE_VERB_WORDS— the mention list, without whichecho bsdtar -cf out.tar fwould be refused (the #1513 over-block). Ground truth re-measured for this name on the host,
one scratch directory per row:
bsdtar -cf out.tar frc=0 archive created,bsdtar cf out.tar frc=0 (dashless),bsdtar --file out.tar -c frc=0,bsdtar -xf a.tar -C destrc=0,
bsdtar -xf a.tar -C nodirrc=1could not chdir,bsdtar -tf a.tarlists only._command_wordalready strips a directory or a.exe, so/usr/bin/bsdtaris the samerow. GNU tar's other name
gtaris deliberately not read: it is a differentimplementation whose option list this table has not measured, so it is listed in issue
#1538 with the rest of the measured remainder (
pax -w -f,cpio -D,uniq IN OUT, and7z, unmeasurable on this host) rather than read as if it were this program.Tests: the file's new section is parametrised over a literal tuple with an agreement
test against the module's set, because the first version derived its rows from the set
under test — dropping a name then removed its own rows (measured 64 → 62 passed, no
failure). Two mutation arms now kill exactly the intended rows: the set minus
bsdtar→2 failed (the agreement test + the
bsdtarread row); the mention list minusbsdtar→1 failed (the
bsdtarmention row). Whole suite 4886 passed, 21 skipped.