diff --git a/crates/execpolicy/src/command_safety.rs b/crates/execpolicy/src/command_safety.rs index 936e19d2a6..8dd8add393 100644 --- a/crates/execpolicy/src/command_safety.rs +++ b/crates/execpolicy/src/command_safety.rs @@ -2018,17 +2018,45 @@ 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 == '"' + && chars.peek().is_some_and(|next| matches!(next, '"' | '\\')) + { + current.push(chars.next().unwrap()); + } else 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)); } '|' | ';' => 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), } } @@ -2064,6 +2092,33 @@ pub fn is_literal_rm_invocation(command: &str) -> bool { .unwrap_or(false) } +/// 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 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..]) + .filter(|payload| std::ptr::eq(*payload, &words[index])) + .cloned(); + } + let flag = words[index - 1].to_ascii_lowercase(); + match name.as_str() { + "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()) + } + _ => None, + } + }) +} + /// Every argv that could run as a command somewhere in `command`: each stage /// (`;`, `&&`, `||`, `|`), started at *every* word of it — so no wrapper's /// options (`sudo -u me`, `timeout -s KILL 60`, `xargs -0`) can hide the @@ -2074,10 +2129,6 @@ 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. -/// -/// A descriptor redirect (`2>&1`) is not a command line: its `&` is part of -/// the redirect. Descending into it re-read the same word until the depth -/// limit, so every `npm test 2>&1` failed closed as a catastrophic command. pub fn command_invocations(command: &str) -> Option>> { fn collect(command: &str, depth: usize, out: &mut Vec>) -> bool { if depth > MAX_WRAPPER_DEPTH { @@ -2089,9 +2140,15 @@ 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 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 word.contains(|ch: char| ch.is_whitespace() || matches!(ch, ';' | '&' | '|')) - && !is_descriptor_duplication(word) - && !collect(word, depth + 1, out) + && let Some(script) = nested_shell_script(&words, index) + && !collect(&script, depth + 1, out) { return false; } @@ -2103,18 +2160,6 @@ pub fn command_invocations(command: &str) -> Option>> { collect(command, 0, &mut out).then_some(out) } -/// A whole word that only duplicates or closes a file descriptor: `2>&1`, -/// `>&2`, `1>&-`, `0<&3`. Nothing else qualifies: `&>file`, a trailing `&` and -/// `a&b` still fail closed in [`command_invocations`]. -fn is_descriptor_duplication(word: &str) -> bool { - let rest = word.trim_start_matches(|ch: char| ch.is_ascii_digit()); - rest.strip_prefix(">&") - .or_else(|| rest.strip_prefix("<&")) - .is_some_and(|target| { - target == "-" || (!target.is_empty() && target.bytes().all(|b| b.is_ascii_digit())) - }) -} - /// How many wrappers (`sudo env nice sh -c ...`) the classifier will peel /// before it refuses to reason further. /// @@ -2939,6 +2984,11 @@ mod tests { "timeout -s KILL 60 rm -rf /etc", "echo x | xargs rm -rf /etc", "bash -lc 'cd /; rm -rf /etc'", + "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", ] { @@ -2953,6 +3003,74 @@ mod tests { ); } + /// 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", + "npm test &", + "npm test &>/dev/null", + "tool 0<&3", + "& $cmd auth status", + ] { + 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", + r#"printf "\\";rm -rf /etc"#, + "npm test 2>&1&rm -rf /etc", + "curl https://example.com & rm -rf /etc", + ] { + 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}" + ); + } + } #[test] fn command_invocations_read_past_a_descriptor_redirect() { // `2>&1` is a redirect, not a nested command line. It used to recurse @@ -2983,17 +3101,6 @@ mod tests { "{command}" ); } - // Only a whole descriptor-duplication word is exempt. Other `&` - // spellings keep failing closed. - for command in [ - "npm test &", - "true&rm -rf /etc", - "npm test &>/dev/null", - "npm test 2>&1&rm -rf /etc", - "npm test 2>&file", - ] { - assert!(command_invocations(command).is_none(), "{command}"); - } } use super::*; 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 66e2c5bcab..2743c76ded 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}"); } }