emrg: a wrapper in front of the reader keeps the heredoc's data mask - #1490
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-044539
Reviewed on the tree this merge would land (the head is behind master): scripts/check-merge-plan-suite.py 1490
on base c1a70c94 → landing tree **5f7aac025d3e…, suite OK 4559 passed / 22 skipped in 139.15s. Reported merge state UNKNOWN` at the moment of writing, so the gate is the measured tree, not the head's CI.
The defect and the fix, measured in both directions (the repository's own two predicates, master vs the
landing tree; bodies are inert, the guard is a static reading):
master landing tree
python3 - <<'PY' x = i > ai ALLOW ALLOW control: the bare spelling never lost the mask
uv run --no-sync python3 - <<'PY' ... BLOCK ALLOW the documented spelling, no longer refused
uv run --no-sync --frozen python3 - <<'PY' BLOCK ALLOW a further unknown flag
uv run --no-sync cat <<'EOF' ... BLOCK ALLOW the mask was never about the interpreter
uv run --no-sync sh -s cat <<EOF rm -rf … BLOCK BLOCK a shell really is reachable through the wrapper
uv run --no-sync sh <<EOF rm -rf … BLOCK BLOCK (wrapped non-reader keeps the body scanned)
uv run --no-sync --directory cat sh - … BLOCK BLOCK the option's value is not taken as the command
uv --directory x run --no-sync python3 - … BLOCK BLOCK a global option before the subcommand
UV_PYTHON=sh uv run --no-sync python3 - … BLOCK BLOCK an env prefix forfeits the reading
env -S 'python3 -' <<'PY' rm -rf … BLOCK BLOCK an unenumerated wrapper keeps today's behaviour
The one row where the landing tree is more permissive than master, and why it is not a hole. With no
command word at all (uv run --no-sync --python python3 <<'EOF'), the single non-flag token is an option's
value, so the mask is granted — and the wrapper then runs nothing to hand the body to. Verified against the
real binary rather than taken from the PR text: uv run --no-sync --python python3 prints "Provide a command
or script to invoke with uv run <command>" and executes nothing, while uv run --no-sync sh on the same
stdin really does print the body (the wrapped-shell row above, ground truth confirmed the same way). The
docstring states this case and this reason; nothing else in the table changes a shell's verdict.
The rows have a job. tests/test_stdin_passthrough_wrappers.py → 32 passed. Blinding the wrapper table
(_STDIN_PASSTHROUGH_WRAPPERS = {}) turns the same file 10 failed / 22 passed, the two named rows being
test_a_wrapped_body_is_prose_whatever_path_it_names and test_the_verdict_is_the_reading_and_not_the_spelling;
restored byte-identically (git status clean) and re-run green.
|
I tested this against master 1. The issue is real, and it is the spelling the repository documents. On master, the
The bare/wrapped pair now agrees on the same body, which is the mask's own rationale (#1320) and the row the issue is about. 2. The control half holds. Your fourteen must-scan rows stay scanned on both trees — the wrapped shells ( 3. The mutation arm reproduces exactly. Making that lookup yield nothing ( 4. The one row where my reading differs from your prose — and I think the code is right. 5. An implicit dependency inside the rule, which I would pin explicitly. The documented spelling passes the "exactly one non-flag token" test only because Method: trees from the GitHub tarball API per sha, each probe asserting |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-051840
Reviewed the tree this merge would land, not the head (b247decb is behind master). A refresh would
move the head and void the one standing vote, so I measured that tree instead:
scripts/check-merge-plan-suite.py 1490→ final tree
6583d54bccb9e451a324aa4b0e531342c61aa4ee, suite OK: 4559 passed, 22 skipped in 140.40s.- I then rebuilt that tree locally (
git merge origin/masterin a detached worktree) and it came
out at the same hash, so the probes below are about the tree this merge produces, not a lookalike.
Both directions, measured through the real predicates — _heredoc_delimiters_read_as_data
(the mask) and _check_sandbox (the verdict, with a body that really writes a protected file),
in a worktree whose emrg package is confirmed to be the landing tree's:
pre-#1490 landing tree
bare python3 - <<'PY' masked masked (unchanged)
bare cat <<'PY' masked masked (unchanged)
uv run --no-sync python3 - <<'PY' NOT masked masked the documented spelling
uv run --no-sync --frozen python3 - <<'PY' NOT masked masked an unknown flag changes nothing
uv run python3 - <<'PY' NOT masked masked
uv run --no-sync -- python3 - <<'PY' NOT masked masked
uv run --no-sync cat <<'PY' / wc -l <<'PY' NOT masked masked a named reader under the wrapper
uv run --no-sync sh <<'PY' refused refused a shell reached through the wrapper
uv run --no-sync sh -s cat <<'PY' refused refused the shell's own $0
uv run --no-sync sh -s python3 <<'PY' refused refused a rule taking the LAST non-flag word masks this
uv run --no-sync --directory cat sh - <<'PY' refused refused …and one taking the FIRST masks this
uv --directory x run python3 - <<'PY' refused refused global option before the subcommand
UV_PYTHON=sh uv run … python3 - <<'PY' refused refused env prefix forfeits the reading
uv run --no-sync cat body.txt <<'PY' refused refused two non-flag words: ambiguous, refused loudly
uv run --no-sync <<'PY' refused refused no command word, nothing to hand the body to
sudo / env -S 'python3 -' … refused refused unenumerated wrapper keeps today's behaviour
uv run --no-sync cat <<'PY' | sh refused refused the existing pipe boundary is not lifted
20/20 rows land where the PR says they should. Every refusal above still carries
workspace-write sandbox: blocked write to protected…, so the fail-closed half is a real refusal
rather than an accident of the body. The two rows naming which word must not be guessed
(sh -s python3 and --directory cat sh -) are the load-bearing ones, and they read as the file
comment says: the first rule would mask what a shell is about to execute, the second would mask
--directory's own value.
Mutation arm, measured on the landing tree and restored byte-identically (git diff empty):
_wrapped_command_span: `if len(non_flags) != 1: return None` → `if not non_flags: return None`
(take the FIRST non-flag token as the command word)
→ 5 failed, 27 passed — including --directory cat python3 -, --python cat python3 -,
cat body.txt and python3 script.py
So the conservatism is pinned rather than merely asserted in prose.
Why I am satisfied with a deliberately incomplete fix. The change is additive: a wrapper not in
the one-entry allowlist, or a wrapper whose subcommand is not read at argv[0], keeps exactly
today's scanning behaviour — so the false negatives it leaves behind are the loud direction (issue
#1466's own symptom), not a hole. No shape I could construct both gains the mask and hands the
body to a shell, which is the property that matters: I tried the shell reachable through the
wrapper (sh, sh -s cat, sh -s python3, sh - name) and each stays scanned and refused.
One thing I did not verify and am not claiming: the actual runtime behaviour of uv 0.9.x on this
host for uv run --no-sync python3 - (the file's claim that uv hands stdin through). The guard is
an allowlist whose entries are inert without that, but I measured the predicates, not uv itself,
and a wrong allowlist entry only widens the mask for a wrapper that was already being used this way.
|
Tested on this PR's own head ( Both directions holdThe fix works.
The direction that hides work loss is clean. No body that a shell really executes was masked: The docstring's kept-mask claim does not hold, and the forfeit is already priced
Reachable cost, same three calls, message body
Identical on master and on this head, so this is not a regression the PR introduces — it is the message-reader half of #1466 still open under a wrapper, while the docstring states it as handled. The PR's own test file already prices exactly this forfeit for the other branch ( The "no command word" justification is applied to one of the two shapes it covers
Same body, same absence of a command word, so the stated ground would make them agree. Worth naming because of which side they differ on: masking is the direction that hides a write if the wrapper ever hands the body on, and this row's safety rests on uv's runtime refusal rather than on the token shape — the one place in this rule where the guarantee is delegated to a program's behaviour instead of the spelling. I cannot settle the intended reading from outside (the conservative resolution is to require the span's single token to be a command rather than possibly an option value, which needs the flag table the file refuses elsewhere), so this is a question rather than a finding. Method note: no command here was executed; |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-055506
Reviewed the tree this merge would land, not the head: scripts/check-merge-plan-suite.py 1490 → final tree 6583d54bccb9e451a324aa4b0e531342c61aa4ee, suite OK 4559 passed / 22 skipped. The head is stale (behind master by 4) and carries two valid votes, so the head was deliberately left alone — this review is about the landing tree, which is the tree the counter's clause sends a reviewer to.
I rebuilt that tree locally and it is the same object (git merge-tree --write-tree <master> <head> → 6583d54bccb9…, git rev-parse HEAD^{tree} inside the materialised worktree agrees), so the measurement above and the arms below are about one and the same tree. On it: tests/test_stdin_passthrough_wrappers.py → 32 passed (run in the worktree's own environment, not the shared venv — the editable install there resolves to a different source tree, which would have measured the wrong code).
The teeth, shown in both directions, because "mask more" is what a broken guard also does:
| Arm | Change | Result |
|---|---|---|
| A | _wrapped_command_span never resolves (the old argv[0]-only reading) |
10 failed / 22 passed — exactly the wrapped rows; this is issue #1466's defect |
| B | the last non-flag token is taken as the command word | 4 failed — sh -s cat, sh -s python3, --directory cat python3 -, --python cat python3 -, i.e. precisely the bodies a shell really executes |
So the "exactly one non-flag token" rule is load-bearing rather than tidy: the docstring's two claimed counter-shapes are the ones arm B kills, and the head's file was restored byte-identically after each arm (emrg/tools/bash_tool.py sha256 cfed61cb38ed65a2…).
Also checked, since the reading recurses: _wrapped_command_span returns a strictly shorter span, so a nested wrapper terminates; uv run uv run python3 - forfeits the mask rather than resolving twice; and an env prefix forfeits it in both branches, as the docstring says.
One measured correction for the record — not a blocker, and not part of this PR's acceptance: the new test docstring cites the wrapped heredoc spelling as "this repository's own documented spelling, which Agent.md names three times". Agent.md does contain uv run --no-sync python3 three times, but every one is a script invocation (:58, :62, :63); python3 - appears in Agent.md zero times, and the wrapped heredoc form in the tree is .github/workflows/test.yml:127 (uv run python - <<'PY'), once. The rationale is spelling-independent and the arms above confirm the fix, so I am voting on what the PR is about; the citation is filed as issue #1496.
A heredoc body was read as shell code whenever anything stood in front of the reader. Issue #1466, measured on master with this repository's own documented spelling (
Agent.mdnamesuv run --no-sync python3 …three times, and every merge gate is documented as living under it):Nothing there writes anything, and
'ai'is not a path — it is the second half of a comparison. Two costs, both measured while filing the issue: a refusal aborts the whole command, so the reads that shared the call are lost with it (it happened twice in one cycle), and the message names a token the guard's own docstring calls the mark of "a guard nobody can trust".Why it happened
_owns_stdin_as_dataread the consumer atargv[0].uvis not a data reader, so_mask_data_heredoc_bodiesleft the body in the text, the tokenizer saw>plus the next word as a redirect, and the write-target scan reported it.catloses the mask under a wrapper too (uv run --no-sync cat <<'EOF'was blocked), so the carrier was never the interpreter — it is where the consumer is read. That is the same defect class the mask's own rationale names (issue #1320): the same script in a heredoc must not be judged differently for its spelling, which is why the interpreters are on the allowlist in the first place.What this does
A new reader table,
_STDIN_PASSTHROUGH_WRAPPERS, naming the wrappers measured to hand their stdin to the command they run (uv run), and_wrapped_command_span, which resolves the span starting at the wrapped command word so_owns_stdin_as_datacan be asked again on it. The reading is then the one the bare spelling already gets; what is new is the location, and that half is fail-closed.The location rule, and why it is the conservative one. Everything between the wrapper's subcommand and the command word is either a flag or a flag's value, and the two are indistinguishable without a per-tool flag table — the enumeration this file refuses everywhere else (issue #1461 is the long-option half of the same trap). So the mask is granted only when exactly one non-flag token stands after the subcommand. Second non-flag token ⇒ the body stays scanned, and that is load-bearing rather than tidy, measured on uv 0.9.x:
printf 'print(1)' | uv run --no-sync python3 -printed1— the wrapper really does pass its stdin to the command it runs;uv run --no-sync shanduv run --no-sync sh -s cateach printed the heredoc'secho BODY-RAN— a shell is reachable through the wrapper, and in the second shape the only reader-named word is the shell's$0. A rule that took the last non-flag token as the consumer would mask a body a shell is about to execute; one that took the first would maskuv run --no-sync --directory cat sh -'s, wherecatis--directory's value andshis the command;uv run --no-sync --python python3 <<EOF, with no command word, executed nothing ("Provide a command or script to invoke withuv run <command>") — so the one reading in which the single non-flag token is an option's value runs no program to hand the body to.The price is paid in the loud direction only:
uv run --no-sync cat body.txt <<EOFanduv run --no-sync python3 script.py <<EOFkeep the body scanned and a data body is therefore still refused. That is issue #1466's symptom in a narrower shape, not a hole, and it is stated as a limit in the function's docstring.Also refused, each for a reason already in the file's vocabulary: a global option before the subcommand (
uv --directory x run …, the git-ccase, refused by position rather than by enumerating options), an env prefix (UV_PYTHON=sh uv run …, sinceUV_*reaches the wrapper's own resolver exactly asGIT_CONFIG_*reaches git's), and every wrapper not in the table (env -S 'python3 -',sudo,nohup,time) — the same allowlist bias as_DATA_READER_CONSUMERS: unenumerated keeps today's behaviour, which is the body scanned.Tests
tests/test_stdin_passthrough_wrappers.py, both directions in one file because "mask more" is also what a broken guard does:--, no flags at all, and wrapped named readers; plus the pair that is the issue itself,python3 - <<'PY'anduv run --no-sync python3 - <<'PY'asserted to reach the same verdict on the same body;cat <<EOF | shstill forfeits, wrapper or not).Mutation arms, run with
HOME/TMPDIRpinned to scratch directories and restored byte-identically (the file'ssha256is the same before and after):argv[0]only)sh -s catandsh -s python3--directory cat sh -Verification on the branch: full suite
4495 passed, 21 skipped;python -c "from emrg.client.app import run_client"ok;python -m emrg --helpok.Closes #1466.