From d37431a97439cbe1ec602ba640eda8a94c32db59 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Fri, 9 Oct 2026 01:38:45 +0800 Subject: [PATCH 1/4] fix(execpolicy): a redirect is not a command separator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/execpolicy/src/command_safety.rs | 53 ++++++++++++++++++- .../src/core/engine/tests/test_cases_09.rs | 19 ++----- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/crates/execpolicy/src/command_safety.rs b/crates/execpolicy/src/command_safety.rs index ff0a160635..ae025587eb 100644 --- a/crates/execpolicy/src/command_safety.rs +++ b/crates/execpolicy/src/command_safety.rs @@ -2028,7 +2028,12 @@ fn split_command_segments(command: &str) -> Vec { segments.push(std::mem::take(&mut current)); } '|' | ';' => segments.push(std::mem::take(&mut current)), - '&' => current.push(ch), + // `>&` copies a file descriptor (`2>&1`, `>&2`) and `&>` is bash's + // combined redirect; both keep the `&` inside the word so the + // redirect survives as one token. Any other `&` separates commands + // — on POSIX shells and on cmd.exe alike. + '&' if current.ends_with('>') || chars.peek() == Some(&'>') => current.push(ch), + '&' => segments.push(std::mem::take(&mut current)), _ => current.push(ch), } } @@ -2085,7 +2090,13 @@ pub fn command_invocations(command: &str) -> Option>> { let mut argv = words[index..].to_vec(); argv[0] = command_word(word); out.push(argv); - if word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '&' | '|')) + // Re-parse a word only when re-parsing can yield a different + // token stream: whitespace separates words, `;` and `|` + // separate segments. A `&` that survived segmenting is part of + // a redirect (`2>&1`), and re-parsing it reproduces the same + // single word until the depth cap — which failed perfectly + // readable commands closed as unclassifiable (`git log 2>&1`). + if word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '|')) && !collect(word, depth + 1, out) { return false; @@ -2935,6 +2946,44 @@ mod tests { "too deep fails closed" ); } + + /// A redirect is a token inside one command, not a command separator, and + /// it must not fail the classifier closed. A `&` inside `2>&1` used to be + /// re-parsed as a nested command line, which reproduced the same word until + /// `MAX_WRAPPER_DEPTH` blew and every command carrying a redirect came back + /// unclassified. The Windows runtime floor turns that into a hard block + /// (reported 2026-10-09: `2>&1` could not run at all). + #[test] + fn redirects_stay_classifiable_and_a_lone_ampersand_still_separates() { + for command in [ + "git log 2>&1", + "cargo test 2>&1 | tail -5", + "make > build.log 2>&1", + "tool &> out.txt", + ] { + assert!( + command_invocations(command).is_some(), + "a redirect must stay readable instead of failing closed: {command}" + ); + } + + // A lone `&` separates commands, so a destructive second stage stays + // reachable whether or not the author spaced it out. Blanket-refusing + // these was how the redirect bug stayed invisible. + let has_rm = |command: &str| { + command_invocations(command) + .expect("readable") + .iter() + .any(|argv| argv[0] == "rm" && argv.iter().any(|arg| arg == "/etc")) + }; + for command in [ + "echo x & rm -rf /etc", + "echo x&rm -rf /etc", + "curl https://example.com & rm -rf /etc", + ] { + assert!(has_rm(command), "second stage must be reached: {command}"); + } + } use super::*; #[test] diff --git a/crates/tui/src/core/engine/tests/test_cases_09.rs b/crates/tui/src/core/engine/tests/test_cases_09.rs index 0ab57f0b0c..ac5bdeed82 100644 --- a/crates/tui/src/core/engine/tests/test_cases_09.rs +++ b/crates/tui/src/core/engine/tests/test_cases_09.rs @@ -471,21 +471,10 @@ fn auto_review_routes_shell_commands_requiring_approval_to_reviewer() { None, ); - if cfg!(windows) && command == "cargo test & curl https://example.com" { - // Unclassified Windows input hits the built-in floor before the reviewer. - assert_eq!( - decision, - AutoReviewPlanDecision::Block( - "Built-in safety gate requires approval: Windows command input cannot be classified safely enough to exclude termination of Codewhale npm launchers; use a direct PID- or port-specific command".into() - ) - ); - assert_eq!(audit["decision"], "hold_for_review"); - } else { - assert!( - matches!(decision, AutoReviewPlanDecision::ConsultReviewer(_)), - "Auto-Review must not auto-approve {command} without reviewer judgment" - ); - } + assert!( + matches!(decision, AutoReviewPlanDecision::ConsultReviewer(_)), + "Auto-Review must not auto-approve {command} without reviewer judgment" + ); assert_ne!(audit["decision"], "allow", "unexpected allow for {command}"); } } From ea9bc1b28433f857599397eb25b5bff9f6ff2098 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Fri, 9 Oct 2026 02:23:36 +0800 Subject: [PATCH 2/4] fix(execpolicy): a quoted operator is data, not a separator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/execpolicy/src/command_safety.rs | 80 ++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 7 deletions(-) diff --git a/crates/execpolicy/src/command_safety.rs b/crates/execpolicy/src/command_safety.rs index ae025587eb..72c123326b 100644 --- a/crates/execpolicy/src/command_safety.rs +++ b/crates/execpolicy/src/command_safety.rs @@ -2018,11 +2018,29 @@ fn split_command_segments(command: &str) -> Vec { // Char-based, not byte-indexed: commands carry non-ASCII paths and slicing // a multibyte character in half panics. `&&` and `||` are consumed as one // unit so `||` cannot leave a stray `|` behind to split again. + // + // Quotes are tracked so an operator inside them stays data — the same + // promise the read-only grammar makes (`rg 'a && b; c' src`). Splitting + // regardless of quoting held `git commit -m "fix & feature"` as though it + // ran two commands, and invented an `rm` for `echo 'x & rm -rf /etc'` that + // the shell never runs. let mut segments = Vec::new(); let mut current = String::new(); let mut chars = command.chars().peekable(); + let mut quote: Option = None; while let Some(ch) = chars.next() { + if let Some(active) = quote { + current.push(ch); + if ch == active { + quote = None; + } + continue; + } match ch { + '\'' | '"' => { + quote = Some(ch); + current.push(ch); + } '&' | '|' if chars.peek() == Some(&ch) => { chars.next(); segments.push(std::mem::take(&mut current)); @@ -2079,6 +2097,28 @@ pub fn is_literal_rm_invocation(command: &str) -> bool { /// Deliberately over-inclusive (`echo rm -rf /etc` yields an `rm` argv too): /// callers use it to *hold* catastrophic commands, never to allow anything. /// `None` when words nest deeper than `MAX_WRAPPER_DEPTH`: fail closed. +/// Whether `words[index]` is the script body a shell was asked to run — the +/// argument after `sh -c`, `bash -lc`, and friends. +/// +/// Only that position is a nested command line. An ordinary quoted argument +/// that merely contains an operator (`echo 'x & rm -rf /etc'`) is data, and +/// re-parsing it invents commands the shell never runs. +fn is_shell_script_argument(words: &[String], index: usize) -> bool { + let Some(flag) = index + .checked_sub(1) + .and_then(|previous| words.get(previous)) + else { + return false; + }; + let Some(letters) = flag.strip_prefix('-') else { + return false; + }; + !letters.is_empty() + && !letters.starts_with('-') + && letters.chars().all(|ch| ch.is_ascii_alphabetic()) + && letters.contains('c') +} + pub fn command_invocations(command: &str) -> Option>> { fn collect(command: &str, depth: usize, out: &mut Vec>) -> bool { if depth > MAX_WRAPPER_DEPTH { @@ -2090,13 +2130,14 @@ pub fn command_invocations(command: &str) -> Option>> { let mut argv = words[index..].to_vec(); argv[0] = command_word(word); out.push(argv); - // Re-parse a word only when re-parsing can yield a different - // token stream: whitespace separates words, `;` and `|` - // separate segments. A `&` that survived segmenting is part of - // a redirect (`2>&1`), and re-parsing it reproduces the same - // single word until the depth cap — which failed perfectly - // readable commands closed as unclassifiable (`git log 2>&1`). - if word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '|')) + // Re-parse a word only when it can be a command line of its + // own: the script body of `sh -c`/`bash -lc`, where nested + // whitespace, `;`, and `|` are real separators. Anything else + // is an argument — `echo 'x & rm -rf /etc'` names no `rm`, and + // re-parsing it both invented one and failed readable commands + // closed once a `&` made the word reproduce itself. + if is_shell_script_argument(&words, index) + && word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '|')) && !collect(word, depth + 1, out) { return false; @@ -2984,6 +3025,31 @@ mod tests { assert!(has_rm(command), "second stage must be reached: {command}"); } } + + /// A quoted operator is data, which the read-only grammar already promises + /// (`rg 'a && b; c' src`). Splitting regardless of quoting held + /// `git commit -m "fix & feature"` as though it ran two commands, and + /// invented an `rm` for `echo 'x & rm -rf /etc'` that the shell never runs. + #[test] + fn a_quoted_operator_stays_data() { + for command in [ + r#"git commit -m "fix & feature""#, + r#"echo "a & b""#, + "echo 'x & rm -rf /etc'", + r#"printf 'a;b | c'"#, + ] { + let invocations = command_invocations(command).expect("readable"); + assert!( + !invocations.iter().any(|argv| argv[0] == "rm"), + "a quoted `rm` is data, not a command: {command}" + ); + assert_eq!( + invocations.iter().filter(|argv| argv[0] == "git").count(), + usize::from(command.starts_with("git")), + "quoting must not split one command into two: {command}" + ); + } + } use super::*; #[test] From 0b5042a4433f76b8c97ba7e2f310293a92dcfd49 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Mon, 12 Oct 2026 23:15:43 -0700 Subject: [PATCH 3/4] fix(execpolicy): preserve separators after escaped backslashes 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 eb817dbf87 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 #6929 Refs #6963 --- crates/execpolicy/src/command_safety.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/execpolicy/src/command_safety.rs b/crates/execpolicy/src/command_safety.rs index a76dc4680a..0a56d24072 100644 --- a/crates/execpolicy/src/command_safety.rs +++ b/crates/execpolicy/src/command_safety.rs @@ -2031,7 +2031,10 @@ fn split_command_segments(command: &str) -> Vec { while let Some(ch) = chars.next() { if let Some(active) = quote { current.push(ch); - if ch == '\\' && active == '"' && chars.peek() == Some(&'"') { + if ch == '\\' + && active == '"' + && chars.peek().is_some_and(|next| matches!(next, '"' | '\\')) + { current.push(chars.next().unwrap()); } else if ch == active { quote = None; @@ -3032,6 +3035,7 @@ mod tests { for command in [ "echo x & rm -rf /etc", "echo x&rm -rf /etc", + r#"printf "\\";rm -rf /etc"#, "npm test 2>&1&rm -rf /etc", "curl https://example.com & rm -rf /etc", ] { From 454b607bdf34229df76fbd35721c808ee4e4b6c7 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Tue, 13 Oct 2026 00:54:43 -0700 Subject: [PATCH 4/4] fix(execpolicy): keep cmd apostrophes from hiding later commands 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 #6963. Follow-up review on contributor PR #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. --- crates/execpolicy/src/command_safety.rs | 26 ++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/crates/execpolicy/src/command_safety.rs b/crates/execpolicy/src/command_safety.rs index 0a56d24072..8dd8add393 100644 --- a/crates/execpolicy/src/command_safety.rs +++ b/crates/execpolicy/src/command_safety.rs @@ -2092,26 +2092,29 @@ pub fn is_literal_rm_invocation(command: &str) -> bool { .unwrap_or(false) } -/// Whether `words[index]` is the script body a shell was asked to run — the +/// Extract the script body a shell was asked to run — the /// argument after `sh -c`, `bash -lc`, and friends. /// /// Only that position is a nested command line. An ordinary quoted argument /// that merely contains an operator (`echo 'x & rm -rf /etc'`) is data, and /// re-parsing it invents commands the shell never runs. -fn is_shell_script_argument(words: &[String], index: usize) -> bool { - words[..index].iter().enumerate().any(|(start, word)| { +fn nested_shell_script(words: &[String], index: usize) -> Option { + words[..index].iter().enumerate().find_map(|(start, word)| { let name = command_word(word); if SHELL_WRAPPERS.contains(&name.as_str()) { return shell_command_payload(&words[start + 1..]) - .is_some_and(|payload| std::ptr::eq(payload, &words[index])); + .filter(|payload| std::ptr::eq(*payload, &words[index])) + .cloned(); } let flag = words[index - 1].to_ascii_lowercase(); match name.as_str() { - "cmd" => matches!(flag.as_str(), "/c" | "/k"), - "powershell" | "pwsh" => { - flag == "-c" || flag == "-command" || flag == "-commandwithargs" + "cmd" if matches!(flag.as_str(), "/c" | "/k") => Some(words[index].replace('\'', "")), + "powershell" | "pwsh" + if flag == "-c" || flag == "-command" || flag == "-commandwithargs" => + { + Some(words[index].clone()) } - _ => false, + _ => None, } }) } @@ -2143,9 +2146,9 @@ pub fn command_invocations(command: &str) -> Option>> { // is an argument — `echo 'x & rm -rf /etc'` names no `rm`, and // re-parsing it both invented one and failed readable commands // closed once a `&` made the word reproduce itself. - if is_shell_script_argument(&words, index) - && word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '&' | '|')) - && !collect(word, depth + 1, out) + if word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '&' | '|')) + && let Some(script) = nested_shell_script(&words, index) + && !collect(&script, depth + 1, out) { return false; } @@ -2984,6 +2987,7 @@ mod tests { "bash -c -e 'rm -rf /etc'", "sh -c 'true&rm -rf /etc'", "cmd /C \"echo x&rm -rf /etc\"", + "cmd /C \"echo 'x & rm -rf /etc & echo '\"", "powershell -Command 'echo x; rm -rf /etc'", "\\rm -rf /etc", "/usr/bin/rm -rf /etc",