diff --git a/crates/codegen/kigi-workspace/src/permission/auto_mode.rs b/crates/codegen/kigi-workspace/src/permission/auto_mode.rs index 86968d6..5c54eff 100644 --- a/crates/codegen/kigi-workspace/src/permission/auto_mode.rs +++ b/crates/codegen/kigi-workspace/src/permission/auto_mode.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use tree_sitter::Node; use super::bash_command_splitting::{ - PlainCommand, is_wrapper_command, strip_wrapper_command, try_parse_shell, + PlainCommand, is_wrapper_command, matches_prefix_words, strip_wrapper_command, try_parse_shell, try_parse_word_only_commands_sequence, unwrap_wrappers, }; use super::shell_access::{command_words_write_paths, command_write_paths_in_tree}; @@ -424,7 +424,11 @@ fn classify_bash(cmd: &str) -> ClassifierVerdict { // (`cargo`/`make`/`pytest`/`python`/`node`, `npm test`/`run`, `uv run // `) execute project-controlled code; this heuristic is a fail-closed // FALLBACK and the real safety boundary is the LLM side-query + managed policy. - if !cmds.is_empty() && cmds.iter().all(|c| bash_command_is_routine(c.words())) { + if !cmds.is_empty() + && cmds + .iter() + .all(|c| bash_command_is_routine(c.words(), c.expandable())) + { return ClassifierVerdict::Allow; } ClassifierVerdict::Block @@ -438,10 +442,11 @@ fn classify_bash(cmd: &str) -> ClassifierVerdict { /// classified by [`package_manager_subcommand_is_routine`]: a fail-closed /// SAFE-subcommand allowlist (build/test/dep-management Allow; explicit launchers /// re-classified; remote / arbitrary-exec / unknown → Block). -fn bash_command_is_routine(words: &[String]) -> bool { +fn bash_command_is_routine(words: &[String], expandable: &[bool]) -> bool { // Peel canonical (quote-aware) wrappers: env [NAME=VALUE], timeout, nice, // stdbuf, ionice, chrt (incl. path-qualified). let inner = unwrap_wrappers(words); + let inner_expandable = &expandable[words.len() - inner.len()..]; // A bare wrapper (e.g. `env` printing the environment) or a command that was // only env assignments → routine. if inner.is_empty() || is_lone_wrapper(inner) { @@ -454,7 +459,7 @@ fn bash_command_is_routine(words: &[String]) -> bool { .to_ascii_lowercase(); // Package managers: fail-closed safe-subcommand allowlist (None = not a // package manager → fall through to the generic find/prefix checks). - if let Some(routine) = package_manager_subcommand_is_routine(&head, inner) { + if let Some(routine) = package_manager_subcommand_is_routine(&head, inner, inner_expandable) { return routine; } // `find` is routine only without a filesystem-mutating primary. @@ -490,6 +495,10 @@ fn bash_command_is_routine(words: &[String]) -> bool { if head == "gh" { return gh_subcommand_is_read_only(inner); } + // `rg --pre`/`--hostname-bin` execute a program per file; never routine. + if head == "rg" && super::manager::rg_has_unsafe_flag(inner, inner_expandable) { + return false; + } // Branch switches and stash workflow are routine; a path operand turns // `checkout` into a working-tree discard, so those forms fail closed. if head == "git" { @@ -500,11 +509,9 @@ fn bash_command_is_routine(words: &[String]) -> bool { _ => {} } } - let joined = inner.join(" ").to_ascii_lowercase(); - ROUTINE_PREFIXES.iter().any(|p| { - let base = p.trim(); - joined == base || (joined.starts_with(base) && joined[base.len()..].starts_with(' ')) - }) + ROUTINE_PREFIXES + .iter() + .any(|p| matches_prefix_words(inner, p, true)) } const BRANCH_SWITCH_BENIGN_FLAGS: &[&str] = &[ @@ -650,7 +657,11 @@ fn gh_subcommand_is_read_only(inner: &[String]) -> bool { /// fetch-and-run via [`is_remote_launcher`], explicit launchers via /// [`explicit_launch_target`] (which re-classifies the inner command after /// re-checking its writes/env), and everything else against a per-tool allowlist. -fn package_manager_subcommand_is_routine(prog: &str, inner: &[String]) -> Option { +fn package_manager_subcommand_is_routine( + prog: &str, + inner: &[String], + expandable: &[bool], +) -> Option { if !matches!( prog, "uv" | "uvx" | "npm" | "npx" | "pnpm" | "yarn" | "rustup" @@ -672,7 +683,10 @@ fn package_manager_subcommand_is_routine(prog: &str, inner: &[String]) -> Option return Some( !command_env_is_unsafe(launched) && !launched_writes_nonsink(launched) - && bash_command_is_routine(launched), + && bash_command_is_routine( + launched, + &expandable[inner.len() - launched.len()..], + ), ); } LaunchTarget::NotLauncher => {} @@ -2495,6 +2509,46 @@ mod tests { ] { assert_eq!(v(cmd), ClassifierVerdict::Block, "`{cmd}` must block"); } + for cmd in [ + "rg --pre ./pre.sh TODO .", + "rg --pre=cat TODO .", + "rg --hostname-bin=./payload needle", + "rg --hostname-bin ./payload needle", + "env rg --hostname-bin ./payload needle", + "RG --hostname-bin ./payload needle", + "timeout 5 RG --pre ./payload needle .", + "/usr/bin/rg --pre=cat needle .", + "rg --replace -- --pre ./payload needle .", + "rg --replace -e --hostname-bin ./payload needle .", + "rg -ne -- --hostname-bin ./payload .", + "rg -- --hostname-bin .", + "rg --host\"\"name-bin ./payload needle .", + "rg --p're' ./payload needle .", + "rg --{hostname-bin,hostname-bin}=./payload needle file.rs", + "rg --{pre,pre}=./payload needle file.rs", + "rg --hostname-b[i]n ./payload needle file.rs", + "rg ''--hostname-bin ./payload needle file.rs", + "rg {--hostname-bin,--hostname-bin}=./payload needle file.rs", + "rg \"--host\\\nname-bin\" ./payload needle file.rs", + "rg needle *", + "env \"rg\"\" \" needle file.rs", + "env rg\" /../payload\" needle file.rs", + "\"git status\"", + ] { + assert_eq!(v(cmd), ClassifierVerdict::Block, "`{cmd}` must block"); + } + for cmd in [ + "rg -n pattern .", + "rg --pre-glob '*.pdf' pattern .", + "rg -e needle --replace hostname .", + "rg -n \"foo bar\" .", + "rg -n 'foo bar' src", + "rg -g '*.rs' needle .", + "ls *.rs", + "grep \"foo bar\" file.txt", + ] { + assert_eq!(v(cmd), ClassifierVerdict::Allow, "`{cmd}` must be routine"); + } for cmd in [ "git checkout", "git checkout --detach main", diff --git a/crates/codegen/kigi-workspace/src/permission/bash_command_splitting.rs b/crates/codegen/kigi-workspace/src/permission/bash_command_splitting.rs index 1e4c3b8..b200e36 100644 --- a/crates/codegen/kigi-workspace/src/permission/bash_command_splitting.rs +++ b/crates/codegen/kigi-workspace/src/permission/bash_command_splitting.rs @@ -16,6 +16,8 @@ pub struct BashCommandHighlights { #[derive(Debug, Clone)] pub struct PlainCommand { words: Vec, + expandable: Vec, + legacy: Vec, span_start: usize, span_end: usize, } @@ -25,6 +27,106 @@ impl PlainCommand { pub fn words(&self) -> &[String] { &self.words } + /// Per word: an unquoted segment still carries glob, brace, tilde or + /// backslash syntax the shell would expand or decode into other argv. + pub fn expandable(&self) -> &[bool] { + &self.expandable + } + /// The spelling the pre-0.1.15 parser produced (string content with its + /// escapes, concatenations as raw source); saved deny rules from that + /// era are matched against it as well. + pub fn legacy_words(&self) -> &[String] { + &self.legacy + } +} + +/// `pattern` (`git status`) matches when its whitespace-separated words equal +/// the leading argv elements. Matching per element keeps a decoded word with +/// inner whitespace (`rg `, `rg /../x`) from posing as a safe prefix. +pub(crate) fn matches_prefix_words(words: &[String], pattern: &str, ignore_case: bool) -> bool { + let pat: Vec<&str> = pattern.split_whitespace().collect(); + !pat.is_empty() + && words.len() >= pat.len() + && pat.iter().zip(words).all(|(p, w)| { + if ignore_case { + w.eq_ignore_ascii_case(p) + } else { + w == p + } + }) +} + +/// Unquoted text the shell would still expand or decode before exec: +/// globs, brace lists, tilde, backslash escapes. +pub(crate) fn unquoted_may_expand(text: &str) -> bool { + text.contains(['*', '?', '[', ']', '{', '}', '~', '\\']) +} + +/// Bash's double-quote decoding: a backslash escapes only `$`, `` ` ``, +/// `"`, `\\` and newline (`\` is removed); any other backslash +/// stays literal. +fn decode_double_quoted(raw: &str) -> String { + let mut out = String::with_capacity(raw.len()); + let mut chars = raw.chars().peekable(); + while let Some(c) = chars.next() { + if c != '\\' { + out.push(c); + continue; + } + match chars.peek() { + Some('\n') => { + chars.next(); + } + Some(&escaped) if matches!(escaped, '$' | '`' | '"' | '\\') => { + out.push(escaped); + chars.next(); + } + _ => out.push('\\'), + } + } + out +} + +/// Decoded content of a plain double-quoted string: `"`, any number of +/// `string_content` parts, `"`. An expansion child rejects the string. The +/// text is the node's own source minus the two quote characters: +/// tree-sitter leaves literal newlines in the gaps between +/// `string_content` children and may fold whitespace into the closing +/// quote token, so neither the children nor the token bounds are the text. +fn simple_string_content(node: Node, src: &str) -> Option { + let count = node.child_count(); + if count < 2 + || node.child(0)?.kind() != "\"" + || node.child(count - 1)?.kind() != "\"" + || (1..count - 1).any(|i| { + node.child(i) + .is_none_or(|part| part.kind() != "string_content") + }) + { + return None; + } + let text = node.utf8_text(src.as_bytes()).ok()?; + Some(decode_double_quoted( + text.strip_prefix('"')?.strip_suffix('"')?, + )) +} + +/// What the old parser stored for a double-quoted string: its +/// `string_content` text with escapes intact, or the raw source when the +/// content is split (the old parser rejected those, so nothing was saved). +fn legacy_string_spelling(node: Node, src: &str) -> Option { + let text = match node.child_count() { + 3 => node.child(1)?.utf8_text(src.as_bytes()).ok()?, + _ => node.utf8_text(src.as_bytes()).ok()?, + }; + Some(text.to_owned()) +} + +fn raw_string_content<'a>(node: Node<'a>, src: &'a str) -> Option<&'a str> { + node.utf8_text(src.as_bytes()) + .ok()? + .strip_prefix('\'')? + .strip_suffix('\'') } /// Parse the provided bash source using tree-sitter-bash, returning a Tree on @@ -426,6 +528,8 @@ fn parse_plain_command_from_node(cmd: Node, src: &str) -> Option { return None; } let mut words = Vec::new(); + let mut expandable = Vec::new(); + let mut legacy = Vec::new(); let mut cursor = cmd.walk(); let mut span_start: Option = None; @@ -450,6 +554,8 @@ fn parse_plain_command_from_node(cmd: Node, src: &str) -> Option { } span_end = Some(word_node.end_byte()); + expandable.push(unquoted_may_expand(&text)); + legacy.push(text.clone()); words.push(text); } "word" | "number" => { @@ -460,55 +566,62 @@ fn parse_plain_command_from_node(cmd: Node, src: &str) -> Option { } span_end = Some(child.end_byte()); + expandable.push(unquoted_may_expand(&text)); + legacy.push(text.clone()); words.push(text); } "string" => { - // Allow only simple double-quoted strings with plain content. - if child.child_count() == 3 - && child.child(0)?.kind() == "\"" - && child.child(1)?.kind() == "string_content" - && child.child(2)?.kind() == "\"" - { - let content_node = child.child(1)?; - let text = content_node.utf8_text(src.as_bytes()).ok()?.to_owned(); + // Only a plain double-quoted string; interpolation rejects. + let text = simple_string_content(child, src)?; - if span_start.is_none() { - // Highlight whole quoted string in the original script, - // so span uses the outer node. - span_start = Some(child.start_byte()); - } - span_end = Some(child.end_byte()); - - words.push(text); - } else { - // Reject complex / interpolated strings - return None; + if span_start.is_none() { + // Highlight whole quoted string in the original script, + // so span uses the outer node. + span_start = Some(child.start_byte()); } + span_end = Some(child.end_byte()); + + expandable.push(false); + legacy.push(legacy_string_spelling(child, src)?); + words.push(text); } "raw_string" => { - let raw_string = child.utf8_text(src.as_bytes()).ok()?; - let stripped = raw_string - .strip_prefix('\'') - .and_then(|s| s.strip_suffix('\'')); - { - let s = stripped?; - if span_start.is_none() { - span_start = Some(child.start_byte()); - } - span_end = Some(child.end_byte()); - - words.push(s.to_owned()); + let text = raw_string_content(child, src)?.to_owned(); + if span_start.is_none() { + span_start = Some(child.start_byte()); } + span_end = Some(child.end_byte()); + + expandable.push(false); + legacy.push(text.clone()); + words.push(text); } "concatenation" => { - // Handle concatenation nodes (e.g., {} in find -exec) - let text = child.utf8_text(src.as_bytes()).ok()?.to_owned(); + // Decode the segments (`--host""name-bin` → `--hostname-bin`) + // so allowlists compare what the shell hands the program. + let mut text = String::new(); + let mut may_expand = false; + let mut parts = child.walk(); + for part in child.named_children(&mut parts) { + match part.kind() { + "word" | "number" => { + let t = part.utf8_text(src.as_bytes()).ok()?; + may_expand |= unquoted_may_expand(t); + text.push_str(t); + } + "string" => text.push_str(&simple_string_content(part, src)?), + "raw_string" => text.push_str(raw_string_content(part, src)?), + _ => return None, + } + } if span_start.is_none() { span_start = Some(child.start_byte()); } span_end = Some(child.end_byte()); + expandable.push(may_expand); + legacy.push(child.utf8_text(src.as_bytes()).ok()?.to_owned()); words.push(text); } _ => return None, @@ -520,6 +633,8 @@ fn parse_plain_command_from_node(cmd: Node, src: &str) -> Option { Some(PlainCommand { words, + expandable, + legacy, span_start, span_end, }) @@ -783,7 +898,7 @@ mod tests { highlighted_words: vec![ "python3".to_owned(), "-c".to_owned(), - "some text\\\" here ".to_owned() + "some text\" here ".to_owned() ], suffix: vec![ "|".to_owned(), @@ -878,8 +993,7 @@ mod tests { highlighted_words: vec![ "python".to_owned(), "-c".to_owned(), - "\"\n import os\n os.environ.get(\"something\")\n \"" - .to_owned() + "\n import os\n os.environ.get(something)\n ".to_owned() ], suffix: vec![], }) @@ -1245,3 +1359,64 @@ mod tests { ); } } + +#[cfg(test)] +mod decoded_word_tests { + use super::all_commands_from_script; + + fn parse(script: &str) -> (Vec, Vec) { + let cmds = all_commands_from_script(script).expect("word-only script"); + let cmd = &cmds[0]; + (cmd.words().to_vec(), cmd.expandable().to_vec()) + } + + #[test] + fn concatenations_decode_to_what_the_shell_passes() { + for (script, decoded) in [ + ("rg --host\"\"name-bin x", "--hostname-bin"), + ("rg ''--hostname-bin x", "--hostname-bin"), + ("rg --p're' x", "--pre"), + ("rg --pre=\"a b\" x", "--pre=a b"), + ("rg \"--host\\\nname-bin\" x", "--hostname-bin"), + ("rg --host\"\\\nname-bin\" x", "--hostname-bin"), + ("rg \"a\\\"b\\$c\\d\" x", "a\"b$c\\d"), + ("rg \"foo\nbar\" x", "foo\nbar"), + ("rg \" two words \" x", " two words "), + ("rg \" \" x", " "), + ("rg \"foo\n \" x", "foo\n "), + ("env \"rg\"\" \" needle x", "rg "), + ] { + let (words, expandable) = parse(script); + assert_eq!(words[1], decoded, "{script}"); + assert!(!expandable[1], "{script}"); + } + } + + #[test] + fn unquoted_expansion_syntax_is_flagged_and_quoted_is_not() { + let (words, expandable) = parse("rg --{pre,pre}=x {--pre,--pre} *.rs -g '*.rs' \"?\""); + assert_eq!(words[1], "--{pre,pre}=x"); + assert_eq!( + expandable, + vec![false, true, true, true, false, false, false] + ); + let (_, expandable) = parse("cat ~/notes.txt"); + assert!(expandable[1]); + } + + #[test] + fn legacy_spelling_keeps_escapes_and_raw_concatenations() { + let cmds = all_commands_from_script("rg \"\\\"api_key\\\"\" pri\"vate\" x").unwrap(); + assert_eq!(cmds[0].words(), ["rg", "\"api_key\"", "private", "x"]); + assert_eq!( + cmds[0].legacy_words(), + ["rg", "\\\"api_key\\\"", "pri\"vate\"", "x"] + ); + } + + #[test] + fn a_quoted_newline_in_the_head_is_not_the_bare_program() { + let (words, _) = parse("env \"r\ng\" needle file.rs"); + assert_eq!(words[1], "r\ng"); + } +} diff --git a/crates/codegen/kigi-workspace/src/permission/manager.rs b/crates/codegen/kigi-workspace/src/permission/manager.rs index 20442d7..54d88ec 100644 --- a/crates/codegen/kigi-workspace/src/permission/manager.rs +++ b/crates/codegen/kigi-workspace/src/permission/manager.rs @@ -8,7 +8,8 @@ use kigi_acp_lib::AcpAgentGatewaySender as GatewaySender; use tokio::sync::{mpsc, oneshot}; use crate::permission::bash_command_splitting::{ - all_commands_from_script, is_setup_command, unwrap_wrappers, + all_commands_from_script, is_setup_command, matches_prefix_words, unquoted_may_expand, + unwrap_wrappers, }; use crate::permission::policy::CompiledPolicy; use crate::permission::prompter::{AcpPrompter, PromptOutcome}; @@ -143,75 +144,98 @@ fn mcp_pre_decision( /// True when `words` is an `rg` invocation that enables a preprocessor. /// -/// `rg --pre COMMAND` (or `--pre=COMMAND`) runs `COMMAND ` for every -/// searched file, so it can execute arbitrary programs. It must not ride the -/// built-in safe-command auto-allow (unlike a pipeline, `--pre` stays one -/// bash segment whose primary is still `rg`). -/// -/// Deliberately does **not** match `--pre-glob`, which only filters when a -/// preprocessor runs and does not itself spawn processes. -fn rg_has_pre_flag(words: &[String]) -> bool { - if words.first().map(String::as_str) != Some("rg") { +/// `rg` options that execute a program per searched file: `--pre COMMAND` +/// runs it as a preprocessor, `--hostname-bin COMMAND` to print hostnames. +/// Both accept `--flag=VALUE`; `--pre-glob` only filters and is not listed. +pub(crate) const RG_UNSAFE_FLAGS: &[&str] = &["--pre", "--hostname-bin"]; + +/// Whether an `rg` invocation carries a flag from [`RG_UNSAFE_FLAGS`], so +/// it must not ride the built-in safe-command auto-allow (unlike a +/// pipeline, the flag stays one bash segment whose primary is still `rg`). +/// The head is matched by lowercased basename like Auto's routine check. +/// Every later word is inspected, on purpose: rg's option grammar +/// (`--replace -- --pre …`, `-ne -- --hostname-bin …`) makes any partial +/// arity model bypassable, so a literal search for the flag name costs a +/// prompt instead of the auto-allow. Words are the parser's decoded text; +/// one the shell would still expand (`--{pre,pre}`, a glob that could +/// match a file named `--pre`) counts as unsafe too. +pub(crate) fn rg_has_unsafe_flag(words: &[String], expandable: &[bool]) -> bool { + let head = words.first().map(|w| { + w.rsplit(['/', '\\']) + .next() + .unwrap_or(w) + .to_ascii_lowercase() + }); + if head.as_deref() != Some("rg") { return false; } - words - .iter() - .any(|w| w == "--pre" || w.starts_with("--pre=")) + words.iter().zip(expandable).skip(1).any(|(w, may_expand)| { + let name = w.split_once('=').map_or(w.as_str(), |(name, _)| name); + *may_expand || RG_UNSAFE_FLAGS.contains(&name) + }) } /// Check whether the command words (already parsed by tree-sitter) match one of /// the known safe command prefixes. -fn is_safe_command_words(words: &[String]) -> bool { +fn is_safe_command_words(words: &[String], expandable: &[bool]) -> bool { if words.is_empty() { return false; } - if rg_has_pre_flag(words) { + if rg_has_unsafe_flag(words, expandable) { return false; } - let joined = words.join(" "); - is_safe_command_words_str(&joined) + SAFE_COMMAND_PREFIXES + .iter() + .any(|p| matches_prefix_words(words, p, false)) } fn matches_command_prefix(cmd: &str, pattern: &str) -> bool { cmd == pattern || (cmd.starts_with(pattern) && cmd.as_bytes().get(pattern.len()) == Some(&b' ')) } -/// Shared prefix check used by both the tree-sitter path and the fallback path. +/// Built-in safe prefixes shared by the argv path and the display-string path. +const SAFE_COMMAND_PREFIXES: &[&str] = &[ + "ls", + "cat", + "pwd", + "date", + "git status", + "git branch", + "git log", + "git diff", + "git ls-files", + "git show", + "git rev-parse", + "cargo check", + "whoami", + "hostname", + "uptime", + "grep", + "rg", + "kubectl get", + "kubectl logs", + "kubectl describe", + "ps", + "bin/explorer ls", + "head", + "tail", + "wc", + "sort", + "uniq", + "tr", + "cut", +]; + +/// Display-string form of [`SAFE_COMMAND_PREFIXES`] (scope suggestions). fn is_safe_command_words_str(cmd: &str) -> bool { - matches_command_prefix(cmd, "ls") - || matches_command_prefix(cmd, "cat") - || matches_command_prefix(cmd, "pwd") - || matches_command_prefix(cmd, "date") - || matches_command_prefix(cmd, "git status") - || matches_command_prefix(cmd, "git branch") - || matches_command_prefix(cmd, "git log") - || matches_command_prefix(cmd, "git diff") - || matches_command_prefix(cmd, "git ls-files") - || matches_command_prefix(cmd, "git show") - || matches_command_prefix(cmd, "git rev-parse") - || matches_command_prefix(cmd, "cargo check") - || matches_command_prefix(cmd, "whoami") - || matches_command_prefix(cmd, "hostname") - || matches_command_prefix(cmd, "uptime") - || matches_command_prefix(cmd, "grep") - || matches_command_prefix(cmd, "rg") - || matches_command_prefix(cmd, "kubectl get") - || matches_command_prefix(cmd, "kubectl logs") - || matches_command_prefix(cmd, "kubectl describe") - || matches_command_prefix(cmd, "ps") - || matches_command_prefix(cmd, "bin/explorer ls") - || matches_command_prefix(cmd, "head") - || matches_command_prefix(cmd, "tail") - || matches_command_prefix(cmd, "wc") - || matches_command_prefix(cmd, "sort") - || matches_command_prefix(cmd, "uniq") - || matches_command_prefix(cmd, "tr") - || matches_command_prefix(cmd, "cut") + SAFE_COMMAND_PREFIXES + .iter() + .any(|p| matches_command_prefix(cmd, p)) // CWE-863: `tee` is not in the safe-command list — it writes stdin // to arbitrary files, enabling pipelines like `cat data | tee /target` to // bypass edit permissions. // - // `rg --pre` is excluded at the words level via [`rg_has_pre_flag`] — the + // `rg --pre` is excluded at the words level via [`rg_has_unsafe_flag`] — the // string form here cannot see flag structure reliably after join. } @@ -254,25 +278,17 @@ const ALWAYS_SAFE_COMMANDS: &[&str] = &[ /// auto-approve via the always-safe primary alone — every non-setup segment /// must independently pass this (or the broader `is_safe_command_words`, /// or a user whitelist) check. -fn is_always_safe_command_words(words: &[String]) -> bool { +fn is_always_safe_command_words(words: &[String], expandable: &[bool]) -> bool { if words.is_empty() { return false; } - if rg_has_pre_flag(words) { + if rg_has_unsafe_flag(words, expandable) { return false; } - - let joined = words.join(" "); - - // CWE-183: use matches_command_prefix to require a word boundary after - // the safe prefix, preventing e.g. "tr" from matching "truncate". - for safe_pattern in ALWAYS_SAFE_COMMANDS { - if matches_command_prefix(&joined, safe_pattern) { - return true; - } - } - - false + // CWE-183: whole-word match, so "tr" cannot match "truncate". + ALWAYS_SAFE_COMMANDS + .iter() + .any(|p| matches_prefix_words(words, p, false)) } /// Default always-allow whitelist scope (word count) for a parsed command. @@ -291,7 +307,9 @@ pub fn default_always_allow_scope(words: &[String]) -> usize { if words.is_empty() { return 0; } - if is_safe_command_words(words) { + // Display words carry no parser flags; expansion syntax is conservative. + let expandable: Vec = words.iter().map(|w| unquoted_may_expand(w)).collect(); + if is_safe_command_words(words, &expandable) { if is_safe_command_words_str(&words[0]) { return 1; } @@ -314,26 +332,35 @@ pub fn default_always_allow_scope(words: &[String]) -> usize { /// `is_dangerous_command` script-level check, but applied to every /// segment in a chain instead of only the start of the script. fn is_dangerous_command_words(words: &[String]) -> bool { - if words.is_empty() { - return false; - } - let joined = words.join(" "); - matches_command_prefix(&joined, "rm") - || matches_command_prefix(&joined, "chmod") - || matches_command_prefix(&joined, "chown") - || matches_command_prefix(&joined, "chgrp") - || matches_command_prefix(&joined, "chattr") - || matches_command_prefix(&joined, "pkill") - || matches_command_prefix(&joined, "kill") - || matches_command_prefix(&joined, "killall") - || matches_command_prefix(&joined, "git push") + [ + "rm", "chmod", "chown", "chgrp", "chattr", "pkill", "kill", "killall", "git push", + ] + .iter() + .any(|p| matches_prefix_words(words, p, false)) } /// Whitelist matching helper. Uses `matches_command_prefix` so that user /// allow/deny entries enforce a word boundary after the prefix — preventing /// the "git" entry from matching "gitleaks" (CWE-183). -fn matches_whitelist_prefix(segment_str: &str, allowed_prefix: &str) -> bool { - matches_command_prefix(segment_str, allowed_prefix) +/// A saved rule is the selected argv elements joined by one space, so it +/// matches when some argv prefix joins back to it exactly: `rg private key` +/// covers both `rg "private key"` and `rg private key`, while a padded +/// program name (`rg `) never joins to `rg`. +fn matches_saved_deny(words: &[String], legacy_inner: &[String], rule: &str) -> bool { + // The legacy branch is the pre-0.1.15 check verbatim (wrappers peeled on + // the legacy spelling, space-boundary prefix on its joined text), so no + // deny saved by an older build narrows on upgrade. + (1..=words.len()).any(|k| words[..k].join(" ") == rule) + || matches_command_prefix(&legacy_inner.join(" "), rule) +} + +/// A grant matches only token for word: the rule's tokens must equal the +/// leading argv elements one by one, so neither one word spelled +/// `git status` nor `./filter -n` absorbing a granted `-n` can claim a +/// grant. A grant saved from a spaced argument therefore prompts again; +/// that is the safe direction for an allow. +fn matches_saved_grant(words: &[String], rule: &str) -> bool { + matches_prefix_words(words, rule, false) } /// Result of evaluating a bash script's segments against the current @@ -392,6 +419,8 @@ pub(crate) fn evaluate_bash_segments_inner( // such as `timeout 30 rm -rf /tmp/foo` would be treated as a benign // `timeout` invocation and silently auto-allowed. let words = unwrap_wrappers(raw_words); + let expandable = &parsed.expandable()[raw_words.len() - words.len()..]; + let legacy_inner = unwrap_wrappers(parsed.legacy_words()); if is_setup_command(words) { continue; } @@ -401,7 +430,7 @@ pub(crate) fn evaluate_bash_segments_inner( if let Some(d) = state .disallowed_bash_commands .iter() - .find(|d| matches_whitelist_prefix(&s, d)) + .find(|d| matches_saved_deny(words, legacy_inner, d)) { return SegmentEvaluation::Reject(format!( "User previously rejected `{d}` for this session" @@ -422,9 +451,10 @@ pub(crate) fn evaluate_bash_segments_inner( let matched_grant = state .allowed_bash_commands .iter() - .any(|a| matches_whitelist_prefix(&s, a)); + .any(|a| matches_saved_grant(words, a)); let matched_safe = honor_safe_lists - && (is_safe_command_words(words) || is_always_safe_command_words(words)); + && (is_safe_command_words(words, expandable) + || is_always_safe_command_words(words, expandable)); if matched_grant || matched_safe { if matched_grant { via_session_grant = true; @@ -3616,7 +3646,7 @@ mod tests { /// Test shim: pure rename of `is_always_safe_primary_command`. fn is_always_safe_primary_command(words: &[String]) -> bool { - is_always_safe_command_words(words) + is_always_safe_command_words(words, &vec![false; words.len()]) } #[test] @@ -3673,6 +3703,57 @@ mod tests { assert!(!is_safe_command("rg --pre cat pattern .")); assert!(!is_safe_command("rg --pre=/bin/cat pattern .")); assert!(!is_safe_command("rg -n --pre ./wrapper pattern")); + // --hostname-bin runs COMMAND to print the hostname — same exec hole. + assert!(!is_safe_command("rg --hostname-bin=./payload needle")); + assert!(!is_safe_command("rg --hostname-bin ./payload needle")); + // rg's option grammar lets a real flag follow a `--` or `-e` that is + // itself a value; the scan therefore never trusts those positions, + // and a literal search for the flag name falls back to a prompt. + assert!(!is_safe_command("rg --replace -- --pre ./payload needle .")); + assert!(!is_safe_command( + "rg --replace -e --hostname-bin ./payload needle ." + )); + assert!(!is_safe_command("rg -ne -- --hostname-bin ./payload .")); + assert!(!is_safe_command("rg -e needle --hostname-bin ./payload .")); + assert!(!is_safe_command("rg -- --hostname-bin .")); + assert!(!is_safe_command("RG --pre ./payload needle .")); + assert!(!is_safe_command("/usr/bin/rg --pre=cat needle .")); + // Quote concatenation keeps its source text through the parser; the + // shell would decode it into the flag. + assert!(!is_safe_command("rg --host\"\"name-bin ./payload needle .")); + assert!(!is_safe_command("rg --p're' ./payload needle .")); + assert!(!is_safe_command("rg --hos\\tname-bin ./payload needle .")); + assert!(!is_safe_command( + "rg --{hostname-bin,hostname-bin}=./payload needle file.rs" + )); + assert!(!is_safe_command("rg --{pre,pre}=./payload needle file.rs")); + assert!(!is_safe_command( + "rg --hostname-b[i]n ./payload needle file.rs" + )); + assert!(!is_safe_command( + "rg ''--hostname-bin ./payload needle file.rs" + )); + assert!(!is_safe_command( + "rg {--hostname-bin,--hostname-bin}=./payload needle file.rs" + )); + assert!(!is_safe_command( + "rg \"--host\\\nname-bin\" ./payload needle file.rs" + )); + // Decoded words keep their argv boundary: a padded or slashed program + // name is not `rg`, and a single word is not a two-word prefix. + assert!(!is_safe_command("env \"rg\"\" \" needle file.rs")); + assert!(!is_safe_command("env rg\" /../payload\" needle file.rs")); + assert!(!is_safe_command("\"git status\"")); + assert!(is_safe_command("grep \"foo bar\" file.txt")); + assert!(!is_safe_command("rg needle *")); + assert!(is_safe_command("rg -n \"foo bar\" .")); + assert!(is_safe_command("rg -g '*.rs' needle .")); + assert!(is_safe_command("ls *.rs")); + let unsafe_words: Vec = "rg --hostname-bin=./payload needle" + .split(' ') + .map(String::from) + .collect(); + assert_eq!(default_always_allow_scope(&unsafe_words), 2); assert!(!is_safe_command( "rg --pre-glob '*.pdf' --pre pdftotext pattern" )); @@ -4183,6 +4264,92 @@ mod tests { } } + #[test] + fn saved_rules_match_argv_prefixes_not_joined_text() { + let mut state = PermissionState::default(); + state + .disallowed_bash_commands + .insert("rg private key".to_string()); + for cmd in ["rg \"private key\" file.rs", "rg private key file.rs"] { + assert!( + matches!( + evaluate_bash_segments(cmd, &state), + SegmentEvaluation::Reject(_) + ), + "`{cmd}` must hit the saved deny" + ); + } + // Denies saved by the pre-0.1.15 parser keep their raw spelling and + // the old space-boundary prefix semantics. + let mut state = PermissionState::default(); + for rule in [ + "rg \\\"api_key\\\"", + "rg pri\"vate\"", + "rg secret", + "en\"v\" rg", + ] { + state.disallowed_bash_commands.insert(rule.to_string()); + } + for cmd in [ + "rg \"\\\"api_key\\\"\" file.rs", + "rg pri\"vate\" file.rs", + "rg \"secret phrase\" file.rs", + "env rg \"secret phrase\" file.rs", + "env en\"v\" rg needle file.rs", + ] { + assert!( + matches!( + evaluate_bash_segments(cmd, &state), + SegmentEvaluation::Reject(_) + ), + "`{cmd}` must hit its legacy deny" + ); + } + assert!(!matches!( + evaluate_bash_segments("rg secretive file.rs", &state), + SegmentEvaluation::Reject(_) + )); + let mut state = PermissionState::default(); + state.allowed_bash_commands.insert("rg".to_string()); + state.allowed_bash_commands.insert("git status".to_string()); + assert!(matches!( + evaluate_bash_segments("git status --short", &state), + SegmentEvaluation::AutoAllow { .. } + )); + assert!( + !matches!( + evaluate_bash_segments("env \"rg\"\" \" needle file.rs", &state), + SegmentEvaluation::AutoAllow { .. } + ), + "a padded program name is not the granted `rg`" + ); + // A grant matches token for word: neither one executable word nor a + // spaced argument absorbing a granted token can claim it. + let mut state = PermissionState::default(); + state.allowed_bash_commands.insert("git status".to_string()); + state + .allowed_bash_commands + .insert("rg --pre ./filter -n".to_string()); + for cmd in [ + "env git\" status\" --short", + "env rg\" --pre ./filter -n\" needle file.rs", + "rg --pre ./filter\" -n\" needle file.rs", + "env rg --pre ./filter\" -n\" needle file.rs", + ] { + assert!( + !matches!( + evaluate_bash_segments(cmd, &state), + SegmentEvaluation::AutoAllow { .. } + ), + "`{cmd}` must not ride a multiword grant" + ); + } + assert!(matches!( + evaluate_bash_segments("rg --pre ./filter -n needle file.rs", &state), + SegmentEvaluation::AutoAllow { .. } + )); + } + #[test] fn evaluate_disallow_segment_rejects_whole_script() { // Disallow on any segment short-circuits with a Reject for the