Detailed changes
@@ -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"
@@ -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"] }
@@ -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
@@ -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;
@@ -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<String> {
- 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<String> {
+ 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<String> {
.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<String> {
+ let command_name = extract_command_name(command)?;
+ Some(format!("^{}\\b", regex::escape(&command_name)))
+}
+
pub fn extract_terminal_pattern_display(command: &str) -> Option<String> {
- 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<String> {
@@ -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);
@@ -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<Vec<String>> {
+ 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<String>) {
+ 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<String>,
+) {
+ 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<String>) {
+ 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<String>) {
+ for command in &pipeline.seq {
+ extract_commands_from_command(command, commands);
+ }
+}
+
+fn extract_commands_from_command(command: &ast::Command, commands: &mut Vec<String>) {
+ 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<String>,
+) {
+ 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<String>) {
+ 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<String>) {
+ 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<String>,
+) {
+ 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<String>) {
+ 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<String>) {
+ 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<String>) {
+ 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<String>) {
+ 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<String>,
+) {
+ 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<String>) {
+ extract_commands_from_compound_list(&do_group.list, commands);
+}
+
+fn extract_commands_from_function_body(func_body: &ast::FunctionBody, commands: &mut Vec<String>) {
+ extract_commands_from_compound_command(&func_body.0, commands);
+}
+
+fn extract_commands_from_extended_test_expr(
+ test_expr: &ast::ExtendedTestExprCommand,
+ commands: &mut Vec<String>,
+) {
+ 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<String>,
+) {
+ 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());
+ }
+}
@@ -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(
@@ -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<Item = String>,
+ 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<String>
///
/// 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"),
+ }
+ }
}
@@ -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<Path>, is_windows: bool) -> Self {
let program = program.as_ref();
let program = program