diff --git a/crates/jcode-command-risk/src/assess_tests.rs b/crates/jcode-command-risk/src/assess_tests.rs index 3d17b20eb8f..02902d995c0 100644 --- a/crates/jcode-command-risk/src/assess_tests.rs +++ b/crates/jcode-command-risk/src/assess_tests.rs @@ -723,3 +723,88 @@ fn issue_1753_inert_text_stays_safe() { assert!(level(command).runs_immediately(), "{command:?}"); } } + +#[test] +fn issue_932_a_leading_cd_moves_where_relative_targets_resolve() { + // A relative delete after `cd` resolves against the directory the shell is + // actually in, not the tool call's working directory. Without the tracking + // these looked like ordinary deletes inside the workspace. + assert_eq!(level("cd /etc && rm -rf passwd"), RiskLevel::Catastrophic); + assert_eq!(level("cd /etc; rm -rf passwd"), RiskLevel::Catastrophic); + assert_eq!( + level("cd / && cd etc && rm -rf passwd"), + RiskLevel::Catastrophic + ); + assert_eq!(level("cd ~/.ssh && rm -rf *"), RiskLevel::Catastrophic); + // `cd` with no operand goes home. + assert_eq!(level("cd && rm -rf Documents"), RiskLevel::Catastrophic); + + // A temp-directory cleanup must not be attributed to the workspace. + let assessment = assess("cd /tmp && rm -rf build-xyz", &ctx()); + assert!( + assessment + .findings + .iter() + .all(|finding| finding.target.as_deref() != Some("/home/u/proj/build-xyz")), + "{:?}", + assessment.findings + ); + assert!( + assessment.level.runs_immediately(), + "{:?}", + assessment.findings + ); + + // A destination that cannot be resolved statically is not assumed to be + // the workspace either. + let assessment = assess("cd $UNKNOWN_DIR && rm -rf build-xyz", &ctx()); + assert!( + assessment + .findings + .iter() + .all(|finding| finding.target.as_deref() != Some("/home/u/proj/build-xyz")), + "{:?}", + assessment.findings + ); +} + +#[test] +fn issue_932_redirect_syntax_is_not_a_delete_target() { + let bare = assess("rm -rf build-xyz", &ctx()); + for command in [ + "rm -rf build-xyz 2>/dev/null", + "rm -rf build-xyz 2> /dev/null", + "rm -rf build-xyz >/dev/null 2>&1", + "rm -rf build-xyz 2>>/dev/null", + "rm -rf build-xyz &>/dev/null", + "rm -rf build-xyz < /dev/null", + ] { + let assessment = assess(command, &ctx()); + assert_eq!( + assessment.level, bare.level, + "{command:?}: {:?}", + assessment.findings + ); + // Neither the descriptor number nor the destination is an operand. + assert!( + assessment + .findings + .iter() + .all(|finding| finding.target.as_deref() != Some("/home/u/proj/2")), + "{command:?}: {:?}", + assessment.findings + ); + assert!( + assessment.findings.len() <= bare.findings.len(), + "{command:?}: {:?}", + assessment.findings + ); + } + + // A space between the number and the operator makes it a real operand, + // which the shell would pass to the command. + assert_eq!(level("rm -rf build-xyz 2 >/dev/null"), RiskLevel::Low); + + // Truncating a protected path through a redirect is still catastrophic. + assert_eq!(level("echo pwned > /etc/passwd"), RiskLevel::Catastrophic); +} diff --git a/crates/jcode-command-risk/src/lib.rs b/crates/jcode-command-risk/src/lib.rs index 2a3b83ad807..3e0acca105c 100644 --- a/crates/jcode-command-risk/src/lib.rs +++ b/crates/jcode-command-risk/src/lib.rs @@ -214,8 +214,28 @@ pub fn assess(command: &str, ctx: &RiskContext) -> RiskAssessment { }); } + // A `cd` earlier in the same command line decides where later relative + // paths resolve (#932). Without that, `cd /etc && rm -rf passwd` was judged + // as a delete inside the workspace while the shell would remove + // `/etc/passwd`. Subshell parentheses are flattened away by the tokenizer, + // so a `cd` inside `( ... )` over-approximates the rest of the line; that + // direction only ever costs a confirmation. + let mut cwd = ctx.working_dir.clone(); for segment in tokenize::split_segments(command) { - assess_segment(&segment, ctx, &mut findings); + let segment_ctx = RiskContext { + working_dir: cwd.clone(), + ..ctx.clone() + }; + match directory_change(&segment, &segment_ctx) { + DirectoryChange::None => {} + DirectoryChange::Known(dir) => cwd = Some(dir), + DirectoryChange::Unknown => cwd = None, + } + let segment_ctx = RiskContext { + working_dir: cwd.clone(), + ..ctx.clone() + }; + assess_segment(&segment, &segment_ctx, &mut findings); } // `"$(...)"`, backticks and unquoted heredoc bodies run commands the @@ -235,6 +255,67 @@ pub fn assess(command: &str, ctx: &RiskContext) -> RiskAssessment { RiskAssessment::from_findings(findings) } +/// What a segment does to the shell's working directory. +enum DirectoryChange { + /// Not a `cd`. + None, + /// A `cd` whose destination can be resolved statically. + Known(std::path::PathBuf), + /// A `cd` whose destination depends on runtime state (`cd -`, `cd $VAR`). + Unknown, +} + +/// Track `cd` so later relative operands resolve where the shell will be. +fn directory_change(tokens: &[Token], ctx: &RiskContext) -> DirectoryChange { + let mut rest = tokens; + while rest.len() > 1 && SHELL_CONTROL_PREFIXES.contains(&rest[0].text.as_str()) { + rest = &rest[1..]; + } + // Leading `VAR=value` assignments prefix a command without changing it. + while rest.len() > 1 && rest[0].text.contains('=') && !rest[0].is_flag() { + rest = &rest[1..]; + } + if rest.first().map(Token::basename).as_deref() != Some("cd") { + return DirectoryChange::None; + } + + let mut destination: Option<&str> = None; + for token in rest.iter().skip(1) { + if token.is_operator || token.is_redirect_target { + continue; + } + // `cd -P /x`, `cd -- /x`: options precede the destination. + if destination.is_none() && (token.is_flag() || token.text == "--") { + continue; + } + if destination.is_some() { + // `cd a b` is an error in every shell, so the directory did not + // change in a way worth guessing at. + return DirectoryChange::Unknown; + } + destination = Some(token.text.as_str()); + } + + let Some(destination) = destination else { + // A bare `cd` goes home. + return match &ctx.home_dir { + Some(home) => DirectoryChange::Known(home.clone()), + None => DirectoryChange::Unknown, + }; + }; + // `cd -` uses OLDPWD, and substitutions or globs need the shell's own + // expansion before the directory is known. + if destination == "-" || destination.contains(['$', '`', '*', '?']) { + return DirectoryChange::Unknown; + } + let resolved = paths::expand(destination, ctx); + if resolved.is_absolute() { + DirectoryChange::Known(resolved) + } else { + DirectoryChange::Unknown + } +} + fn assess_segment(tokens: &[Token], ctx: &RiskContext, findings: &mut Vec) { // Shell redirects happen even when a wrapper only prints information. for target in tokens.iter().filter(|t| t.is_truncating_redirect_target) { @@ -389,7 +470,7 @@ fn assess_segment(tokens: &[Token], ctx: &RiskContext, findings: &mut Vec = tokens .iter() .skip(1) - .filter(|t| !t.is_flag() && !t.is_operator && !t.is_truncating_redirect_target) + .filter(|t| !t.is_flag() && !t.is_operator && !t.is_redirect_target) .collect(); // A destructive command fed by a pipe takes its operands from the previous @@ -449,7 +530,7 @@ fn assess_find(tokens: &[Token], ctx: &RiskContext, findings: &mut Vec| { for raw in roots diff --git a/crates/jcode-command-risk/src/tokenize.rs b/crates/jcode-command-risk/src/tokenize.rs index 173753f38b5..243c02d3222 100644 --- a/crates/jcode-command-risk/src/tokenize.rs +++ b/crates/jcode-command-risk/src/tokenize.rs @@ -14,6 +14,11 @@ pub struct Token { pub receives_pipe: bool, /// True for `>` / `>|` redirect destinations, which are truncated on open. pub is_truncating_redirect_target: bool, + /// True for every redirect destination (`>`, `>>`, `<`, `&>`). These are + /// never operands of the command, so they are excluded from target + /// collection; only the truncating subset is also classified as a write + /// (#932: `rm -rf x 2>/dev/null` reported `/dev/null` as a delete target). + pub is_redirect_target: bool, /// True for control operators like `&&`, which are never path targets. pub is_operator: bool, } @@ -24,6 +29,7 @@ impl Token { text: text.into(), receives_pipe: false, is_truncating_redirect_target: false, + is_redirect_target: false, is_operator: false, } } @@ -99,22 +105,40 @@ pub fn tokenize(command: &str) -> Vec { let mut has_content = false; let mut chars = command.chars().peekable(); let mut pending_redirect = false; + let mut pending_truncating = false; macro_rules! flush { - () => { + () => {{ if has_content { let mut token = Token::word(std::mem::take(&mut current)); + if pending_redirect { + token.is_redirect_target = true; + token.is_truncating_redirect_target = pending_truncating; + } #[allow(unused_assignments)] { - if pending_redirect { - token.is_truncating_redirect_target = true; - pending_redirect = false; - } has_content = false; } tokens.push(token); + #[allow(unused_assignments)] + { + pending_redirect = false; + pending_truncating = false; + } + } + }}; + } + + // A redirect operator with no destination word leaves nothing to mark, and + // the flag must not carry over across a command boundary to a later word. + macro_rules! clear_redirect { + () => {{ + #[allow(unused_assignments)] + { + pending_redirect = false; + pending_truncating = false; } - }; + }}; } while let Some(c) = chars.next() { @@ -154,12 +178,14 @@ pub fn tokenize(command: &str) -> Vec { ' ' | '\t' => flush!(), '\n' | ';' => { flush!(); + clear_redirect!(); let mut op = Token::word(c.to_string()); op.is_operator = true; tokens.push(op); } '&' | '|' => { flush!(); + clear_redirect!(); let mut text = c.to_string(); if chars.peek() == Some(&c) { text.push(c); @@ -171,16 +197,19 @@ pub fn tokenize(command: &str) -> Vec { } '(' | ')' => { flush!(); + clear_redirect!(); let mut op = Token::word(c.to_string()); op.is_operator = true; tokens.push(op); } '>' => { + drop_fd_prefix(&mut current, &mut has_content); flush!(); // `>>` appends and does not truncate, so it is far less // destructive; only a single `>` clobbers. if chars.peek() == Some(&'>') { chars.next(); + pending_redirect = true; } else if chars.peek() == Some(&'&') { // `>&N` / `>&-` duplicate or close a file descriptor and // never name a file to truncate. Consume the FD word. @@ -197,15 +226,21 @@ pub fn tokenize(command: &str) -> Vec { if fd.is_empty() { // `>& file` (bash: redirect stdout+stderr) truncates. pending_redirect = true; + pending_truncating = true; } } else { if chars.peek() == Some(&'|') { chars.next(); } pending_redirect = true; + pending_truncating = true; } } - '<' => flush!(), + '<' => { + drop_fd_prefix(&mut current, &mut has_content); + flush!(); + pending_redirect = true; + } _ => { has_content = true; current.push(c); @@ -217,6 +252,21 @@ pub fn tokenize(command: &str) -> Vec { tokens } +/// Drop a bare file-descriptor number that prefixes a redirect (`2>/dev/null`). +/// +/// The number is redirect syntax, not an operand, but it was emitted as an +/// ordinary word and then collected as a delete target: `rm -rf x 2>/dev/null` +/// reported a recursive delete of `.../2` (#932). A descriptor prefix has to be +/// adjacent to the operator, and a space flushes the word first, so only the +/// adjacent spelling reaches here. `rm -rf x 2 >/dev/null` really does name a +/// file called `2` and is left alone. +fn drop_fd_prefix(current: &mut String, has_content: &mut bool) { + if *has_content && current.chars().all(|c| c.is_ascii_digit()) { + current.clear(); + *has_content = false; + } +} + /// Remove heredoc payloads before tokenizing shell source. /// /// A heredoc body is input data for the command on the declaration line, not diff --git a/crates/jcode-command-risk/src/tokenize_tests.rs b/crates/jcode-command-risk/src/tokenize_tests.rs index 8e112109c96..0fbb50eb6a9 100644 --- a/crates/jcode-command-risk/src/tokenize_tests.rs +++ b/crates/jcode-command-risk/src/tokenize_tests.rs @@ -179,3 +179,42 @@ fn parentheses_split_subshell_and_command_substitution_bodies() { vec![vec!["x=$"], vec!["rm", "-rf", "~"], vec!["echo", "ok"]] ); } + +#[test] +fn redirect_destinations_are_marked_and_fd_prefixes_are_not_words() { + // Every destination is a redirect target, so it is never an operand. + for (command, destination) in [ + ("rm -rf x 2>/dev/null", "/dev/null"), + ("rm -rf x 2> /dev/null", "/dev/null"), + ("rm -rf x >> log.txt", "log.txt"), + ("rm -rf x < /dev/null", "/dev/null"), + ("rm -rf x &>/dev/null", "/dev/null"), + ] { + let tokens = tokenize(command); + assert!( + tokens + .iter() + .any(|t| t.is_redirect_target && t.text == destination), + "{command}: {tokens:?}" + ); + assert!( + !tokens.iter().any(|t| t.text == "2"), + "{command} keeps the fd prefix as a word: {tokens:?}" + ); + } + + // An input redirect is a destination but never truncates. + let input = tokenize("rm -rf x < /dev/null"); + assert!( + !input + .iter() + .any(|t| t.is_truncating_redirect_target && t.text == "/dev/null") + ); + + // A space before the operator makes the number a real argument. + assert!( + tokenize("rm -rf x 2 >/dev/null") + .iter() + .any(|t| t.text == "2" && !t.is_redirect_target) + ); +}