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/2] 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/2] 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]