Repository navigation
fix(execpolicy): a redirect is not a command separator - #9
SparkofSpike wants to merge 2 commits into
Conversation
A lone `&` inside a word was fed back into the classifier's own word walk, and `split_command_segments` never separates a bare `&`. The word therefore reproduced itself until `MAX_WRAPPER_DEPTH` blew, `command_invocations` returned `None`, and the caller treated the whole command as unclassifiable. On Windows the runtime floor turns that into a non-bypassable block, so any command carrying `2>&1` — a plain redirect — could not run at all, and the agent burned turns retrying different spellings of the same refusal. Two changes, one cause: - `split_command_segments` now separates a `&` that is a command separator (POSIX job control, and cmd.exe's own separator) while keeping the `&` of a redirection inside the word: `>&` copies a file descriptor and `&>` is bash's combined redirect. - `command_invocations` re-parses a word only when re-parsing can yield a different token stream — whitespace, `;`, or `|`. A `&` that survived segmenting is part of a redirect, and recursing on it could only reproduce the same word. Separating the bare `&` also keeps a destructive second stage reachable: `echo x&rm -rf /etc` now yields an `rm` invocation instead of being refused wholesale by the depth cap. Tests: `redirects_stay_classifiable_and_a_lone_ampersand_still_separates` fails on the previous code (`a redirect must stay readable instead of failing closed: git log 2>&1`) and passes on this one. `test_cases_09.rs` no longer asserts the Windows floor as the expected outcome for `cargo test & curl https://example.com`; that command is classifiable and now reaches the reviewer like every other approval-shaped command.
Adversarial review — PR #9: "a redirect is not a command separator"VerdictThe fix holds. The root-cause analysis is accurate, both edits are needed and Findings (most severe first)F1 — CONFIRMED:
|
| command | previous code | this PR |
|---|---|---|
echo "a & b" |
readable, single argv | invocations [["echo","a"],...,["b"]] — phantom b |
echo "a&b" |
None (fail-closed) |
invocations include phantom ["b"] |
echo 'x & rm -rf /etc' |
level=Dangerous via None |
invocations include ["rm","-rf","/etc"] (a destructor the shell never runs), level=Dangerous |
The quoted-&-with-rm case is the sharpest: command_invocations("echo 'x & rm -rf /etc'") now yields an rm -rf /etc argv, which reaches
argv_is_destroyer at crates/tui/src/tui/auto_review.rs:1565-1570. The two
command_invocations consumers there are both hold-only (block), never
allow gates, so a phantom rm can only over-hold — it cannot let a real
destroyer through. Consequence: benign commands quoting shell metacharacters
(git commit -m "fix & feature", checks that grep for rm -rf text) get
split into phantom invocations and may be refused/held spuriously. This is a
availability / classification regression, not a security opening. Old code
failed closed on the same input (returned None), which is where the bug the
PR fixes came from; but the fix could equally skip splitting when the & is
inside quotes, at once preserving readability and not inventing commands.
I could not turn this into a false negative: an innocent & is split too,
never merging two real commands, so no real separator is hidden.
F2. CONFIRMED: analyze_command levels are byte-for-byte unchanged across the adversarial battery — blast radius is well bounded
I captured SafetyLevel for 50 commands (redirects, glued separators, quoting,
eval, curl|sh, a&b… shapes) under both the previous and new source. That
list was identical in order and level. The only behavioral delta the PR creates
is in command_invocations() — every & command went from None(fail-closed)
to a readable Vec. So the functional effect is confined to the three
consumers at auto_review.rs:1364, 1376, 1569 (the Windows floor and the
destroyer hold). The Windowsnpm-launcher hard-blow the PR targets
(command_invocations == None ⇒ UnclassifiedWindowsInvocation) is exactly what
changes. That is the correct, contained surface. TUI-side I did not build
(codewhale-tui takes tens of minutes on this box); the 72 passed CI claim
was not verified by me.
F3. CONFIRMED: no discovered invocation is lost — provably a superset
Every & that is not preceded by > and not followed by >
(command_safety.rs:2033) splits; the only survivor inside a segment is the
redirect form. &-free commands are byte-identical between versions. Therefore
command_invocations and split_command_segments can only ever enumerate a
strict superset of the old (readable) set. I tested the shapes the task named —
a>&b, a&>b, 2>&1&rm -rf /etc, cmd &>file & rm -rf /etc, x>&y&z — plus
echo >& rm -rf /etc, cmd > file >& rm -rf /etc, sort < in > out & mv, and
the second rm stage reappears in every case the shell would actually run it
and in none where the & was a redirect. The author's argument that "edit 2
alone would be a weakening" is correct: with only the & removed from the
re-parse predicate and no split, echo x&rm -rf /etc would read words
["echo","x&rm",…] and the rm never discovered. Both edits must land
together — they do.
F4. Confirmed (correct behavior, not a defect): redirect spellings are all preserved
I exercised the operator list for the guard table:
| spelling | survives split? | evidence |
|---|---|---|
2>&1 |
yes | git log 2>&1 readable |
>&2 |
yes | >&2 echo hi → [">&2","echo","hi"] |
1>&- |
yes | single token, readable |
&>file |
yes | tool &> out.txt readable |
&>>file |
yes | >>&file single token |
2>&1& (trailing) |
yes→bare & splits after |
2>&1&rm -rf /etc → ["2>&1"],["rm…"] |
N>&M |
yes | exec 3>&1, 2>file 3>&1 |
The trailing-& sub-case (2>&1&rm) is itself nearly the acid test for F3 and
works. The >&/&> guard correctly discerns redirect from backgrounding.
5 (asked) — test_vacuity
The new test redirects_stay_classifiable_and_a_lone_ampersand_still_separates
is not vacuous: run against the previous code it fails on the first assert
(git log 2>&1 → None), and its second half reproduces the F1 phantom for the
spaced echo x & rm -rf /etc (which is genuinely a real separator and must be
rm). The is_some assert is the one the old code cannot pass. Credible.
6 — test_cases_09.rs assertion change is correct, and the Windows floor isn't dead
The deleted branch asserted Block(UnclassifiedWindowsInvocation) for cargo test & curl … under the buggy behavior; with the fix that command is
classifiable, so the correct expectation for it on all platforms is
ConsultReviewer (still refuses auto-approval). Real coverage of the floor is
retained elsewhere: auto_review.rs:2017 asserts deep sh -c nesting still
yields UnclassifiedWindowsInvocation via the depth cap, and
windows_session_runtime_risk/.._risk_scoped still fire on genuine
unclassifiable nesting. Not a coverage hole.
7 — Comments/doc accuracy
- The comment at
command_safety.rs:2030-2032is accurate for POSIX and for
cmd.exe bare-&. - The depth accounting is unchanged and, now that re-parse no longer recurses on
redirects, the previously-infinite redirect loop is the one real depth bug
the PR removes;MAX_WRAPPER_DEPTHbounds the genuine nestedsh -ccase
still covered atauto_review.rs:2017. No miscount found.
Bottom line
No adversary succeeded against it. The core reclassification (client
&-separator vs. redirect) is sound, the two edits are mutually necessary, the
effect is precisely scoped to two hold-only consumers, and no real destructive
invocation was demonstrated lost. Confirm (i.e. the bug is fixed and the
fix does not open a destructive-path dimpling). Highest-value follow-up:
teach split_command_segments about quotes so & inside quotes doesn't
manufacture phantom commands (F1), which currently flags more than it should
and would also trip a user grepping for 2>&1 in their own helper text.
References (lines in commit d37431a):
crates/execpolicy/src/command_safety.rs:2033-2035— guard + bare-&splitcrates/execpolicy/src/command_safety.rs:2096-2097— re-parse predicatecrates/tui/src/tui/auto_review.rs:1364—None→ Unclassified (the bug)crates/tui/src/tui/auto_review.rs:1565-1570— hold-only use ofcommand_invocationscrates/tui/src/core/engine/tests/test_cases_09.rs:474-486— changed windows-floor assertion
Attribution
🤖 Generated by SpikeBot 003(ClaudeCode-JP)
The redirect fix made `&` a separator but left the scanner quote-blind, so an operator inside quotes was split anyway. The adversarial review of this branch caught the result: `git commit -m "fix & feature"` read as two commands, and `echo 'x & rm -rf /etc'` produced an `rm` invocation the shell never runs. Both reach the destroyer hold, which can only refuse a command that was fine — and a spurious refusal is the bug class this branch exists to remove. Two parts: - `split_command_segments` tracks quote state, so an operator inside quotes stays in its word. The read-only grammar already promised this (`rg 'a && b; c' src`); the shared scanner now agrees. - `command_invocations` re-parses a word only when it is a shell's script argument (`sh -c`, `bash -lc`, ...). An ordinary argument that happens to contain whitespace or an operator is not a nested command line; re-parsing it invented commands, and once a `&` made a word reproduce itself it also failed readable commands closed. Tests: `a_quoted_operator_stays_data` covers the four shapes from the review (`git commit -m "fix & feature"`, `echo "a & b"`, `echo 'x & rm -rf /etc'`, `printf 'a;b | c'`) and asserts one command stays one command. The nested `sh -c` detection the previous change relies on is still covered by `command_invocations_reach_past_wrapper_options_and_into_shell_payloads`, including its depth-cap assertion. `cargo test -p codewhale-execpolicy command_safety` -> 88 passed; 0 failed.
Adversarial review — follow-up commit
|
| command | d37431a argv (the F1 phantom) |
ea9bc1b argv |
result |
|---|---|---|---|
echo "a & b" |
["echo","a"],["a"],["b"] |
["echo","a & b"] |
phantom gone |
echo "a&b" |
["echo","a"],["a"],["b"] |
["echo","a&b"] |
phantom gone |
echo 'x & rm -rf /etc' |
["rm","-rf","/etc"] present |
no rm argv |
phantom rm gone |
git commit -m "fix & feature" |
["git","commit","-m","fix"], phantom feature |
["git","commit","-m","fix & feature"] |
one command |
No phantom rm survives in command_invocations, and no stage the shell
actually runs is missing from the argv list.
Nuance (not a defect): the user-visible
analyze_commandverdict for
echo 'x & rm -rf /etc'is still dangerous —command_safety.rs:1821
DANGEROUS_PATTERNS(rm -rf /) is quote-blind and fires on the literal
text. That is pre-existing (unchanged frommain) and intentional
hardening (the "keep that protection" comment at:1814); it over-reads,
never under-reads. The F1 issue — the inventedrmargv at the destroyer
hold — is gone even though the substring fallback still classifies the
quoted string dangerous. If the goal were "a benign string quotingrm -rf /etcis never refused", that remains unmet, but it is not this edit's
omission.
2. CONFIRMED — narrowed recursion loses no real command; every dropped re-parse is a phantom.
Ran the battery under both d37431a and ea9bc1b. For the -e interpreters
(perl -e 'system("rm -rf /etc")', ruby -e 'system "rm", "-rf", "/etc"',
node -e '...execSync("rm -rf /etc")', awk 'BEGIN { system("rm -rf /etc") }')
and python3 -c "import os; os.system('rm -rf /etc')", the old code re-parsed
the payload and produced an rm argv whose argv[0] was "system(rm", "rm,",
"require(...)execsync(rm", or "os.system('rm" — none equal "rm", so
none ever matched auto_review.rs:1580 argv_is_destroyer. Those were phantom
non-holds already; the new code drops them, and the real destructive text is
still caught by the substring fallback → Dangerous in both old and new.
No destructive stage is lost.
The genuinely-running shell payloads — sh -c 'rm -rf /home',
bash -lc 'cd /; rm -rf /tmp', bash -- -c 'rm -rf /boot',
/bin/sh -c 'rm -rf /var', find | xargs [ -I{} ] sh -c … — are still
re-parsed (previous word -c / -lc / -ic passes
is_shell_script_argument), yield the exact ["rm","-rf",…] argv, and are
dangerous. Verified argv-for-argv.
sh -c 'true' -- 'rm -rf /etc' and sh --command 'rm -rf /etc': old code
re-parsed and raised a phantom; new drops it. That is correct — in
sh -c 'true' -- 'rm -rf /etc' the rm word is $1 of the true script,
never executed, and --command is not a real option. Both were over-reads.
xargs -I{} sh -c 'rm -rf {}' still surfaces ["rm","-rf","{}"]. The
command_word-folded forms (/bin/sh, \rm, /usr/bin/rm) and the
find | xargs … sh -c '/etc' chain behave identically old vs new. This
category yielded no false negative.
3. CONFIRMED — one pre-existing residual: the scanner does not honor backslash escapes, so \" inside double quotes can still invent a phantom rm.
echo "a \" & rm -rf /etc": the char scanner at 2017 treats the \" as
closing the double quote (no escape awareness), so & rm -rf /etc lands
outside a quote and the & splits → a phantom ["rm","-rf","/etc"] argv the
shell never runs. This is identical in d37431a and ea9bc1b and redundant
with the Dangerous already raised; it is not introduced or reopened by
this commit, but the fix's quote-tracking still is not a shell lexer. The
opposite failure — quote state closing early and thereby hiding a real
separator — did not surface: echo "a&b" & rm -rf /etc (quoted op followed by
real) keeps the quoted & as data and still splits the real one, finding the
rm; echo "&" ; rm behaves the same; consecutive single quotes
(echo 'it'\''s & x') stay data with no phantom; and an unterminated double
quote (echo "x & rm -rf /etc) keeps & inside the never-closed quote,
matching the shell's non-execution of that rm. Effect: exactly one,
pre-existing, over-read-only finding in this category.
4. CONFIRMED — Windows floor & the PR's original fix still hold.
command_invocations returning None still arms UnclassifiedWindowsInvocation
(auto_review.rs:1359). git log 2>&1 and make >build.log 2>&1 | tail -3
both stay Some (classifiable) here. The depth-cap path
command_invocations_reach_past_wrapper_options_and_into_shell_payloads still
asserts a too-deep nested sh -c returns None, and the suite passes it.
The PR's own redirects_stay_classifiable_and_a_lone_ampersand_still_separates
still covers echo x&rm -rf /etc (un-spaced real &) and tool &> out.txt.
Nothing new in this category.
5. CONFIRMED — wrapper-option reach-in (sudo -u, command_word folding, --, xargs) is preserved.
sudo -u me rm -rf /etc, \rm -rf /etc, /usr/bin/rm -rf /etc, and
find | xargs [ -I{} ] sh -c/bash -c 'rm -rf /etc' all still yield the
rm argv and are dangerous. This is the original PR's core guarantee and
none of it regresses.
Bottom line: the follow-up resolves F1 in exactly the surface it touched
and I could not find a genuine command it now misses that the old code caught —
the dropped re-parses are phantom rms, substring-fallback redundancy, or
system()-payload gaps identical before and after the change. Merge-able.
Categories 2 and 4 produced no confirmed defect.
Attribution
🤖 Generated by SpikeBot 003(ClaudeCode-JP)
1 participant
Summary
Every shell command containing a
2>&1redirect was refused on Windows with:The refusal reason names npm launchers, but no launcher check ever ran. The
command was simply unclassifiable, and the reason text is a canned
fail-closed string. Two lines in the shared classifier made a plain redirect
unreadable:
crates/execpolicy/src/command_safety.rs,split_command_segments, kept a bare&inside the word ('&' => current.push(ch)), andcommand_invocationsre-parsed any word containing
&. Since the segmenter never separates a bare&, that re-parse returned the identical word, recursed again, and hitMAX_WRAPPER_DEPTH.command_invocationsthen returnedNone— "fail closed"— and
windows_session_runtime_riskturned that intoUnclassifiedWindowsInvocation, which the runtime floor converts into anon-bypassable block (
auto_review.rs:1364).So: the safety gate was not too strict about launchers. It could not read the
command at all.
Reproduction (before this change)
In a Full Access session with the sandbox disabled, on Windows:
cargo fmt --all -- --check 2>&1 | Select-Object -First 30git fetch origin 2>&1; git log --oneline -1 origin/main& $GH auth status(PowerShell's call operator — a bare&)git grep -n "2>&1" -- crates/(the literal text inside quotes)&removedThe last two lines are the tell: the classifier never gets to decide what the
command means, because a bare
&makes it stop reading.Changes
split_command_segments: a&that is a command separator now splitsthe segment. POSIX job control and cmd.exe both treat it that way. The
&ofa redirection stays inside the word:
>&copies a file descriptor (2>&1,>&2) and&>is bash's combined redirect.command_invocations: re-parse a word only when re-parsing can yield adifferent token stream — whitespace,
;, or|. A&that survivedsegmenting is part of a redirect; recursing on it could only reproduce the
same word.
The second change alone would have been a weakening: a bare
&glued toits neighbours (
echo x&rm -rf /etc) names two commands, and left unsplit itcorrupts the command word of the second one. Splitting it is the fix; dropping
the recursion is what makes the redirect readable.
Evidence
redirects_stay_classifiable_and_a_lone_ampersand_still_separates(new, incrates/execpolicy/src/command_safety.rs) asserts both directions:git log 2>&1,cargo test 2>&1 | tail -5,make > build.log 2>&1,tool &> out.txtstay classifiable.echo x & rm -rf /etc,echo x&rm -rf /etc,curl https://example.com & rm -rf /etcstill yield anrminvocation, sothe second stage is reached rather than blanket-refused.
Run against the previous code, that test fails with exactly the reported
symptom:
With the change:
cargo test -p codewhale-execpolicy command_safety→87 passed; 0 failed.
TUI side:
cargo test -p codewhale-tui auto_review→ 72 passed; 0 failed.test_cases_09.rsused to assert the Windows floor as the expected outcomefor
cargo test & curl https://example.com; that assertion is removed becausethe command is classifiable and belongs on the reviewer path like every other
approval-shaped command. The remaining
windows_*tests in that module passunchanged.
cargo fmt --all -- --checkis clean.Note on a flaky-looking neighbour
core::engine::tests::auto_review_asks_the_user_and_returns_the_answeroverflowed its stack on this machine. It is unrelated to this change (the diff
only removes recursion) and it is a stack-size limit, not a logic failure:
with
RUST_MIN_STACK=33554432it passes, and it passes as part of the 72. Notfixed here — out of scope, but worth a separate look.
Type of Change
Checklist
Related Issues
No-Issue: observed in an operator session on 0.10.1 (Windows/PowerShell); no
upstream issue filed for it.
Attribution
🤖 Generated by SpikeBot 000(CodeWhale-LOCAL)