From 0868fef919e8152f5a06453c2d6fa508ef598bac Mon Sep 17 00:00:00 2001 From: Olivier Mansour Date: Tue, 3 Jun 2025 17:06:42 +0200 Subject: [PATCH 1/2] feat(execute_bash): untrust commands based on env parameter --- crates/cli/src/cli/chat/mod.rs | 9 +- crates/cli/src/cli/chat/tools/execute_bash.rs | 121 ++++++++++++++---- 2 files changed, 107 insertions(+), 23 deletions(-) diff --git a/crates/cli/src/cli/chat/mod.rs b/crates/cli/src/cli/chat/mod.rs index b4bcb8af7..71359cfd5 100644 --- a/crates/cli/src/cli/chat/mod.rs +++ b/crates/cli/src/cli/chat/mod.rs @@ -3016,9 +3016,16 @@ impl ChatContext { } // If there is an override, we will use it. Otherwise fall back to Tool's default. - let allowed = self.tool_permissions.trust_all + let mut allowed = self.tool_permissions.trust_all || (self.tool_permissions.has(&tool.name) && self.tool_permissions.is_trusted(&tool.name)) || !tool.tool.requires_acceptance(&self.ctx); + + // Check if the tool has a should_untrust method and if it returns true + if let Tool::ExecuteBash(execute_bash) = &tool.tool { + if execute_bash.should_untrust() { + allowed = false; + } + } if database .settings diff --git a/crates/cli/src/cli/chat/tools/execute_bash.rs b/crates/cli/src/cli/chat/tools/execute_bash.rs index 68caa287d..e3bcb16c8 100644 --- a/crates/cli/src/cli/chat/tools/execute_bash.rs +++ b/crates/cli/src/cli/chat/tools/execute_bash.rs @@ -1,35 +1,19 @@ use std::collections::VecDeque; use std::io::Write; -use std::process::{ - ExitStatus, - Stdio, -}; +use std::process::{ExitStatus, Stdio}; use std::str::from_utf8; use crossterm::queue; -use crossterm::style::{ - self, - Color, -}; -use eyre::{ - Context as EyreContext, - Result, -}; +use crossterm::style::{self, Color}; +use eyre::{Context as EyreContext, Result}; use serde::Deserialize; use tokio::io::AsyncBufReadExt; use tokio::select; use tracing::error; use super::super::util::truncate_safe; -use super::{ - InvokeOutput, - MAX_TOOL_RESPONSE_SIZE, - OutputKind, -}; -use crate::cli::chat::{ - CONTINUATION_LINE, - PURPOSE_ARROW, -}; +use super::{InvokeOutput, MAX_TOOL_RESPONSE_SIZE, OutputKind}; +use crate::cli::chat::{CONTINUATION_LINE, PURPOSE_ARROW}; use crate::platform::Context; const READONLY_COMMANDS: &[&str] = &["ls", "cat", "echo", "pwd", "which", "head", "tail", "find", "grep"]; @@ -40,7 +24,16 @@ pub struct ExecuteBash { } impl ExecuteBash { - pub fn requires_acceptance(&self) -> bool { + pub fn should_untrust(&self) -> bool { + // check for custom untrusted patterns from environment variable + // ie: export Q_EXECUTE_BASH_UNTRUSTED_PATTERNS = "git push,delete,rm" + let env = crate::platform::Env::new(); + if let Ok(untrusted_patterns) = env.get("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS") { + let patterns: Vec<&str> = untrusted_patterns.split(',').map(|s| s.trim()).collect(); + if patterns.iter().any(|pattern| self.command.contains(pattern)) { + return true; + } + } let Some(args) = shlex::split(&self.command) else { return true; }; @@ -96,6 +89,10 @@ impl ExecuteBash { false } + + pub fn requires_acceptance(&self) -> bool { + self.should_untrust() + } pub async fn invoke(&self, updates: impl Write) -> Result { let output = run_command(&self.command, MAX_TOOL_RESPONSE_SIZE / 3, Some(updates)).await?; @@ -393,4 +390,84 @@ mod tests { ); } } + + #[test] + fn test_requires_acceptance_for_custom_untrusted_patterns() { + use std::env; + + // Save original environment variable if it exists + let original_value = env::var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS").ok(); + + // Test with single pattern + env::set_var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS", "custom_pattern"); + + let safe_cmd = serde_json::from_value::(serde_json::json!({ + "command": "echo hello world", + })) + .unwrap(); + assert_eq!( + safe_cmd.requires_acceptance(), + false, + "Safe command should not require acceptance" + ); + + let unsafe_cmd = serde_json::from_value::(serde_json::json!({ + "command": "echo custom_pattern", + })) + .unwrap(); + assert_eq!( + unsafe_cmd.requires_acceptance(), + true, + "Command with custom untrusted pattern should require acceptance" + ); + + // Test with multiple patterns + env::set_var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS", "pattern1,pattern2, pattern3"); + + let safe_cmd = serde_json::from_value::(serde_json::json!({ + "command": "echo safe command", + })) + .unwrap(); + assert_eq!( + safe_cmd.requires_acceptance(), + false, + "Safe command should not require acceptance" + ); + + let unsafe_cmd1 = serde_json::from_value::(serde_json::json!({ + "command": "echo pattern1 is unsafe", + })) + .unwrap(); + assert_eq!( + unsafe_cmd1.requires_acceptance(), + true, + "Command with first pattern should require acceptance" + ); + + let unsafe_cmd2 = serde_json::from_value::(serde_json::json!({ + "command": "echo pattern2 is also unsafe", + })) + .unwrap(); + assert_eq!( + unsafe_cmd2.requires_acceptance(), + true, + "Command with second pattern should require acceptance" + ); + + let unsafe_cmd3 = serde_json::from_value::(serde_json::json!({ + "command": "echo pattern3 is unsafe too", + })) + .unwrap(); + assert_eq!( + unsafe_cmd3.requires_acceptance(), + true, + "Command with third pattern should require acceptance" + ); + + // Restore original environment variable or remove it + match original_value { + Some(value) => env::set_var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS", value), + None => env::remove_var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS"), + } + } } From 6ec3cd49d608bae273fac94abf4c46edc11c4bd9 Mon Sep 17 00:00:00 2001 From: Olivier Mansour Date: Wed, 4 Jun 2025 10:05:41 +0200 Subject: [PATCH 2/2] feat(execute_bash): let the user use a regex to catch untruste generated bash commands --- crates/cli/src/cli/chat/tools/execute_bash.rs | 62 ++++++++++++++++++- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/crates/cli/src/cli/chat/tools/execute_bash.rs b/crates/cli/src/cli/chat/tools/execute_bash.rs index e3bcb16c8..f9fdd5ec1 100644 --- a/crates/cli/src/cli/chat/tools/execute_bash.rs +++ b/crates/cli/src/cli/chat/tools/execute_bash.rs @@ -6,6 +6,7 @@ use std::str::from_utf8; use crossterm::queue; use crossterm::style::{self, Color}; use eyre::{Context as EyreContext, Result}; +use regex::Regex; use serde::Deserialize; use tokio::io::AsyncBufReadExt; use tokio::select; @@ -26,12 +27,24 @@ pub struct ExecuteBash { impl ExecuteBash { pub fn should_untrust(&self) -> bool { // check for custom untrusted patterns from environment variable - // ie: export Q_EXECUTE_BASH_UNTRUSTED_PATTERNS = "git push,delete,rm" + // ie: export Q_EXECUTE_BASH_UNTRUSTED_PATTERNS = "git push,delete,/^\s*rm.*/" let env = crate::platform::Env::new(); if let Ok(untrusted_patterns) = env.get("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS") { let patterns: Vec<&str> = untrusted_patterns.split(',').map(|s| s.trim()).collect(); - if patterns.iter().any(|pattern| self.command.contains(pattern)) { - return true; + for pattern in patterns { + // Check if pattern is wrapped in slashes (regex convention) + if pattern.starts_with('/') && pattern.ends_with('/') && pattern.len() > 2 { + // Extract the regex pattern between slashes + let regex_pattern = &pattern[1..pattern.len()-1]; + if let Ok(regex) = Regex::new(regex_pattern) { + if regex.is_match(&self.command) { + return true; + } + } + } else if self.command.contains(pattern) { + // Fall back to simple string matching for non-regex patterns + return true; + } } } let Some(args) = shlex::split(&self.command) else { @@ -420,6 +433,49 @@ mod tests { true, "Command with custom untrusted pattern should require acceptance" ); + + // Test with regex patterns using slash convention + env::set_var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS", "/^\\s*rm.*/,/.*sudo.*/"); + + let safe_cmd1 = serde_json::from_value::(serde_json::json!({ + "command": "echo rm is safe here", + })) + .unwrap(); + assert_eq!( + safe_cmd1.requires_acceptance(), + false, + "Command with rm in the middle should be safe" + ); + + let unsafe_cmd1 = serde_json::from_value::(serde_json::json!({ + "command": "rm -rf /tmp/test", + })) + .unwrap(); + assert_eq!( + unsafe_cmd1.requires_acceptance(), + true, + "Command starting with rm should require acceptance" + ); + + let unsafe_cmd2 = serde_json::from_value::(serde_json::json!({ + "command": " rm -rf /tmp/test", + })) + .unwrap(); + assert_eq!( + unsafe_cmd2.requires_acceptance(), + true, + "Command with leading whitespace before rm should require acceptance" + ); + + let unsafe_cmd3 = serde_json::from_value::(serde_json::json!({ + "command": "echo hello | sudo rm -rf", + })) + .unwrap(); + assert_eq!( + unsafe_cmd3.requires_acceptance(), + true, + "Command containing sudo should require acceptance" + ); // Test with multiple patterns env::set_var("Q_EXECUTE_BASH_UNTRUSTED_PATTERNS", "pattern1,pattern2, pattern3");