diff --git a/codex-rs/shell-command/src/command_safety/is_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs index bca44139cd..13b249e8b3 100644 --- a/codex-rs/shell-command/src/command_safety/is_safe_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_safe_command.rs @@ -187,7 +187,12 @@ pub(crate) fn is_safe_git_command(command: &[String]) -> bool { let subcommand_args = &command[subcommand_idx + 1..]; match subcommand { - "status" | "log" | "diff" | "show" => git_subcommand_args_are_read_only(subcommand_args), + "status" | "log" | "diff" | "show" => { + // These subcommands can consult repository-controlled configuration + // for external helpers, so argv-only classification is not enough to + // auto-approve them. + false + } "branch" => { git_subcommand_args_are_read_only(subcommand_args) && git_branch_is_read_only(subcommand_args) @@ -345,7 +350,6 @@ mod tests { #[test] 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", @@ -376,6 +380,25 @@ mod tests { } } + #[test] + fn git_config_sensitive_subcommands_require_approval() { + for args in [ + vec_str(&["git", "status"]), + vec_str(&["git", "log", "-1"]), + vec_str(&["git", "diff"]), + vec_str(&["git", "show", "HEAD"]), + vec_str(&["bash", "-lc", "git status"]), + vec_str(&["bash", "-lc", "git log -1"]), + vec_str(&["bash", "-lc", "git diff"]), + vec_str(&["bash", "-lc", "git show HEAD"]), + ] { + assert!( + !is_known_safe_command(&args), + "expected {args:?} to require approval because git may invoke repository-configured helpers", + ); + } + } + #[test] fn git_branch_mutating_flags_are_not_safe() { assert!(!is_known_safe_command(&vec_str(&[ @@ -461,13 +484,15 @@ mod tests { } #[test] - fn git_subcommand_patch_flags_remain_safe() { - assert!(is_known_safe_command(&vec_str(&["git", "log", "-p", "-1"]))); - assert!(is_known_safe_command(&vec_str(&["git", "diff", "-p"]))); - assert!(is_known_safe_command(&vec_str(&[ + fn git_patch_display_subcommands_require_approval() { + assert!(!is_known_safe_command(&vec_str(&[ + "git", "log", "-p", "-1" + ]))); + assert!(!is_known_safe_command(&vec_str(&["git", "diff", "-p"]))); + assert!(!is_known_safe_command(&vec_str(&[ "git", "show", "-p", "HEAD", ]))); - assert!(is_known_safe_command(&vec_str(&[ + assert!(!is_known_safe_command(&vec_str(&[ "bash", "-lc", "git log -p -1", @@ -651,11 +676,6 @@ mod tests { fn bash_lc_safe_examples() { assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls"]))); assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls -1"]))); - assert!(is_known_safe_command(&vec_str(&[ - "bash", - "-lc", - "git status" - ]))); assert!(is_known_safe_command(&vec_str(&[ "bash", "-lc", diff --git a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs index df2025253b..134bdae609 100644 --- a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs @@ -242,13 +242,6 @@ mod tests { "Get-ChildItem -Path .", ]))); - assert!(is_safe_command_windows(&vec_str(&[ - "powershell.exe", - "-NoProfile", - "-Command", - "git status", - ]))); - assert!(is_safe_command_windows(&vec_str(&[ "powershell.exe", "Get-Content", @@ -290,7 +283,7 @@ mod tests { } #[test] - fn allows_read_only_pipelines_and_git_usage() { + fn allows_read_only_pipelines_and_git_branch_usage() { let Some(pwsh) = try_find_pwsh_executable_blocking() else { return; }; @@ -316,7 +309,7 @@ mod tests { assert!(is_safe_command_windows(&[ pwsh.clone(), "-Command".to_string(), - "git show HEAD:foo.rs".to_string() + "git branch --show-current".to_string() ])); assert!(is_safe_command_windows(&[ @@ -332,6 +325,34 @@ mod tests { ])); } + #[test] + fn rejects_git_config_sensitive_subcommands() { + let results: Vec<(&str, bool)> = ["git status", "git log -1", "git diff", "git show HEAD"] + .into_iter() + .map(|script| { + ( + script, + is_safe_command_windows(&[ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + script.to_string(), + ]), + ) + }) + .collect(); + + assert_eq!( + vec![ + ("git status", false), + ("git log -1", false), + ("git diff", false), + ("git show HEAD", false), + ], + results + ); + } + #[test] fn rejects_git_global_override_options() { let Some(pwsh) = try_find_pwsh_executable_blocking() else {