Skip to content

fix: add more safety checks for execute_bash readonly checks #2605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brandonskiser merged 3 commits into from
Aug 14, 2025
Merged

fix: add more safety checks for execute_bash readonly checks #2605

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6b53f2
fix: add more safety checks for execute_bash readonly checks
brandonskiser Aug 13, 2025
e59cfd8
add extra check
brandonskiser Aug 14, 2025
3f909ea
fix newline check
brandonskiser Aug 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions crates/chat-cli/src/cli/chat/tools/execute/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ pub struct ExecuteCommand {

impl ExecuteCommand {
pub fn requires_acceptance(&self, allowed_commands: Option<&Vec<String>>, allow_read_only: bool) -> bool {
// Always require acceptance for multi-line commands.
if self.command.contains("\n") || self.command.contains("\r") {
return true;
}

let default_arr = vec![];
let allowed_commands = allowed_commands.unwrap_or(&default_arr);

Expand All @@ -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", "\r", "IFS"];

if args
.iter()
Expand Down Expand Up @@ -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;
Expand All @@ -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());
Expand Down Expand Up @@ -290,6 +300,14 @@ mod tests {
("cat <<< 'some string here' > myimportantfile", true),
("echo '\n#!/usr/bin/env bash\necho hello\n' > myscript.sh", true),
("cat <<EOF > myimportantfile\nhello world\nEOF", true),
// 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"#,
true,
),
// Safe piped commands
("find . -name '*.rs' | grep main", false),
("ls -la | grep .git", false),
Expand All @@ -307,8 +325,12 @@ mod tests {
true,
),
("find important-dir/ -name '*.txt'", false),
(r#"find / -fprintf "/path/to/file" <data-to-write> -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::<ExecuteCommand>(serde_json::json!({
Expand Down
Loading