diff --git a/codex-rs/core/src/command_safety/is_dangerous_command.rs b/codex-rs/core/src/command_safety/is_dangerous_command.rs index 256f36c600..468a856358 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -32,7 +32,11 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { match cmd0 { Some(cmd) if cmd.ends_with("git") || cmd.ends_with("/git") => { - matches!(command.get(1).map(String::as_str), Some("reset" | "rm")) + match command.get(1).map(String::as_str) { + Some("reset" | "rm") => true, + Some("branch") => git_branch_is_delete(command), + _ => false, + } } Some("rm") => matches!(command.get(1).map(String::as_str), Some("-f" | "-rf")), @@ -45,9 +49,19 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { } } +fn git_branch_is_delete(command: &[String]) -> bool { + command.iter().skip(2).any(|arg| { + matches!(arg.as_str(), "-d" | "-D" | "--delete") + || arg.starts_with("--delete=") + || (arg.starts_with("-d") && arg != "-d") + || (arg.starts_with("-D") && arg != "-D") + }) +} + #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; fn vec_str(items: &[&str]) -> Vec { items.iter().map(std::string::ToString::to_string).collect() @@ -55,64 +69,89 @@ mod tests { #[test] fn git_reset_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&["git", "reset"]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["git", "reset"])), + true + ); } #[test] fn bash_git_reset_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&[ - "bash", - "-lc", - "git reset --hard" - ]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["bash", "-lc", "git reset --hard"])), + true + ); } #[test] fn zsh_git_reset_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&[ - "zsh", - "-lc", - "git reset --hard" - ]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["zsh", "-lc", "git reset --hard"])), + true + ); } #[test] fn git_status_is_not_dangerous() { - assert!(!command_might_be_dangerous(&vec_str(&["git", "status"]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["git", "status"])), + false + ); } #[test] fn bash_git_status_is_not_dangerous() { - assert!(!command_might_be_dangerous(&vec_str(&[ - "bash", - "-lc", - "git status" - ]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["bash", "-lc", "git status"])), + false + ); } #[test] fn sudo_git_reset_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&[ - "sudo", "git", "reset", "--hard" - ]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["sudo", "git", "reset", "--hard"])), + true + ); } #[test] fn usr_bin_git_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&[ - "/usr/bin/git", - "reset", - "--hard" - ]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["/usr/bin/git", "reset", "--hard"])), + true + ); + } + + #[test] + fn git_branch_delete_is_dangerous() { + assert_eq!( + command_might_be_dangerous(&vec_str(&["git", "branch", "-d", "feature"])), + true + ); + assert_eq!( + command_might_be_dangerous(&vec_str(&["git", "branch", "-D", "feature"])), + true + ); + assert_eq!( + command_might_be_dangerous(&vec_str(&["bash", "-lc", "git branch --delete feature"])), + true + ); } #[test] fn rm_rf_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"])), + true + ); } #[test] fn rm_f_is_dangerous() { - assert!(command_might_be_dangerous(&vec_str(&["rm", "-f", "/"]))); + assert_eq!( + command_might_be_dangerous(&vec_str(&["rm", "-f", "/"])), + true + ); } } diff --git a/codex-rs/core/src/command_safety/is_safe_command.rs b/codex-rs/core/src/command_safety/is_safe_command.rs index 01a52026e2..38cdf0a5ef 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -131,10 +131,11 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } // Git - Some("git") => matches!( - command.get(1).map(String::as_str), - Some("branch" | "status" | "log" | "diff" | "show") - ), + Some("git") => match command.get(1).map(String::as_str) { + Some("status" | "log" | "diff" | "show") => true, + Some("branch") => git_branch_is_read_only(command), + _ => false, + }, // Rust Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, @@ -155,6 +156,34 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } } +// Treat `git branch` as safe only when the arguments clearly indicate +// a read-only query, not a branch mutation (create/rename/delete). +fn git_branch_is_read_only(command: &[String]) -> bool { + if command.len() <= 2 { + // `git branch` with no additional args lists branches. + return true; + } + + let mut saw_read_only_flag = false; + for arg in command.iter().skip(2).map(String::as_str) { + match arg { + "--list" | "-l" | "--show-current" | "-a" | "--all" | "-r" | "--remotes" | "-v" + | "-vv" | "--verbose" => { + saw_read_only_flag = true; + } + _ if arg.starts_with("--format=") => { + saw_read_only_flag = true; + } + _ => { + // Any other flag or positional argument may create, rename, or delete branches. + return false; + } + } + } + + saw_read_only_flag +} + // (bash parsing helpers implemented in crate::bash) /* ---------------------------------------------------------- @@ -207,6 +236,12 @@ mod tests { fn known_safe_examples() { assert!(is_safe_to_call_with_exec(&vec_str(&["ls"]))); assert!(is_safe_to_call_with_exec(&vec_str(&["git", "status"]))); + assert!(is_safe_to_call_with_exec(&vec_str(&["git", "branch"]))); + assert!(is_safe_to_call_with_exec(&vec_str(&[ + "git", + "branch", + "--show-current" + ]))); assert!(is_safe_to_call_with_exec(&vec_str(&["base64"]))); assert!(is_safe_to_call_with_exec(&vec_str(&[ "sed", "-n", "1,5p", "file.txt" @@ -231,6 +266,18 @@ mod tests { } } + #[test] + fn git_branch_mutating_flags_are_not_safe() { + assert!(!is_known_safe_command(&vec_str(&[ + "git", "branch", "-d", "feature" + ]))); + assert!(!is_known_safe_command(&vec_str(&[ + "git", + "branch", + "new-branch" + ]))); + } + #[test] fn zsh_lc_safe_command_sequence() { assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"])));