From dbeb0af9adeb0839fc0d7b3949392bb183ce183a Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Wed, 28 Jan 2026 15:51:39 -0500 Subject: [PATCH] Fix shell injection vulnerability in terminal tool permissions (#47807) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Screenshot 2026-01-28 at 3 35 52 PM Screenshot 2026-01-28 at 3 40 54 PM Previously, if a user configured `^ls` as an always-allow pattern, an attacker could craft a command like `ls && rm -rf /` which would be auto-approved because the regex only matched the beginning of the command string. Now the command is parsed into individual sub-commands (`ls`, `rm -rf /`) and EACH sub-command must match an allow pattern for auto-approval. This prevents shell injection attacks using operators like: - `&&` and `||` (boolean operators) - `;` and `&` (sequential/background execution) - `|` (pipes) - Newlines - Command substitution (`$()` and backticks) - Process substitution (`<()` and `>()`) ## Matching Logic - **always_deny**: if ANY sub-command matches, deny the entire command - **always_confirm**: if ANY sub-command matches, require confirmation (unless always_deny matched, in which case deny) - **always_allow**: ALL sub-commands must match for auto-approval (unless always_confirm or always_deny matched, in which case defer to those) - If parsing fails, or if the shell is unsupported, then always_allow is disabled for this command As usual, `always_allow_tool_actions` supercedes all of these. If it is `true`, then we always allow all tool calls, no questions asked. ## Shell Compatibility The shell parser only supports POSIX-like command chaining syntax (`&&`, `||`, `;`, `|`). **Supported shells:** Posix (sh, bash, dash, zsh), Fish 3.0+, PowerShell 7+/Pwsh, Cmd, Xonsh, Csh, Tcsh **Unsupported shells:** Nushell (uses `and`/`or` keywords), Elvish (uses `and`/`or` keywords), Rc (Plan 9 shell - no `&&`/`||` operators) For unsupported shells: - The "Always allow" UI options are hidden for the terminal tool - If the user has `always_allow` patterns configured in settings, they will see a `Deny` with an explanatory error message (No release notes because granular tool permissions are behind a feature flag.) Release Notes: - N/A --------- Co-authored-by: Zed Zippy <234243425+zed-zippy[bot]@users.noreply.github.com> --- Cargo.lock | 83 ++++ Cargo.toml | 1 + crates/agent/Cargo.toml | 1 + crates/agent/src/agent.rs | 1 + crates/agent/src/pattern_extraction.rs | 49 +- crates/agent/src/shell_parser.rs | 396 +++++++++++++++ crates/agent/src/thread.rs | 89 +++- crates/agent/src/tool_permissions.rs | 634 +++++++++++++++++++------ crates/util/src/shell.rs | 24 + 9 files changed, 1082 insertions(+), 196 deletions(-) create mode 100644 crates/agent/src/shell_parser.rs diff --git a/Cargo.lock b/Cargo.lock index 67449974992ec870d714f2920efff856494c8056..27355fd88341c50063278fb2e19c86e8d14d1f8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -161,6 +161,7 @@ dependencies = [ "agent_servers", "agent_settings", "anyhow", + "brush-parser", "chrono", "client", "clock", @@ -2290,6 +2291,31 @@ dependencies = [ "unicode-segmentation", ] +[[package]] +name = "bon" +version = "3.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "234655ec178edd82b891e262ea7cf71f6584bcd09eff94db786be23f1821825c" +dependencies = [ + "bon-macros", + "rustversion", +] + +[[package]] +name = "bon-macros" +version = "3.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "89ec27229c38ed0eb3c0feee3d2c1d6a4379ae44f418a29a658890e062d8f365" +dependencies = [ + "darling", + "ident_case", + "prettyplease", + "proc-macro2", + "quote", + "rustversion", + "syn 2.0.106", +] + [[package]] name = "borrow-or-share" version = "0.2.4" @@ -2351,6 +2377,21 @@ dependencies = [ "alloc-stdlib", ] +[[package]] +name = "brush-parser" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f7367124d4f38fdcd65f4b815bda7caeb3de377b9cd95ffa1b23627989c93718" +dependencies = [ + "bon", + "cached", + "indenter", + "peg", + "thiserror 2.0.17", + "tracing", + "utf8-chars", +] + [[package]] name = "bstr" version = "1.12.0" @@ -8370,6 +8411,12 @@ version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e7c5cedc30da3a610cac6b4ba17597bdf7152cf974e8aab3afb3d54455e371c8" +[[package]] +name = "indenter" +version = "0.3.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "964de6e86d545b246d84badc0fef527924ace5134f30641c203ef52ba83f58d5" + [[package]] name = "indexmap" version = "2.11.4" @@ -11609,6 +11656,33 @@ version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0008e816fcdaf229cdd540e9b6ca2dc4a10d65c31624abb546c6420a02846e61" +[[package]] +name = "peg" +version = "0.8.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9928cfca101b36ec5163e70049ee5368a8a1c3c6efc9ca9c5f9cc2f816152477" +dependencies = [ + "peg-macros", + "peg-runtime", +] + +[[package]] +name = "peg-macros" +version = "0.8.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6298ab04c202fa5b5d52ba03269fb7b74550b150323038878fe6c372d8280f71" +dependencies = [ + "peg-runtime", + "proc-macro2", + "quote", +] + +[[package]] +name = "peg-runtime" +version = "0.8.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "132dca9b868d927b35b5dd728167b2dee150eb1ad686008fc71ccb298b776fca" + [[package]] name = "pem" version = "3.0.6" @@ -18274,6 +18348,15 @@ version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09cc8ee72d2a9becf2f2febe0205bbed8fc6615b7cb429ad062dc7b7ddd036a9" +[[package]] +name = "utf8-chars" +version = "3.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ebe49e006d6df172d7f14794568a90fe41e05a1fa9e03dc276fa6da4bb747ec3" +dependencies = [ + "arrayvec", +] + [[package]] name = "utf8_iter" version = "1.0.4" diff --git a/Cargo.toml b/Cargo.toml index 8e7ea3d734b25b00821c60d90ea165294ac8f2af..7310d5b4f31179f6ae9ddb9170718ba359765b76 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -497,6 +497,7 @@ brotli = "8.0.2" bytes = "1.0" cargo_metadata = "0.19" cargo_toml = "0.21" +brush-parser = "0.3" cfg-if = "1.0.3" chardetng = "0.1" chrono = { version = "0.4", features = ["serde"] } diff --git a/crates/agent/Cargo.toml b/crates/agent/Cargo.toml index 83bac27f76d1ef6ddbd9e69598a0b8044f0a4bae..0a9a3541890795b1a1783211559242d60b8b4e8b 100644 --- a/crates/agent/Cargo.toml +++ b/crates/agent/Cargo.toml @@ -19,6 +19,7 @@ workspace = true [dependencies] acp_thread.workspace = true +brush-parser.workspace = true action_log.workspace = true agent-client-protocol.workspace = true agent_servers.workspace = true diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index d8d2f4d3a74be2d9bd0f9c82add1301f1a0299a9..43555e30dc4fe8830150a44bd0793f551835845a 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -4,6 +4,7 @@ mod legacy_thread; mod native_agent_server; pub mod outline; mod pattern_extraction; +mod shell_parser; mod templates; #[cfg(test)] mod tests; diff --git a/crates/agent/src/pattern_extraction.rs b/crates/agent/src/pattern_extraction.rs index e3b3bc20d98c6d45a0b6962c28c8e1d0b33bf266..96accf5421bf175ad2ca24a141e43e3c5432be3f 100644 --- a/crates/agent/src/pattern_extraction.rs +++ b/crates/agent/src/pattern_extraction.rs @@ -1,13 +1,17 @@ +use crate::shell_parser::extract_commands; use url::Url; -/// Extracts a regex pattern from a terminal command based on the first token (command name). +/// Extracts the command name from a shell command using the shell parser. /// -/// Returns `None` for commands starting with `./`, `/`, or other path-like prefixes. -/// This is a deliberate security decision: we only allow pattern-based "always allow" -/// rules for well-known command names (like `cargo`, `npm`, `git`), not for arbitrary -/// scripts or absolute paths which could be manipulated by an attacker. -pub fn extract_terminal_pattern(command: &str) -> Option { - let first_token = command.split_whitespace().next()?; +/// This parses the command properly to extract just the command name (first word), +/// handling shell syntax correctly. Returns `None` if parsing fails or if the +/// command name contains path separators (for security reasons). +fn extract_command_name(command: &str) -> Option { + let commands = extract_commands(command)?; + let first_command = commands.first()?; + + let first_token = first_command.split_whitespace().next()?; + // Only allow alphanumeric commands with hyphens/underscores. // Reject paths like "./script.sh" or "/usr/bin/python" to prevent // users from accidentally allowing arbitrary script execution. @@ -15,22 +19,25 @@ pub fn extract_terminal_pattern(command: &str) -> Option { .chars() .all(|c| c.is_alphanumeric() || c == '-' || c == '_') { - Some(format!("^{}\\s", regex::escape(first_token))) + Some(first_token.to_string()) } else { None } } +/// Extracts a regex pattern from a terminal command based on the first token (command name). +/// +/// Returns `None` for commands starting with `./`, `/`, or other path-like prefixes. +/// This is a deliberate security decision: we only allow pattern-based "always allow" +/// rules for well-known command names (like `cargo`, `npm`, `git`), not for arbitrary +/// scripts or absolute paths which could be manipulated by an attacker. +pub fn extract_terminal_pattern(command: &str) -> Option { + let command_name = extract_command_name(command)?; + Some(format!("^{}\\b", regex::escape(&command_name))) +} + pub fn extract_terminal_pattern_display(command: &str) -> Option { - let first_token = command.split_whitespace().next()?; - if first_token - .chars() - .all(|c| c.is_alphanumeric() || c == '-' || c == '_') - { - Some(first_token.to_string()) - } else { - None - } + extract_command_name(command) } pub fn extract_path_pattern(path: &str) -> Option { @@ -71,19 +78,19 @@ mod tests { fn test_extract_terminal_pattern() { assert_eq!( extract_terminal_pattern("cargo build --release"), - Some("^cargo\\s".to_string()) + Some("^cargo\\b".to_string()) ); assert_eq!( extract_terminal_pattern("npm install"), - Some("^npm\\s".to_string()) + Some("^npm\\b".to_string()) ); assert_eq!( extract_terminal_pattern("git-lfs pull"), - Some("^git\\-lfs\\s".to_string()) + Some("^git\\-lfs\\b".to_string()) ); assert_eq!( extract_terminal_pattern("my_script arg"), - Some("^my_script\\s".to_string()) + Some("^my_script\\b".to_string()) ); assert_eq!(extract_terminal_pattern("./script.sh arg"), None); assert_eq!(extract_terminal_pattern("/usr/bin/python arg"), None); diff --git a/crates/agent/src/shell_parser.rs b/crates/agent/src/shell_parser.rs new file mode 100644 index 0000000000000000000000000000000000000000..47c13f7687a4a6cc1fc878489ac64d9f64f64098 --- /dev/null +++ b/crates/agent/src/shell_parser.rs @@ -0,0 +1,396 @@ +use brush_parser::ast; +use brush_parser::word::WordPiece; +use brush_parser::{Parser, ParserOptions, SourceInfo}; +use std::io::BufReader; + +pub fn extract_commands(command: &str) -> Option> { + let reader = BufReader::new(command.as_bytes()); + let options = ParserOptions::default(); + let source_info = SourceInfo::default(); + let mut parser = Parser::new(reader, &options, &source_info); + + let program = parser.parse_program().ok()?; + + let mut commands = Vec::new(); + extract_commands_from_program(&program, &mut commands); + + Some(commands) +} + +fn extract_commands_from_program(program: &ast::Program, commands: &mut Vec) { + for complete_command in &program.complete_commands { + extract_commands_from_compound_list(complete_command, commands); + } +} + +fn extract_commands_from_compound_list( + compound_list: &ast::CompoundList, + commands: &mut Vec, +) { + for item in &compound_list.0 { + extract_commands_from_and_or_list(&item.0, commands); + } +} + +fn extract_commands_from_and_or_list(and_or_list: &ast::AndOrList, commands: &mut Vec) { + extract_commands_from_pipeline(&and_or_list.first, commands); + + for and_or in &and_or_list.additional { + match and_or { + ast::AndOr::And(pipeline) | ast::AndOr::Or(pipeline) => { + extract_commands_from_pipeline(pipeline, commands); + } + } + } +} + +fn extract_commands_from_pipeline(pipeline: &ast::Pipeline, commands: &mut Vec) { + for command in &pipeline.seq { + extract_commands_from_command(command, commands); + } +} + +fn extract_commands_from_command(command: &ast::Command, commands: &mut Vec) { + match command { + ast::Command::Simple(simple_command) => { + extract_commands_from_simple_command(simple_command, commands); + } + ast::Command::Compound(compound_command, _redirect_list) => { + extract_commands_from_compound_command(compound_command, commands); + } + ast::Command::Function(func_def) => { + extract_commands_from_function_body(&func_def.body, commands); + } + ast::Command::ExtendedTest(test_expr) => { + extract_commands_from_extended_test_expr(test_expr, commands); + } + } +} + +fn extract_commands_from_simple_command( + simple_command: &ast::SimpleCommand, + commands: &mut Vec, +) { + let command_str = simple_command.to_string(); + if !command_str.trim().is_empty() { + commands.push(command_str); + } + + if let Some(prefix) = &simple_command.prefix { + extract_commands_from_command_prefix(prefix, commands); + } + if let Some(word) = &simple_command.word_or_name { + extract_commands_from_word(word, commands); + } + if let Some(suffix) = &simple_command.suffix { + extract_commands_from_command_suffix(suffix, commands); + } +} + +fn extract_commands_from_command_prefix(prefix: &ast::CommandPrefix, commands: &mut Vec) { + for item in &prefix.0 { + extract_commands_from_prefix_or_suffix_item(item, commands); + } +} + +fn extract_commands_from_command_suffix(suffix: &ast::CommandSuffix, commands: &mut Vec) { + for item in &suffix.0 { + extract_commands_from_prefix_or_suffix_item(item, commands); + } +} + +fn extract_commands_from_prefix_or_suffix_item( + item: &ast::CommandPrefixOrSuffixItem, + commands: &mut Vec, +) { + match item { + ast::CommandPrefixOrSuffixItem::IoRedirect(redirect) => { + extract_commands_from_io_redirect(redirect, commands); + } + ast::CommandPrefixOrSuffixItem::AssignmentWord(assignment, _word) => { + extract_commands_from_assignment(assignment, commands); + } + ast::CommandPrefixOrSuffixItem::Word(word) => { + extract_commands_from_word(word, commands); + } + ast::CommandPrefixOrSuffixItem::ProcessSubstitution(_kind, subshell) => { + extract_commands_from_compound_list(&subshell.list, commands); + } + } +} + +fn extract_commands_from_io_redirect(redirect: &ast::IoRedirect, commands: &mut Vec) { + match redirect { + ast::IoRedirect::File(_fd, _kind, target) => { + if let ast::IoFileRedirectTarget::ProcessSubstitution(_kind, subshell) = target { + extract_commands_from_compound_list(&subshell.list, commands); + } + } + ast::IoRedirect::HereDocument(_fd, _here_doc) => {} + ast::IoRedirect::HereString(_fd, word) => { + extract_commands_from_word(word, commands); + } + ast::IoRedirect::OutputAndError(word, _) => { + extract_commands_from_word(word, commands); + } + } +} + +fn extract_commands_from_assignment(assignment: &ast::Assignment, commands: &mut Vec) { + match &assignment.value { + ast::AssignmentValue::Scalar(word) => { + extract_commands_from_word(word, commands); + } + ast::AssignmentValue::Array(words) => { + for (opt_word, word) in words { + if let Some(w) = opt_word { + extract_commands_from_word(w, commands); + } + extract_commands_from_word(word, commands); + } + } + } +} + +fn extract_commands_from_word(word: &ast::Word, commands: &mut Vec) { + let options = ParserOptions::default(); + if let Ok(pieces) = brush_parser::word::parse(&word.value, &options) { + for piece_with_source in pieces { + extract_commands_from_word_piece(&piece_with_source.piece, commands); + } + } +} + +fn extract_commands_from_word_piece(piece: &WordPiece, commands: &mut Vec) { + match piece { + WordPiece::CommandSubstitution(cmd_str) + | WordPiece::BackquotedCommandSubstitution(cmd_str) => { + if let Some(nested_commands) = extract_commands(cmd_str) { + commands.extend(nested_commands); + } + } + WordPiece::DoubleQuotedSequence(pieces) + | WordPiece::GettextDoubleQuotedSequence(pieces) => { + for inner_piece_with_source in pieces { + extract_commands_from_word_piece(&inner_piece_with_source.piece, commands); + } + } + WordPiece::EscapeSequence(_) + | WordPiece::SingleQuotedText(_) + | WordPiece::Text(_) + | WordPiece::AnsiCQuotedText(_) + | WordPiece::TildePrefix(_) + | WordPiece::ParameterExpansion(_) + | WordPiece::ArithmeticExpression(_) => {} + } +} + +fn extract_commands_from_compound_command( + compound_command: &ast::CompoundCommand, + commands: &mut Vec, +) { + match compound_command { + ast::CompoundCommand::BraceGroup(brace_group) => { + extract_commands_from_compound_list(&brace_group.list, commands); + } + ast::CompoundCommand::Subshell(subshell) => { + extract_commands_from_compound_list(&subshell.list, commands); + } + ast::CompoundCommand::ForClause(for_clause) => { + if let Some(words) = &for_clause.values { + for word in words { + extract_commands_from_word(word, commands); + } + } + extract_commands_from_do_group(&for_clause.body, commands); + } + ast::CompoundCommand::CaseClause(case_clause) => { + extract_commands_from_word(&case_clause.value, commands); + for item in &case_clause.cases { + if let Some(body) = &item.cmd { + extract_commands_from_compound_list(body, commands); + } + } + } + ast::CompoundCommand::IfClause(if_clause) => { + extract_commands_from_compound_list(&if_clause.condition, commands); + extract_commands_from_compound_list(&if_clause.then, commands); + if let Some(elses) = &if_clause.elses { + for else_item in elses { + if let Some(condition) = &else_item.condition { + extract_commands_from_compound_list(condition, commands); + } + extract_commands_from_compound_list(&else_item.body, commands); + } + } + } + ast::CompoundCommand::WhileClause(while_clause) + | ast::CompoundCommand::UntilClause(while_clause) => { + extract_commands_from_compound_list(&while_clause.0, commands); + extract_commands_from_do_group(&while_clause.1, commands); + } + ast::CompoundCommand::ArithmeticForClause(arith_for) => { + extract_commands_from_do_group(&arith_for.body, commands); + } + ast::CompoundCommand::Arithmetic(_arith_cmd) => {} + } +} + +fn extract_commands_from_do_group(do_group: &ast::DoGroupCommand, commands: &mut Vec) { + extract_commands_from_compound_list(&do_group.list, commands); +} + +fn extract_commands_from_function_body(func_body: &ast::FunctionBody, commands: &mut Vec) { + extract_commands_from_compound_command(&func_body.0, commands); +} + +fn extract_commands_from_extended_test_expr( + test_expr: &ast::ExtendedTestExprCommand, + commands: &mut Vec, +) { + extract_commands_from_extended_test_expr_inner(&test_expr.expr, commands); +} + +fn extract_commands_from_extended_test_expr_inner( + expr: &ast::ExtendedTestExpr, + commands: &mut Vec, +) { + match expr { + ast::ExtendedTestExpr::Not(inner) => { + extract_commands_from_extended_test_expr_inner(inner, commands); + } + ast::ExtendedTestExpr::And(left, right) | ast::ExtendedTestExpr::Or(left, right) => { + extract_commands_from_extended_test_expr_inner(left, commands); + extract_commands_from_extended_test_expr_inner(right, commands); + } + ast::ExtendedTestExpr::Parenthesized(inner) => { + extract_commands_from_extended_test_expr_inner(inner, commands); + } + ast::ExtendedTestExpr::UnaryTest(_, word) => { + extract_commands_from_word(word, commands); + } + ast::ExtendedTestExpr::BinaryTest(_, word1, word2) => { + extract_commands_from_word(word1, commands); + extract_commands_from_word(word2, commands); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_simple_command() { + let commands = extract_commands("ls").expect("parse failed"); + assert_eq!(commands, vec!["ls"]); + } + + #[test] + fn test_command_with_args() { + let commands = extract_commands("ls -la /tmp").expect("parse failed"); + assert_eq!(commands, vec!["ls -la /tmp"]); + } + + #[test] + fn test_and_operator() { + let commands = extract_commands("ls && rm -rf /").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm -rf /"]); + } + + #[test] + fn test_or_operator() { + let commands = extract_commands("ls || rm -rf /").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm -rf /"]); + } + + #[test] + fn test_semicolon() { + let commands = extract_commands("ls; rm -rf /").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm -rf /"]); + } + + #[test] + fn test_pipe() { + let commands = extract_commands("ls | xargs rm -rf").expect("parse failed"); + assert_eq!(commands, vec!["ls", "xargs rm -rf"]); + } + + #[test] + fn test_background() { + let commands = extract_commands("ls & rm -rf /").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm -rf /"]); + } + + #[test] + fn test_command_substitution_dollar() { + let commands = extract_commands("echo $(whoami)").expect("parse failed"); + assert!(commands.iter().any(|c| c.contains("echo"))); + assert!(commands.contains(&"whoami".to_string())); + } + + #[test] + fn test_command_substitution_backticks() { + let commands = extract_commands("echo `whoami`").expect("parse failed"); + assert!(commands.iter().any(|c| c.contains("echo"))); + assert!(commands.contains(&"whoami".to_string())); + } + + #[test] + fn test_process_substitution_input() { + let commands = extract_commands("cat <(ls)").expect("parse failed"); + assert!(commands.iter().any(|c| c.contains("cat"))); + assert!(commands.contains(&"ls".to_string())); + } + + #[test] + fn test_process_substitution_output() { + let commands = extract_commands("ls >(cat)").expect("parse failed"); + assert!(commands.iter().any(|c| c.contains("ls"))); + assert!(commands.contains(&"cat".to_string())); + } + + #[test] + fn test_newline_separator() { + let commands = extract_commands("ls\nrm -rf /").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm -rf /"]); + } + + #[test] + fn test_subshell() { + let commands = extract_commands("(ls && rm -rf /)").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm -rf /"]); + } + + #[test] + fn test_mixed_operators() { + let commands = extract_commands("ls; echo hello && rm -rf /").expect("parse failed"); + assert_eq!(commands, vec!["ls", "echo hello", "rm -rf /"]); + } + + #[test] + fn test_no_spaces_around_operators() { + let commands = extract_commands("ls&&rm").expect("parse failed"); + assert_eq!(commands, vec!["ls", "rm"]); + } + + #[test] + fn test_nested_command_substitution() { + let commands = extract_commands("echo $(cat $(whoami).txt)").expect("parse failed"); + assert!(commands.iter().any(|c| c.contains("echo"))); + assert!(commands.iter().any(|c| c.contains("cat"))); + assert!(commands.contains(&"whoami".to_string())); + } + + #[test] + fn test_empty_command() { + let commands = extract_commands("").expect("parse failed"); + assert!(commands.is_empty()); + } + + #[test] + fn test_invalid_syntax_returns_none() { + let result = extract_commands("ls &&"); + assert!(result.is_none()); + } +} diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 05ecdb83517b5e00c7b4ddf2caf8d9083917c329..f9c2a761b18f5f8aa68d0274fae5dec55da3ef63 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -624,26 +624,63 @@ impl ToolPermissionContext { /// /// This is the canonical source for permission option generation. /// Tests should use this function rather than manually constructing options. + /// + /// # Shell Compatibility for Terminal Tool + /// + /// For the terminal tool, "Always allow" options are only shown when the user's + /// shell supports POSIX-like command chaining syntax (`&&`, `||`, `;`, `|`). + /// + /// **Why this matters:** When a user sets up an "always allow" pattern like `^cargo`, + /// we need to parse the command to extract all sub-commands and verify that EVERY + /// sub-command matches the pattern. Otherwise, an attacker could craft a command like + /// `cargo build && rm -rf /` that would bypass the security check. + /// + /// **Supported shells:** Posix (sh, bash, dash, zsh), Fish 3.0+, PowerShell 7+/Pwsh, + /// Cmd, Xonsh, Csh, Tcsh + /// + /// **Unsupported shells:** Nushell (uses `and`/`or` keywords), Elvish (uses `and`/`or` + /// keywords), Rc (Plan 9 shell - no `&&`/`||` operators) + /// + /// For unsupported shells, we hide the "Always allow" UI options entirely, and if + /// the user has `always_allow` rules configured in settings, `ToolPermissionDecision::from_input` + /// will return a `Deny` with an explanatory error message. pub fn build_permission_options(&self) -> acp_thread::PermissionOptions { use crate::pattern_extraction::*; + use util::shell::ShellKind; let tool_name = &self.tool_name; let input_value = &self.input_value; - let (pattern, pattern_display) = match tool_name.as_str() { - "terminal" => ( + // Check if the user's shell supports POSIX-like command chaining. + // See the doc comment above for the full explanation of why this is needed. + let shell_supports_always_allow = if tool_name == TerminalTool::name() { + ShellKind::system().supports_posix_chaining() + } else { + true + }; + + let (pattern, pattern_display) = if tool_name == TerminalTool::name() { + ( extract_terminal_pattern(input_value), extract_terminal_pattern_display(input_value), - ), - "edit_file" | "delete_path" | "move_path" | "create_directory" | "save_file" => ( + ) + } else if tool_name == EditFileTool::name() + || tool_name == DeletePathTool::name() + || tool_name == MovePathTool::name() + || tool_name == CreateDirectoryTool::name() + || tool_name == SaveFileTool::name() + { + ( extract_path_pattern(input_value), extract_path_pattern_display(input_value), - ), - "fetch" => ( + ) + } else if tool_name == FetchTool::name() { + ( extract_url_pattern(input_value), extract_url_pattern_display(input_value), - ), - _ => (None, None), + ) + } else { + (None, None) }; let mut choices = Vec::new(); @@ -663,27 +700,29 @@ impl ToolPermissionContext { }); }; - push_choice( - format!("Always for {}", tool_name.replace('_', " ")), - format!("always_allow:{}", tool_name), - format!("always_deny:{}", tool_name), - acp::PermissionOptionKind::AllowAlways, - acp::PermissionOptionKind::RejectAlways, - ); - - if let (Some(pattern), Some(display)) = (pattern, pattern_display) { - let button_text = match tool_name.as_str() { - "terminal" => format!("Always for `{}` commands", display), - "fetch" => format!("Always for `{}`", display), - _ => format!("Always for `{}`", display), - }; + if shell_supports_always_allow { push_choice( - button_text, - format!("always_allow_pattern:{}:{}", tool_name, pattern), - format!("always_deny_pattern:{}:{}", tool_name, pattern), + format!("Always for {}", tool_name.replace('_', " ")), + format!("always_allow:{}", tool_name), + format!("always_deny:{}", tool_name), acp::PermissionOptionKind::AllowAlways, acp::PermissionOptionKind::RejectAlways, ); + + if let (Some(pattern), Some(display)) = (pattern, pattern_display) { + let button_text = if tool_name == TerminalTool::name() { + format!("Always for `{}` commands", display) + } else { + format!("Always for `{}`", display) + }; + push_choice( + button_text, + format!("always_allow_pattern:{}:{}", tool_name, pattern), + format!("always_deny_pattern:{}:{}", tool_name, pattern), + acp::PermissionOptionKind::AllowAlways, + acp::PermissionOptionKind::RejectAlways, + ); + } } push_choice( diff --git a/crates/agent/src/tool_permissions.rs b/crates/agent/src/tool_permissions.rs index 6ae4d14721f73c19ea08135f8990935c6589c7b3..f747aa56eb36f3b73ff8c8ba2450e7f6e843290a 100644 --- a/crates/agent/src/tool_permissions.rs +++ b/crates/agent/src/tool_permissions.rs @@ -1,5 +1,9 @@ +use crate::AgentTool; +use crate::shell_parser::extract_commands; +use crate::tools::TerminalTool; use agent_settings::{AgentSettings, ToolPermissions, ToolRules}; use settings::ToolPermissionMode; +use util::shell::ShellKind; #[derive(Debug, Clone, PartialEq, Eq)] pub enum ToolPermissionDecision { @@ -8,76 +12,169 @@ pub enum ToolPermissionDecision { Confirm, } -/// Determines the permission decision for a tool invocation based on configured rules. -/// -/// # Precedence Order (highest to lowest) -/// -/// 1. **`always_allow_tool_actions`** - When enabled, allows all tool actions except those -/// blocked by `always_deny` patterns. This global setting takes precedence over -/// `always_confirm` patterns and `default_mode`. -/// 2. **`always_deny`** - If any deny pattern matches, the tool call is blocked immediately. -/// This takes precedence over all other rules for security (including `always_allow_tool_actions`). -/// 3. **`always_confirm`** - If any confirm pattern matches (and no deny matched), -/// the user is prompted for confirmation (unless `always_allow_tool_actions` is enabled). -/// 4. **`always_allow`** - If any allow pattern matches (and no deny/confirm matched), -/// the tool call proceeds without prompting. -/// 5. **`default_mode`** - If no patterns match, falls back to the tool's default mode. -/// -/// # Pattern Matching Tips +impl ToolPermissionDecision { + /// Determines the permission decision for a tool invocation based on configured rules. + /// + /// # Precedence Order (highest to lowest) + /// + /// 1. **`always_allow_tool_actions`** - When enabled, allows all tool actions without + /// prompting. This global setting bypasses all other checks including deny patterns. + /// Use with caution as it disables all security rules. + /// 2. **`always_deny`** - If any deny pattern matches, the tool call is blocked immediately. + /// This takes precedence over `always_confirm` and `always_allow` patterns. + /// 3. **`always_confirm`** - If any confirm pattern matches (and no deny matched), + /// the user is prompted for confirmation. + /// 4. **`always_allow`** - If any allow pattern matches (and no deny/confirm matched), + /// the tool call proceeds without prompting. + /// 5. **`default_mode`** - If no patterns match, falls back to the tool's default mode. + /// + /// # Shell Compatibility (Terminal Tool Only) + /// + /// For the terminal tool, commands are parsed to extract sub-commands for security. + /// This parsing only works for shells with POSIX-like `&&` / `||` / `;` / `|` syntax: + /// + /// **Compatible shells:** Posix (sh, bash, dash, zsh), Fish 3.0+, PowerShell 7+/Pwsh, + /// Cmd, Xonsh, Csh, Tcsh + /// + /// **Incompatible shells:** Nushell, Elvish, Rc (Plan 9) + /// + /// For incompatible shells, `always_allow` patterns are disabled for safety. + /// + /// # Pattern Matching Tips + /// + /// Patterns are matched as regular expressions against the tool input (e.g., the command + /// string for the terminal tool). Some tips for writing effective patterns: + /// + /// - Use word boundaries (`\b`) to avoid partial matches. For example, pattern `rm` will + /// match "storm" and "arms", but `\brm\b` will only match the standalone word "rm". + /// This is important for security rules where you want to block specific commands + /// without accidentally blocking unrelated commands that happen to contain the same + /// substring. + /// - Patterns are case-insensitive by default. Set `case_sensitive: true` for exact matching. + /// - Use `^` and `$` anchors to match the start/end of the input. + pub fn from_input( + tool_name: &str, + input: &str, + permissions: &ToolPermissions, + always_allow_tool_actions: bool, + shell_kind: ShellKind, + ) -> ToolPermissionDecision { + // If always_allow_tool_actions is enabled, bypass all permission checks. + // This is intentionally placed first - it's a global override that the user + // must explicitly enable, understanding that it bypasses all security rules. + if always_allow_tool_actions { + return ToolPermissionDecision::Allow; + } + + let rules = match permissions.tools.get(tool_name) { + Some(rules) => rules, + None => { + return ToolPermissionDecision::Confirm; + } + }; + + // Check for invalid regex patterns before evaluating rules. + // If any patterns failed to compile, block the tool call entirely. + if let Some(error) = check_invalid_patterns(tool_name, rules) { + return ToolPermissionDecision::Deny(error); + } + + // For the terminal tool, parse the command to extract all sub-commands. + // This prevents shell injection attacks where a user configures an allow + // pattern like "^ls" and an attacker crafts "ls && rm -rf /". + // + // If parsing fails or the shell syntax is unsupported, always_allow is + // disabled for this command (we set allow_enabled to false to signal this). + if tool_name == TerminalTool::name() { + // Our shell parser (brush-parser) only supports POSIX-like shell syntax. + // See the doc comment above for the list of compatible/incompatible shells. + if !shell_kind.supports_posix_chaining() { + // For shells with incompatible syntax, we can't reliably parse + // the command to extract sub-commands. + if !rules.always_allow.is_empty() { + // If the user has configured always_allow patterns, we must deny + // because we can't safely verify the command doesn't contain + // hidden sub-commands that bypass the allow patterns. + return ToolPermissionDecision::Deny(format!( + "The {} shell does not support \"always allow\" patterns for the terminal \ + tool because Zed cannot parse its command chaining syntax. Please remove \ + the always_allow patterns from your tool_permissions settings, or switch \ + to a POSIX-conforming shell.", + shell_kind + )); + } + // No always_allow rules, so we can still check deny/confirm patterns. + return check_commands(std::iter::once(input.to_string()), rules, tool_name, false); + } + + match extract_commands(input) { + Some(commands) => check_commands(commands, rules, tool_name, true), + None => { + // The command failed to parse, so we check to see if we should auto-deny + // or auto-confirm; if neither auto-deny nor auto-confirm applies here, + // fall back on the default (based on the user's settings, which is Confirm + // if not specified otherwise). Ignore "always allow" when it failed to parse. + check_commands(std::iter::once(input.to_string()), rules, tool_name, false) + } + } + } else { + check_commands(std::iter::once(input.to_string()), rules, tool_name, true) + } + } +} + +/// Evaluates permission rules against a set of commands. /// -/// Patterns are matched as regular expressions against the tool input (e.g., the command -/// string for the terminal tool). Some tips for writing effective patterns: +/// This function performs a single pass through all commands with the following logic: +/// - **DENY**: If ANY command matches a deny pattern, deny immediately (short-circuit) +/// - **CONFIRM**: Track if ANY command matches a confirm pattern +/// - **ALLOW**: Track if ALL commands match at least one allow pattern /// -/// - Use word boundaries (`\b`) to avoid partial matches. For example, pattern `rm` will -/// match "storm" and "arms", but `\brm\b` will only match the standalone word "rm". -/// This is important for security rules where you want to block specific commands -/// without accidentally blocking unrelated commands that happen to contain the same -/// substring. -/// - Patterns are case-insensitive by default. Set `case_sensitive: true` for exact matching. -/// - Use `^` and `$` anchors to match the start/end of the input. -pub fn decide_permission( +/// The `allow_enabled` flag controls whether allow patterns are checked. This is set +/// to `false` when we can't reliably parse shell commands (e.g., parse failures or +/// unsupported shell syntax), ensuring we don't auto-allow potentially dangerous commands. +fn check_commands( + commands: impl IntoIterator, + rules: &ToolRules, tool_name: &str, - input: &str, - permissions: &ToolPermissions, - always_allow_tool_actions: bool, + allow_enabled: bool, ) -> ToolPermissionDecision { - let rules = permissions.tools.get(tool_name); - - let rules = match rules { - Some(rules) => rules, - None => { - return if always_allow_tool_actions { - ToolPermissionDecision::Allow - } else { - ToolPermissionDecision::Confirm - }; + // Single pass through all commands: + // - DENY: If ANY command matches a deny pattern, deny immediately (short-circuit) + // - CONFIRM: Track if ANY command matches a confirm pattern + // - ALLOW: Track if ALL commands match at least one allow pattern + let mut any_matched_confirm = false; + let mut all_matched_allow = true; + let mut had_any_commands = false; + + for command in commands { + had_any_commands = true; + + // DENY: immediate return if any command matches a deny pattern + if rules.always_deny.iter().any(|r| r.is_match(&command)) { + return ToolPermissionDecision::Deny(format!( + "Command blocked by security rule for {} tool", + tool_name + )); } - }; - - // Check for invalid regex patterns before evaluating rules. - // If any patterns failed to compile, block the tool call entirely. - if let Some(error) = check_invalid_patterns(tool_name, rules) { - return ToolPermissionDecision::Deny(error); - } - if rules.always_deny.iter().any(|r| r.is_match(input)) { - return ToolPermissionDecision::Deny(format!( - "Command blocked by security rule for {} tool", - tool_name - )); - } + // CONFIRM: remember if any command matches a confirm pattern + if rules.always_confirm.iter().any(|r| r.is_match(&command)) { + any_matched_confirm = true; + } - if rules.always_confirm.iter().any(|r| r.is_match(input)) { - if !always_allow_tool_actions { - return ToolPermissionDecision::Confirm; + // ALLOW: track if all commands match at least one allow pattern + if !rules.always_allow.iter().any(|r| r.is_match(&command)) { + all_matched_allow = false; } } - if rules.always_allow.iter().any(|r| r.is_match(input)) { - return ToolPermissionDecision::Allow; + // After processing all commands, check accumulated state + if any_matched_confirm { + return ToolPermissionDecision::Confirm; } - if always_allow_tool_actions { + if allow_enabled && all_matched_allow && had_any_commands { return ToolPermissionDecision::Allow; } @@ -111,34 +208,45 @@ fn check_invalid_patterns(tool_name: &str, rules: &ToolRules) -> Option /// /// This is the primary entry point for tools to check permissions. It extracts /// `tool_permissions` and `always_allow_tool_actions` from the settings and -/// delegates to [`decide_permission`]. +/// delegates to [`ToolPermissionDecision::from_input`], using the system shell. pub fn decide_permission_from_settings( tool_name: &str, input: &str, settings: &AgentSettings, ) -> ToolPermissionDecision { - decide_permission( + ToolPermissionDecision::from_input( tool_name, input, &settings.tool_permissions, settings.always_allow_tool_actions, + ShellKind::system(), ) } #[cfg(test)] mod tests { use super::*; + use crate::pattern_extraction::extract_terminal_pattern; use agent_settings::{CompiledRegex, InvalidRegexPattern, ToolRules}; use std::sync::Arc; + fn pattern(command: &str) -> &'static str { + Box::leak( + extract_terminal_pattern(command) + .expect("failed to extract pattern") + .into_boxed_str(), + ) + } + struct PermTest { tool: &'static str, input: &'static str, mode: ToolPermissionMode, - allow: Vec<&'static str>, - deny: Vec<&'static str>, - confirm: Vec<&'static str>, + allow: Vec<(&'static str, bool)>, + deny: Vec<(&'static str, bool)>, + confirm: Vec<(&'static str, bool)>, global: bool, + shell: ShellKind, } impl PermTest { @@ -151,6 +259,7 @@ mod tests { deny: vec![], confirm: vec![], global: false, + shell: ShellKind::Posix, } } @@ -163,21 +272,33 @@ mod tests { self } fn allow(mut self, p: &[&'static str]) -> Self { - self.allow = p.to_vec(); + self.allow = p.iter().map(|s| (*s, false)).collect(); + self + } + fn allow_case_sensitive(mut self, p: &[&'static str]) -> Self { + self.allow = p.iter().map(|s| (*s, true)).collect(); self } fn deny(mut self, p: &[&'static str]) -> Self { - self.deny = p.to_vec(); + self.deny = p.iter().map(|s| (*s, false)).collect(); + self + } + fn deny_case_sensitive(mut self, p: &[&'static str]) -> Self { + self.deny = p.iter().map(|s| (*s, true)).collect(); self } fn confirm(mut self, p: &[&'static str]) -> Self { - self.confirm = p.to_vec(); + self.confirm = p.iter().map(|s| (*s, false)).collect(); self } fn global(mut self, g: bool) -> Self { self.global = g; self } + fn shell(mut self, s: ShellKind) -> Self { + self.shell = s; + self + } fn is_allow(self) { assert_eq!( @@ -212,26 +333,27 @@ mod tests { always_allow: self .allow .iter() - .filter_map(|p| CompiledRegex::new(p, false)) + .filter_map(|(p, cs)| CompiledRegex::new(p, *cs)) .collect(), always_deny: self .deny .iter() - .filter_map(|p| CompiledRegex::new(p, false)) + .filter_map(|(p, cs)| CompiledRegex::new(p, *cs)) .collect(), always_confirm: self .confirm .iter() - .filter_map(|p| CompiledRegex::new(p, false)) + .filter_map(|(p, cs)| CompiledRegex::new(p, *cs)) .collect(), invalid_patterns: vec![], }, ); - decide_permission( + ToolPermissionDecision::from_input( self.tool, self.input, &ToolPermissions { tools }, self.global, + self.shell, ) } } @@ -241,28 +363,30 @@ mod tests { } fn no_rules(input: &str, global: bool) -> ToolPermissionDecision { - decide_permission( + ToolPermissionDecision::from_input( "terminal", input, &ToolPermissions { tools: collections::HashMap::default(), }, global, + ShellKind::Posix, ) } // allow pattern matches #[test] fn allow_exact_match() { - t("cargo test").allow(&["^cargo\\s"]).is_allow(); - } - #[test] - fn allow_with_args() { - t("cargo build --release").allow(&["^cargo\\s"]).is_allow(); + t("cargo test").allow(&[pattern("cargo")]).is_allow(); } #[test] - fn allow_one_of_many() { - t("npm install").allow(&["^cargo\\s", "^npm\\s"]).is_allow(); + fn allow_one_of_many_patterns() { + t("npm install") + .allow(&[pattern("cargo"), pattern("npm")]) + .is_allow(); + t("git status") + .allow(&[pattern("cargo"), pattern("npm"), pattern("git")]) + .is_allow(); } #[test] fn allow_middle_pattern() { @@ -276,12 +400,12 @@ mod tests { // allow pattern doesn't match -> falls through #[test] fn allow_no_match_confirms() { - t("python x.py").allow(&["^cargo\\s"]).is_confirm(); + t("python x.py").allow(&[pattern("cargo")]).is_confirm(); } #[test] fn allow_no_match_global_allows() { t("python x.py") - .allow(&["^cargo\\s"]) + .allow(&[pattern("cargo")]) .global(true) .is_allow(); } @@ -292,8 +416,9 @@ mod tests { t("rm -rf /").deny(&["rm\\s+-rf"]).is_deny(); } #[test] - fn deny_blocks_with_global() { - t("rm -rf /").deny(&["rm\\s+-rf"]).global(true).is_deny(); + fn global_bypasses_deny() { + // always_allow_tool_actions bypasses ALL checks, including deny + t("rm -rf /").deny(&["rm\\s+-rf"]).global(true).is_allow(); } #[test] fn deny_blocks_with_mode_allow() { @@ -307,19 +432,24 @@ mod tests { t("echo rm -rf x").deny(&["rm\\s+-rf"]).is_deny(); } #[test] - fn deny_no_match_allows() { - t("ls -la").deny(&["rm\\s+-rf"]).global(true).is_allow(); + fn deny_no_match_falls_through() { + t("ls -la") + .deny(&["rm\\s+-rf"]) + .mode(ToolPermissionMode::Allow) + .is_allow(); } // confirm pattern matches #[test] fn confirm_requires_confirm() { - t("sudo apt install").confirm(&["sudo\\s"]).is_confirm(); + t("sudo apt install") + .confirm(&[pattern("sudo")]) + .is_confirm(); } #[test] fn global_overrides_confirm() { t("sudo reboot") - .confirm(&["sudo\\s"]) + .confirm(&[pattern("sudo")]) .global(true) .is_allow(); } @@ -335,7 +465,7 @@ mod tests { #[test] fn confirm_beats_allow() { t("git push --force") - .allow(&["^git\\s"]) + .allow(&[pattern("git")]) .confirm(&["--force"]) .is_confirm(); } @@ -349,7 +479,7 @@ mod tests { #[test] fn allow_when_confirm_no_match() { t("git status") - .allow(&["^git\\s"]) + .allow(&[pattern("git")]) .confirm(&["--force"]) .is_allow(); } @@ -362,12 +492,7 @@ mod tests { .deny(&["rm\\s+-rf"]) .is_deny(); } - #[test] - fn deny_beats_allow_diff() { - t("bad deploy").allow(&["deploy"]).deny(&["bad"]).is_deny(); - } - // deny beats confirm #[test] fn deny_beats_confirm() { t("sudo rm -rf /") @@ -409,14 +534,6 @@ mod tests { .is_allow(); } - // default_mode confirm + global - #[test] - fn default_confirm_global_false() { - t("x") - .mode(ToolPermissionMode::Confirm) - .global(false) - .is_confirm(); - } #[test] fn default_confirm_global_true() { t("x") @@ -425,44 +542,35 @@ mod tests { .is_allow(); } - // no rules at all -> global setting #[test] - fn no_rules_global_false() { + fn no_rules_confirms_by_default() { assert_eq!(no_rules("x", false), ToolPermissionDecision::Confirm); } - #[test] - fn no_rules_global_true() { - assert_eq!(no_rules("x", true), ToolPermissionDecision::Allow); - } - // empty input #[test] fn empty_input_no_match() { - t("").deny(&["rm"]).is_confirm(); + t("") + .deny(&["rm"]) + .mode(ToolPermissionMode::Allow) + .is_allow(); } + #[test] - fn empty_input_global() { - t("").deny(&["rm"]).global(true).is_allow(); + fn empty_input_with_allow_falls_to_default() { + t("").allow(&["^ls"]).is_confirm(); } - // multiple patterns - any match #[test] - fn multi_deny_first() { + fn multi_deny_any_match() { t("rm x").deny(&["rm", "del", "drop"]).is_deny(); - } - #[test] - fn multi_deny_last() { t("drop x").deny(&["rm", "del", "drop"]).is_deny(); } + #[test] - fn multi_allow_first() { + fn multi_allow_any_match() { t("cargo x").allow(&["^cargo", "^npm", "^git"]).is_allow(); } #[test] - fn multi_allow_last() { - t("git x").allow(&["^cargo", "^npm", "^git"]).is_allow(); - } - #[test] fn multi_none_match() { t("python x") .allow(&["^cargo", "^npm"]) @@ -497,16 +605,16 @@ mod tests { let p = ToolPermissions { tools }; // With always_allow_tool_actions=true, even default_mode: Deny is overridden assert_eq!( - decide_permission("terminal", "x", &p, true), + ToolPermissionDecision::from_input("terminal", "x", &p, true, ShellKind::Posix), ToolPermissionDecision::Allow ); // With always_allow_tool_actions=false, default_mode: Deny is respected assert!(matches!( - decide_permission("terminal", "x", &p, false), + ToolPermissionDecision::from_input("terminal", "x", &p, false, ShellKind::Posix), ToolPermissionDecision::Deny(_) )); assert_eq!( - decide_permission("edit_file", "x", &p, false), + ToolPermissionDecision::from_input("edit_file", "x", &p, false, ShellKind::Posix), ToolPermissionDecision::Allow ); } @@ -525,13 +633,14 @@ mod tests { }, ); let p = ToolPermissions { tools }; + // "terminal" should not match "term" rules, so falls back to Confirm (no rules) assert_eq!( - decide_permission("terminal", "x", &p, true), - ToolPermissionDecision::Allow + ToolPermissionDecision::from_input("terminal", "x", &p, false, ShellKind::Posix), + ToolPermissionDecision::Confirm ); } - // invalid patterns block the tool + // invalid patterns block the tool (but global bypasses all checks) #[test] fn invalid_pattern_blocks() { let mut tools = collections::HashMap::default(); @@ -549,51 +658,173 @@ mod tests { }], }, ); - let p = ToolPermissions { tools }; + let p = ToolPermissions { + tools: tools.clone(), + }; + // With global=true, all checks are bypassed including invalid pattern check + assert!(matches!( + ToolPermissionDecision::from_input("terminal", "echo hi", &p, true, ShellKind::Posix), + ToolPermissionDecision::Allow + )); + // With global=false, invalid patterns block the tool assert!(matches!( - decide_permission("terminal", "echo hi", &p, true), + ToolPermissionDecision::from_input("terminal", "echo hi", &p, false, ShellKind::Posix), ToolPermissionDecision::Deny(_) )); } - // user scenario: only echo allowed, git should confirm #[test] - fn user_scenario_only_echo() { - t("echo hello").allow(&["^echo\\s"]).is_allow(); + fn shell_injection_via_double_ampersand_not_allowed() { + t("ls && rm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_via_semicolon_not_allowed() { + t("ls; rm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_via_pipe_not_allowed() { + t("ls | xargs rm -rf").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_via_backticks_not_allowed() { + t("echo `rm -rf /`").allow(&[pattern("echo")]).is_confirm(); + } + + #[test] + fn shell_injection_via_dollar_parens_not_allowed() { + t("echo $(rm -rf /)").allow(&[pattern("echo")]).is_confirm(); + } + + #[test] + fn shell_injection_via_or_operator_not_allowed() { + t("ls || rm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_via_background_operator_not_allowed() { + t("ls & rm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_via_newline_not_allowed() { + t("ls\nrm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_via_process_substitution_input_not_allowed() { + t("cat <(rm -rf /)").allow(&["^cat"]).is_confirm(); + } + + #[test] + fn shell_injection_via_process_substitution_output_not_allowed() { + t("ls >(rm -rf /)").allow(&["^ls"]).is_confirm(); } + #[test] - fn user_scenario_git_confirms() { - t("git status").allow(&["^echo\\s"]).is_confirm(); + fn shell_injection_without_spaces_not_allowed() { + t("ls&&rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls;rm -rf /").allow(&["^ls"]).is_confirm(); } + #[test] - fn user_scenario_rm_confirms() { - t("rm -rf /").allow(&["^echo\\s"]).is_confirm(); + fn shell_injection_multiple_chained_operators_not_allowed() { + t("ls && echo hello && rm -rf /") + .allow(&["^ls"]) + .is_confirm(); } - // mcp tools #[test] - fn mcp_allow() { + fn shell_injection_mixed_operators_not_allowed() { + t("ls; echo hello && rm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn shell_injection_pipe_stderr_not_allowed() { + t("ls |& rm -rf /").allow(&["^ls"]).is_confirm(); + } + + #[test] + fn allow_requires_all_commands_to_match() { + t("ls && echo hello").allow(&["^ls", "^echo"]).is_allow(); + } + + #[test] + fn deny_triggers_on_any_matching_command() { + t("ls && rm file").allow(&["^ls"]).deny(&["^rm"]).is_deny(); + } + + #[test] + fn deny_catches_injected_command() { + t("ls && rm -rf /").allow(&["^ls"]).deny(&["^rm"]).is_deny(); + } + + #[test] + fn confirm_triggers_on_any_matching_command() { + t("ls && sudo reboot") + .allow(&["^ls"]) + .confirm(&["^sudo"]) + .is_confirm(); + } + + #[test] + fn always_allow_button_works_end_to_end() { + // This test verifies that the "Always Allow" button behavior works correctly: + // 1. User runs a command like "cargo build" + // 2. They click "Always Allow for `cargo` commands" + // 3. The pattern extracted from that command should match future cargo commands + let original_command = "cargo build --release"; + let extracted_pattern = pattern(original_command); + + // The extracted pattern should allow the original command + t(original_command).allow(&[extracted_pattern]).is_allow(); + + // It should also allow other commands with the same base command + t("cargo test").allow(&[extracted_pattern]).is_allow(); + t("cargo fmt").allow(&[extracted_pattern]).is_allow(); + + // But not commands with different base commands + t("npm install").allow(&[extracted_pattern]).is_confirm(); + + // And it should work with subcommand extraction (chained commands) + t("cargo build && cargo test") + .allow(&[extracted_pattern]) + .is_allow(); + + // But reject if any subcommand doesn't match + t("cargo build && npm install") + .allow(&[extracted_pattern]) + .is_confirm(); + } + + #[test] + fn nested_command_substitution_all_checked() { + t("echo $(cat $(whoami).txt)") + .allow(&["^echo", "^cat", "^whoami"]) + .is_allow(); + } + + #[test] + fn parse_failure_falls_back_to_confirm() { + t("ls &&").allow(&["^ls$"]).is_confirm(); + } + + #[test] + fn mcp_tool_default_modes() { t("") .tool("mcp:fs:read") .mode(ToolPermissionMode::Allow) .is_allow(); - } - #[test] - fn mcp_deny() { t("") .tool("mcp:bad:del") .mode(ToolPermissionMode::Deny) .is_deny(); - } - #[test] - fn mcp_confirm() { t("") .tool("mcp:gh:issue") .mode(ToolPermissionMode::Confirm) .is_confirm(); - } - #[test] - fn mcp_confirm_global() { t("") .tool("mcp:gh:issue") .mode(ToolPermissionMode::Confirm) @@ -601,7 +832,6 @@ mod tests { .is_allow(); } - // mcp vs builtin isolation #[test] fn mcp_doesnt_collide_with_builtin() { let mut tools = collections::HashMap::default(); @@ -627,12 +857,116 @@ mod tests { ); let p = ToolPermissions { tools }; assert!(matches!( - decide_permission("terminal", "x", &p, false), + ToolPermissionDecision::from_input("terminal", "x", &p, false, ShellKind::Posix), ToolPermissionDecision::Deny(_) )); assert_eq!( - decide_permission("mcp:srv:terminal", "x", &p, false), + ToolPermissionDecision::from_input( + "mcp:srv:terminal", + "x", + &p, + false, + ShellKind::Posix + ), ToolPermissionDecision::Allow ); } + + #[test] + fn case_insensitive_by_default() { + t("CARGO TEST").allow(&[pattern("cargo")]).is_allow(); + t("Cargo Test").allow(&[pattern("cargo")]).is_allow(); + } + + #[test] + fn case_sensitive_allow() { + t("cargo test") + .allow_case_sensitive(&[pattern("cargo")]) + .is_allow(); + t("CARGO TEST") + .allow_case_sensitive(&[pattern("cargo")]) + .is_confirm(); + } + + #[test] + fn case_sensitive_deny() { + t("rm -rf /") + .deny_case_sensitive(&[pattern("rm")]) + .is_deny(); + t("RM -RF /") + .deny_case_sensitive(&[pattern("rm")]) + .mode(ToolPermissionMode::Allow) + .is_allow(); + } + + #[test] + fn nushell_denies_when_always_allow_configured() { + t("ls").allow(&["^ls"]).shell(ShellKind::Nushell).is_deny(); + } + + #[test] + fn nushell_allows_deny_patterns() { + t("rm -rf /") + .deny(&["rm\\s+-rf"]) + .shell(ShellKind::Nushell) + .is_deny(); + } + + #[test] + fn nushell_allows_confirm_patterns() { + t("sudo reboot") + .confirm(&["sudo"]) + .shell(ShellKind::Nushell) + .is_confirm(); + } + + #[test] + fn nushell_no_allow_patterns_uses_default() { + t("ls") + .deny(&["rm"]) + .mode(ToolPermissionMode::Allow) + .shell(ShellKind::Nushell) + .is_allow(); + } + + #[test] + fn elvish_denies_when_always_allow_configured() { + t("ls").allow(&["^ls"]).shell(ShellKind::Elvish).is_deny(); + } + + #[test] + fn multiple_invalid_patterns_pluralizes_message() { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![ + InvalidRegexPattern { + pattern: "[bad1".into(), + rule_type: "always_deny".into(), + error: "err1".into(), + }, + InvalidRegexPattern { + pattern: "[bad2".into(), + rule_type: "always_allow".into(), + error: "err2".into(), + }, + ], + }, + ); + let p = ToolPermissions { tools }; + + let result = + ToolPermissionDecision::from_input("terminal", "x", &p, false, ShellKind::Posix); + match result { + ToolPermissionDecision::Deny(msg) => { + assert!(msg.contains("2 regex patterns"), "Expected plural: {}", msg); + } + _ => panic!("Expected Deny"), + } + } } diff --git a/crates/util/src/shell.rs b/crates/util/src/shell.rs index babf5e241143cb5a4652ee1692288d98147f0c88..ecac8eaf6aa7d13148d6a5c0770bcedc351c14cf 100644 --- a/crates/util/src/shell.rs +++ b/crates/util/src/shell.rs @@ -256,6 +256,30 @@ impl ShellKind { Self::new(&get_system_shell(), cfg!(windows)) } + /// Returns whether this shell uses POSIX-like command chaining syntax (`&&`, `||`, `;`, `|`). + /// + /// This is used to determine if we can safely parse shell commands to extract sub-commands + /// for security purposes (e.g., preventing shell injection in "always allow" patterns). + /// + /// **Compatible shells:** Posix (sh, bash, dash, zsh), Fish 3.0+, PowerShell 7+/Pwsh, + /// Cmd, Xonsh, Csh, Tcsh + /// + /// **Incompatible shells:** Nushell (uses `and`/`or` keywords), Elvish (uses `and`/`or` + /// keywords), Rc (Plan 9 shell - no `&&`/`||` operators) + pub fn supports_posix_chaining(&self) -> bool { + matches!( + self, + ShellKind::Posix + | ShellKind::Fish + | ShellKind::PowerShell + | ShellKind::Pwsh + | ShellKind::Cmd + | ShellKind::Xonsh + | ShellKind::Csh + | ShellKind::Tcsh + ) + } + pub fn new(program: impl AsRef, is_windows: bool) -> Self { let program = program.as_ref(); let program = program