Repository navigation
fix(execpolicy): a redirect is not a command separator - #9
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)
… nested-shell detection Resolve command_safety conflicts against origin/main 008d21d without rewriting SparkofSpike's commits. Preserve quoted ampersands as data, recognize redirections and separators, reuse the existing shell payload parser for option-bearing shell wrappers, and retain cmd /C and PowerShell -Command payload inspection. Remove the obsolete descriptor-only helper. Keep the contributor's regression tests and main's redirect coverage; update the existing expectations for the newly classifiable syntax. Verification on macOS, native shared build lock and hermetic test home: - scripts/dev-test.sh execpolicy command_safety: 89 tests run: 89 passed, 147 skipped (filter exclusions). - scripts/dev-test.sh tui auto_review: 79 tests run: 79 passed, 15232 skipped (filter exclusions). - npm test: 1312 passed; 0 failed; 7 skipped. - npm run check:web: passed, including latest-release check and build. - cargo fmt --all -- --check and git diff --check: passed. - clippy -p codewhale-execpolicy --all-targets --locked -D warnings: passed. Windows execution and hosted Windows CI on this merge are still pending. No release, tag, or main push is part of this commit. Refs codewhale-hq#6929, codewhale-hq#6963
Review of the contributor conflict resolution found that a pair of backslashes before a closing double quote consumed that quote as escaped. Consume the escaped backslash as a pair, so the following closing quote ends its string and a glued command separator remains visible. Extended the contributor's existing ampersand regression test; no new test function or code comment was added. macOS verification: 89 tests run: 89 passed, 147 skipped (filter exclusions). Focused all-targets execpolicy Clippy, formatting and diff whitespace pass. Unchanged web/npm source retains the immediately preceding eb817db gate: npm test 1312 passed; 0 failed; 7 skipped, and npm run check:web passed. Windows execution remains pending hosted CI; no Windows proof claimed. Refs codewhale-hq#6929 Refs codewhale-hq#6963
cmd.exe treats apostrophes as literal characters, so its /C or /K payload can execute an ampersand-separated command inside apostrophes. The shared POSIX-oriented scanner previously grouped that text and missed the later invocation. Normalize literal apostrophes only in the explicit cmd payload before the existing bounded nested scan. Keep ordinary quoted arguments and other shell payloads on their existing path. Extend the existing wrapper case table; no new test function or code comment. Refs codewhale-hq#6963. Follow-up review on contributor PR codewhale-hq#6929, retained as its own PR. macOS: CODEWHALE_DEV_NEXTEST=0 CARGO_BUILD_JOBS=1 scripts/dev-test.sh execpolicy command_safety test result: ok. 89 passed; 0 failed; 0 ignored; 0 measured; 147 filtered out; finished in 0.01s Focused all-target Clippy, fmt and git diff --check passed. The first fmt check requested only match-arm formatting, applied before commit. Reused unchanged web/npm gate: 1312 passed; 0 failed; 7 skipped; check:web passed. Windows 11 ARM64: a harmless cmd /D /S /C echo probe emitted the second-stage marker between literal apostrophes. This proves shell semantics only; the new Rust classifier has not yet run on Windows. Hosted Windows CI is required. The prior PR head's Windows job passed 18299 tests, including owner_crash_reentry; that receipt does not qualify this changed head. No native-app, package or Windows 10 1607 acceptance is claimed.
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)