From f50c8b2f81d69e49d1f6eafaeebf0f6ba2bbba52 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Mon, 2 Feb 2026 12:30:17 -0800 Subject: [PATCH] fix: unsafe auto-approval of git commands (#10258) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://github.com/openai/codex/issues/10160 and some more. ## Description Hardens Git command safety to prevent approval bypasses for destructive or write-capable invocations (branch delete, risky push forms, output/config-override flags), so these commands no longer auto-run as “safe.” - `git branch -d` variants (especially in worktrees / with global options like -C / -c) - `git show|diff|log --output` ... style file-write flags - risky Git config override flags (-c, --config-env) that can trigger external execution - dangerous push forms that weren’t fully caught (`--force*`, `--delete`, `+refspec`, `:refspec`) - grouped short-flag delete forms (e.g. stacked branch flags containing `d/D`) will fast follow with a common git policy to bring windows to parity. --------- Co-authored-by: Eric Traut --- .../command_safety/is_dangerous_command.rs | 278 +++++++++++++++++- .../src/command_safety/is_safe_command.rs | 179 ++++++++++- codex-rs/core/src/exec_policy.rs | 24 ++ 3 files changed, 468 insertions(+), 13 deletions(-) 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 256f36c60..3e2c669c4 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -27,12 +27,100 @@ pub fn command_might_be_dangerous(command: &[String]) -> bool { false } +fn is_git_global_option_with_value(arg: &str) -> bool { + matches!( + arg, + "-C" | "-c" + | "--config-env" + | "--exec-path" + | "--git-dir" + | "--namespace" + | "--super-prefix" + | "--work-tree" + ) +} + +fn is_git_global_option_with_inline_value(arg: &str) -> bool { + matches!( + arg, + s if s.starts_with("--config-env=") + || s.starts_with("--exec-path=") + || s.starts_with("--git-dir=") + || s.starts_with("--namespace=") + || s.starts_with("--super-prefix=") + || s.starts_with("--work-tree=") + ) || ((arg.starts_with("-C") || arg.starts_with("-c")) && arg.len() > 2) +} + +/// Find the first matching git subcommand, skipping known global options that +/// may appear before it (e.g., `-C`, `-c`, `--git-dir`). +/// +/// Shared with `is_safe_command` to avoid git-global-option bypasses. +pub(crate) fn find_git_subcommand<'a>( + command: &'a [String], + subcommands: &[&str], +) -> Option<(usize, &'a str)> { + let cmd0 = command.first().map(String::as_str)?; + if !cmd0.ends_with("git") { + return None; + } + + let mut skip_next = false; + for (idx, arg) in command.iter().enumerate().skip(1) { + if skip_next { + skip_next = false; + continue; + } + + let arg = arg.as_str(); + + if is_git_global_option_with_inline_value(arg) { + continue; + } + + if is_git_global_option_with_value(arg) { + skip_next = true; + continue; + } + + if arg == "--" || arg.starts_with('-') { + continue; + } + + if subcommands.contains(&arg) { + return Some((idx, arg)); + } + + // In git, the first non-option token is the subcommand. If it isn't + // one of the subcommands we're looking for, we must stop scanning to + // avoid misclassifying later positional args (e.g., branch names). + return None; + } + + None +} + fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { let cmd0 = command.first().map(String::as_str); match cmd0 { - Some(cmd) if cmd.ends_with("git") || cmd.ends_with("/git") => { - matches!(command.get(1).map(String::as_str), Some("reset" | "rm")) + Some(cmd) if cmd.ends_with("git") => { + let Some((subcommand_idx, subcommand)) = + find_git_subcommand(command, &["reset", "rm", "branch", "push", "clean"]) + else { + return false; + }; + + match subcommand { + "reset" | "rm" => true, + "branch" => git_branch_is_delete(&command[subcommand_idx + 1..]), + "push" => git_push_is_dangerous(&command[subcommand_idx + 1..]), + "clean" => git_clean_is_force(&command[subcommand_idx + 1..]), + other => { + debug_assert!(false, "unexpected git subcommand from matcher: {other}"); + false + } + } } Some("rm") => matches!(command.get(1).map(String::as_str), Some("-f" | "-rf")), @@ -45,6 +133,48 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { } } +fn git_branch_is_delete(branch_args: &[String]) -> bool { + // Git allows stacking short flags (for example, `-dv` or `-vd`). Treat any + // short-flag group containing `d`/`D` as a delete flag. + branch_args.iter().map(String::as_str).any(|arg| { + matches!(arg, "-d" | "-D" | "--delete") + || arg.starts_with("--delete=") + || short_flag_group_contains(arg, 'd') + || short_flag_group_contains(arg, 'D') + }) +} + +fn short_flag_group_contains(arg: &str, target: char) -> bool { + arg.starts_with('-') && !arg.starts_with("--") && arg.chars().skip(1).any(|c| c == target) +} + +fn git_push_is_dangerous(push_args: &[String]) -> bool { + push_args.iter().map(String::as_str).any(|arg| { + matches!( + arg, + "--force" | "--force-with-lease" | "--force-if-includes" | "--delete" | "-f" | "-d" + ) || arg.starts_with("--force-with-lease=") + || arg.starts_with("--force-if-includes=") + || arg.starts_with("--delete=") + || short_flag_group_contains(arg, 'f') + || short_flag_group_contains(arg, 'd') + || git_push_refspec_is_dangerous(arg) + }) +} + +fn git_push_refspec_is_dangerous(arg: &str) -> bool { + // `+` forces updates and `:` deletes remote refs. + (arg.starts_with('+') || arg.starts_with(':')) && arg.len() > 1 +} + +fn git_clean_is_force(clean_args: &[String]) -> bool { + clean_args.iter().map(String::as_str).any(|arg| { + matches!(arg, "--force" | "-f") + || arg.starts_with("--force=") + || short_flag_group_contains(arg, 'f') + }) +} + #[cfg(test)] mod tests { use super::*; @@ -63,7 +193,7 @@ mod tests { assert!(command_might_be_dangerous(&vec_str(&[ "bash", "-lc", - "git reset --hard" + "git reset --hard", ]))); } @@ -72,7 +202,7 @@ mod tests { assert!(command_might_be_dangerous(&vec_str(&[ "zsh", "-lc", - "git reset --hard" + "git reset --hard", ]))); } @@ -86,14 +216,14 @@ mod tests { assert!(!command_might_be_dangerous(&vec_str(&[ "bash", "-lc", - "git status" + "git status", ]))); } #[test] fn sudo_git_reset_is_dangerous() { assert!(command_might_be_dangerous(&vec_str(&[ - "sudo", "git", "reset", "--hard" + "sudo", "git", "reset", "--hard", ]))); } @@ -102,7 +232,141 @@ mod tests { assert!(command_might_be_dangerous(&vec_str(&[ "/usr/bin/git", "reset", - "--hard" + "--hard", + ]))); + } + + #[test] + fn git_branch_delete_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "branch", "-d", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "branch", "-D", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "bash", + "-lc", + "git branch --delete feature", + ]))); + } + + #[test] + fn git_branch_delete_with_stacked_short_flags_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "branch", "-dv", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "branch", "-vd", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "branch", "-vD", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "branch", "-Dvv", "feature", + ]))); + } + + #[test] + fn git_branch_delete_with_global_options_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "-C", ".", "branch", "-d", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", + "-c", + "color.ui=false", + "branch", + "-D", + "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "bash", + "-lc", + "git -C . branch -d feature", + ]))); + } + + #[test] + fn git_checkout_reset_is_not_dangerous() { + // The first non-option token is "checkout", so later positional args + // like branch names must not be treated as subcommands. + assert!(!command_might_be_dangerous(&vec_str(&[ + "git", "checkout", "reset", + ]))); + } + + #[test] + fn git_push_force_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "push", "--force", "origin", "main", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "push", "-f", "origin", "main", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", + "-C", + ".", + "push", + "--force-with-lease", + "origin", + "main", + ]))); + } + + #[test] + fn git_push_plus_refspec_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "push", "origin", "+main", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", + "push", + "origin", + "+refs/heads/main:refs/heads/main", + ]))); + } + + #[test] + fn git_push_delete_flag_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "push", "--delete", "origin", "feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "push", "-d", "origin", "feature", + ]))); + } + + #[test] + fn git_push_delete_refspec_is_dangerous() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "push", "origin", ":feature", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "bash", + "-lc", + "git push origin :feature", + ]))); + } + + #[test] + fn git_push_without_force_is_not_dangerous() { + assert!(!command_might_be_dangerous(&vec_str(&[ + "git", "push", "origin", "main", + ]))); + } + + #[test] + fn git_clean_force_is_dangerous_even_when_f_is_not_first_flag() { + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "clean", "-fdx", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "clean", "-xdf", + ]))); + assert!(command_might_be_dangerous(&vec_str(&[ + "git", "clean", "--force", ]))); } 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 01a52026e..e52079c74 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -1,4 +1,8 @@ use crate::bash::parse_shell_lc_plain_commands; +// Find the first matching git subcommand, skipping known global options that +// may appear before it (e.g., `-C`, `-c`, `--git-dir`). +// Implemented in `is_dangerous_command` and shared here. +use crate::command_safety::is_dangerous_command::find_git_subcommand; use crate::command_safety::windows_safe_commands::is_safe_command_windows; pub fn is_known_safe_command(command: &[String]) -> bool { @@ -131,13 +135,36 @@ 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") => { + // Global config overrides like `-c core.pager=...` can force git + // to execute arbitrary external commands. With no sandboxing, we + // should always prompt in those cases. + if git_has_config_override_global_option(command) { + return false; + } - // Rust - Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, + let Some((subcommand_idx, subcommand)) = + find_git_subcommand(command, &["status", "log", "diff", "show", "branch"]) + else { + return false; + }; + + let subcommand_args = &command[subcommand_idx + 1..]; + + match subcommand { + "status" | "log" | "diff" | "show" => { + git_subcommand_args_are_read_only(subcommand_args) + } + "branch" => { + git_subcommand_args_are_read_only(subcommand_args) + && git_branch_is_read_only(subcommand_args) + } + other => { + debug_assert!(false, "unexpected git subcommand from matcher: {other}"); + false + } + } + } // Special-case `sed -n {N|M,N}p` Some("sed") @@ -155,6 +182,60 @@ 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(branch_args: &[String]) -> bool { + if branch_args.is_empty() { + // `git branch` with no additional args lists branches. + return true; + } + + let mut saw_read_only_flag = false; + for arg in branch_args.iter().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 +} + +fn git_has_config_override_global_option(command: &[String]) -> bool { + command.iter().map(String::as_str).any(|arg| { + matches!(arg, "-c" | "--config-env") + || (arg.starts_with("-c") && arg.len() > 2) + || arg.starts_with("--config-env=") + }) +} + +fn git_subcommand_args_are_read_only(args: &[String]) -> bool { + // Flags that can write to disk or execute external tools should never be + // auto-approved on an unsandboxed machine. + const UNSAFE_GIT_FLAGS: &[&str] = &[ + "--output", + "--ext-diff", + "--textconv", + "--exec", + "--paginate", + ]; + + !args.iter().map(String::as_str).any(|arg| { + UNSAFE_GIT_FLAGS.contains(&arg) + || arg.starts_with("--output=") + || arg.starts_with("--exec=") + }) +} + // (bash parsing helpers implemented in crate::bash) /* ---------------------------------------------------------- @@ -207,6 +288,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 +318,86 @@ 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 git_branch_global_options_respect_safety_rules() { + use pretty_assertions::assert_eq; + + assert_eq!( + is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "--show-current"])), + true + ); + assert_eq!( + is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "-d", "feature"])), + false + ); + assert_eq!( + is_known_safe_command(&vec_str(&["bash", "-lc", "git -C . branch -d feature",])), + false + ); + } + + #[test] + fn git_first_positional_is_the_subcommand() { + // In git, the first non-option token is the subcommand. Later positional + // args (like branch names) must not be treated as subcommands. + assert!(!is_known_safe_command(&vec_str(&[ + "git", "checkout", "status", + ]))); + } + + #[test] + fn git_output_and_config_override_flags_are_not_safe() { + assert!(!is_known_safe_command(&vec_str(&[ + "git", + "log", + "--output=/tmp/git-log-out-test", + "-n", + "1", + ]))); + assert!(!is_known_safe_command(&vec_str(&[ + "git", + "diff", + "--output", + "/tmp/git-diff-out-test", + ]))); + assert!(!is_known_safe_command(&vec_str(&[ + "git", + "show", + "--output=/tmp/git-show-out-test", + "HEAD", + ]))); + assert!(!is_known_safe_command(&vec_str(&[ + "git", + "-c", + "core.pager=cat", + "log", + "-n", + "1", + ]))); + assert!(!is_known_safe_command(&vec_str(&[ + "git", + "-ccore.pager=cat", + "status", + ]))); + } + + #[test] + fn cargo_check_is_not_safe() { + assert!(!is_known_safe_command(&vec_str(&["cargo", "check"]))); + } + #[test] fn zsh_lc_safe_command_sequence() { assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"]))); diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index c0cb7f8fc..a2e1e8d40 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -1280,6 +1280,30 @@ prefix_rule( ); } + #[tokio::test] + async fn dangerous_git_push_requires_approval_in_danger_full_access() { + let command = vec_str(&["git", "push", "origin", "+main"]); + let manager = ExecPolicyManager::default(); + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + features: &Features::with_defaults(), + command: &command, + approval_policy: AskForApproval::OnRequest, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)), + } + ); + } + fn vec_str(items: &[&str]) -> Vec { items.iter().map(std::string::ToString::to_string).collect() }