From f6b53f29d102e04fd282f7d1b6fc2ec2773212d2 Mon Sep 17 00:00:00 2001 From: Brandon Kiser Date: Wed, 13 Aug 2025 15:29:56 -0700 Subject: [PATCH 1/3] fix: add more safety checks for execute_bash readonly checks --- .../src/cli/chat/tools/execute/mod.rs | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs index 2f97471761..a037f1f2af 100644 --- a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs @@ -48,6 +48,11 @@ pub struct ExecuteCommand { impl ExecuteCommand { pub fn requires_acceptance(&self, allowed_commands: Option<&Vec>, allow_read_only: bool) -> bool { + // Always require acceptance for multi-line commands. + if self.command.contains("\n") { + return true; + } + let default_arr = vec![]; let allowed_commands = allowed_commands.unwrap_or(&default_arr); @@ -64,7 +69,7 @@ impl ExecuteCommand { let Some(args) = shlex::split(&self.command) else { return true; }; - const DANGEROUS_PATTERNS: &[&str] = &["<(", "$(", "`", ">", "&&", "||", "&", ";"]; + const DANGEROUS_PATTERNS: &[&str] = &["<(", "$(", "`", ">", "&&", "||", "&", ";", "${", "\n", "IFS"]; if args .iter() @@ -106,6 +111,7 @@ impl ExecuteCommand { arg.contains("-exec") // includes -execdir || arg.contains("-delete") || arg.contains("-ok") // includes -okdir + || arg.contains("-fprint") // includes -fprint0 and -fprintf }) => { return true; @@ -114,7 +120,11 @@ impl ExecuteCommand { // Special casing for `grep`. -P flag for perl regexp has RCE issues, apparently // should not be supported within grep but is flagged as a possibility since this is perl // regexp. - if cmd == "grep" && cmd_args.iter().any(|arg| arg.contains("-P")) { + if cmd == "grep" + && cmd_args + .iter() + .any(|arg| arg.contains("-P") || arg.contains("--perl-regexp")) + { return true; } let is_cmd_read_only = READONLY_COMMANDS.contains(&cmd.as_str()); @@ -290,6 +300,13 @@ mod tests { ("cat <<< 'some string here' > myimportantfile", true), ("echo '\n#!/usr/bin/env bash\necho hello\n' > myscript.sh", true), ("cat < myimportantfile\nhello world\nEOF", true), + // newline check + ("which ls\ntouch asdf", true), + // $IFS check + ( + r#"IFS=';'; for cmd in "which ls;touch asdf"; do eval "$cmd"; done"#, + true, + ), // Safe piped commands ("find . -name '*.rs' | grep main", false), ("ls -la | grep .git", false), @@ -307,8 +324,12 @@ mod tests { true, ), ("find important-dir/ -name '*.txt'", false), + (r#"find / -fprintf "/path/to/file" -quit"#, true), + (r"find . -${t}exec touch asdf \{\} +", true), + (r"find . -${t:=exec} touch asdf2 \{\} +", true), // `grep` command arguments ("echo 'test data' | grep -P '(?{system(\"date\")})'", true), + ("echo 'test data' | grep --perl-regexp '(?{system(\"date\")})'", true), ]; for (cmd, expected) in cmds { let tool = serde_json::from_value::(serde_json::json!({ From e59cfd8577ff8cc46112e8f6947b81ff80ec8596 Mon Sep 17 00:00:00 2001 From: Brandon Kiser Date: Thu, 14 Aug 2025 12:06:00 -0700 Subject: [PATCH 2/3] add extra check --- crates/chat-cli/src/cli/chat/tools/execute/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs index a037f1f2af..ae1bb0afe9 100644 --- a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs @@ -69,7 +69,7 @@ impl ExecuteCommand { let Some(args) = shlex::split(&self.command) else { return true; }; - const DANGEROUS_PATTERNS: &[&str] = &["<(", "$(", "`", ">", "&&", "||", "&", ";", "${", "\n", "IFS"]; + const DANGEROUS_PATTERNS: &[&str] = &["<(", "$(", "`", ">", "&&", "||", "&", ";", "${", "\n", "\r", "IFS"]; if args .iter() @@ -300,8 +300,9 @@ mod tests { ("cat <<< 'some string here' > myimportantfile", true), ("echo '\n#!/usr/bin/env bash\necho hello\n' > myscript.sh", true), ("cat < myimportantfile\nhello world\nEOF", true), - // newline check + // newline checks ("which ls\ntouch asdf", true), + ("which ls\rtouch asdf", true), // $IFS check ( r#"IFS=';'; for cmd in "which ls;touch asdf"; do eval "$cmd"; done"#, From 3f909eadb8851bbb97f73f15fe610f3c3d66bf6e Mon Sep 17 00:00:00 2001 From: Brandon Kiser Date: Thu, 14 Aug 2025 12:09:26 -0700 Subject: [PATCH 3/3] fix newline check --- crates/chat-cli/src/cli/chat/tools/execute/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs index ae1bb0afe9..f035f9e601 100644 --- a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs @@ -49,7 +49,7 @@ pub struct ExecuteCommand { impl ExecuteCommand { pub fn requires_acceptance(&self, allowed_commands: Option<&Vec>, allow_read_only: bool) -> bool { // Always require acceptance for multi-line commands. - if self.command.contains("\n") { + if self.command.contains("\n") || self.command.contains("\r") { return true; }