From 901fa6d18a6bf74745554d2c63a2b02c8848d0ff Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 30 Jan 2026 11:02:12 -0800 Subject: [PATCH 01/10] Fix unsafe auto-approval of git branch deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnosis: Although the bug report specifically mentioned worktrees, this problm isn't worktree-specific. `git branch -d/-D/--delete` isn’t treated as dangerous, and git branch is currently treated as “safe,” so it won’t prompt under on-request. In a worktree, that’s extra risky because branch deletion writes to the common gitdir outside the workdir. * Classify git branch -d/-D/--delete as dangerous to force approval under on-request. * Restrict git branch “safe” classification to read-only forms (e.g., --show-current, --list). * Add focused safety tests for branch deletion and read-only branch queries. This addresses #10160 --- .../command_safety/is_dangerous_command.rs | 95 +++++++++++++------ .../src/command_safety/is_safe_command.rs | 55 ++++++++++- 2 files changed, 118 insertions(+), 32 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 256f36c60057..468a85635875 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 01a52026e2e0..38cdf0a5ef1f 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"]))); From 1e9258c8b3a6f364243b5ac40fd0f8b187ee7bea Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 12:28:44 -0800 Subject: [PATCH 02/10] fix(core): detect git branch deletion with global flags --- .../command_safety/is_dangerous_command.rs | 110 ++++++++++++++++- .../src/command_safety/is_safe_command.rs | 111 ++++++++++++++++-- 2 files changed, 207 insertions(+), 14 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 468a85635875..e8e52d9caf54 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -27,15 +27,90 @@ 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`). +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") || 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)); + } + } + + 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") => { - match command.get(1).map(String::as_str) { - Some("reset" | "rm") => true, - Some("branch") => git_branch_is_delete(command), - _ => false, + let Some((subcommand_idx, subcommand)) = + find_git_subcommand(command, &["reset", "rm", "branch"]) + else { + return false; + }; + + match subcommand { + "reset" | "rm" => true, + "branch" => git_branch_is_delete(&command[subcommand_idx + 1..]), + other => { + debug_assert!(false, "unexpected git subcommand from matcher: {other}"); + false + } } } @@ -49,8 +124,8 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { } } -fn git_branch_is_delete(command: &[String]) -> bool { - command.iter().skip(2).any(|arg| { +fn git_branch_is_delete(branch_args: &[String]) -> bool { + branch_args.iter().any(|arg| { matches!(arg.as_str(), "-d" | "-D" | "--delete") || arg.starts_with("--delete=") || (arg.starts_with("-d") && arg != "-d") @@ -139,6 +214,29 @@ mod tests { ); } + #[test] + fn git_branch_delete_with_global_options_is_dangerous() { + assert_eq!( + command_might_be_dangerous(&vec_str(&["git", "-C", ".", "branch", "-d", "feature"])), + true + ); + assert_eq!( + command_might_be_dangerous(&vec_str(&[ + "git", + "-c", + "color.ui=false", + "branch", + "-D", + "feature", + ])), + true + ); + assert_eq!( + command_might_be_dangerous(&vec_str(&["bash", "-lc", "git -C . branch -d feature",])), + true + ); + } + #[test] fn rm_rf_is_dangerous() { assert_eq!( 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 38cdf0a5ef1f..20c10a9a8a75 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -38,6 +38,72 @@ pub fn is_known_safe_command(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`). +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") || 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)); + } + } + + None +} + fn is_safe_to_call_with_exec(command: &[String]) -> bool { let Some(cmd0) = command.first().map(String::as_str) else { return false; @@ -131,11 +197,22 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } // Git - 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, - }, + Some("git") => { + let Some((subcommand_idx, subcommand)) = + find_git_subcommand(command, &["status", "log", "diff", "show", "branch"]) + else { + return false; + }; + + match subcommand { + "status" | "log" | "diff" | "show" => true, + "branch" => git_branch_is_read_only(&command[subcommand_idx + 1..]), + other => { + debug_assert!(false, "unexpected git subcommand from matcher: {other}"); + false + } + } + } // Rust Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, @@ -158,14 +235,14 @@ 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 { +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 command.iter().skip(2).map(String::as_str) { + for arg in branch_args.iter().map(String::as_str) { match arg { "--list" | "-l" | "--show-current" | "-a" | "--all" | "-r" | "--remotes" | "-v" | "-vv" | "--verbose" => { @@ -278,6 +355,24 @@ mod tests { ]))); } + #[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 zsh_lc_safe_command_sequence() { assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"]))); From 641a8db83d10e1030938f7fd1c0db295d59914fd Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 13:08:01 -0800 Subject: [PATCH 03/10] refactor(core): share git subcommand helper across safety checks --- .../command_safety/is_dangerous_command.rs | 4 +- .../src/command_safety/is_safe_command.rs | 70 ++----------------- 2 files changed, 7 insertions(+), 67 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 e8e52d9caf54..9528f4a886e7 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -54,7 +54,9 @@ fn is_git_global_option_with_inline_value(arg: &str) -> bool { /// Find the first matching git subcommand, skipping known global options that /// may appear before it (e.g., `-C`, `-c`, `--git-dir`). -fn find_git_subcommand<'a>( +/// +/// 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)> { 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 20c10a9a8a75..7a41c2779231 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 { @@ -38,72 +42,6 @@ pub fn is_known_safe_command(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`). -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") || 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)); - } - } - - None -} - fn is_safe_to_call_with_exec(command: &[String]) -> bool { let Some(cmd0) = command.first().map(String::as_str) else { return false; From 9df8b81696675b77b7fbc932c0b4aedc3bc73ee9 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 13:14:47 -0800 Subject: [PATCH 04/10] test(core): use assert! for boolean safety checks --- .../command_safety/is_dangerous_command.rs | 122 ++++++++---------- 1 file changed, 54 insertions(+), 68 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 9528f4a886e7..211edd7da38a 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -138,7 +138,6 @@ fn git_branch_is_delete(branch_args: &[String]) -> bool { #[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() @@ -146,112 +145,99 @@ mod tests { #[test] fn git_reset_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["git", "reset"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&["git", "reset"]))); } #[test] fn bash_git_reset_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["bash", "-lc", "git reset --hard"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&[ + "bash", + "-lc", + "git reset --hard", + ]))); } #[test] fn zsh_git_reset_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["zsh", "-lc", "git reset --hard"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&[ + "zsh", + "-lc", + "git reset --hard", + ]))); } #[test] fn git_status_is_not_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["git", "status"])), - false - ); + assert!(!command_might_be_dangerous(&vec_str(&["git", "status"]))); } #[test] fn bash_git_status_is_not_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["bash", "-lc", "git status"])), - false - ); + assert!(!command_might_be_dangerous(&vec_str(&[ + "bash", + "-lc", + "git status", + ]))); } #[test] fn sudo_git_reset_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["sudo", "git", "reset", "--hard"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&[ + "sudo", "git", "reset", "--hard", + ]))); } #[test] fn usr_bin_git_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["/usr/bin/git", "reset", "--hard"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&[ + "/usr/bin/git", + "reset", + "--hard", + ]))); } #[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 - ); + 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_global_options_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["git", "-C", ".", "branch", "-d", "feature"])), - true - ); - assert_eq!( - command_might_be_dangerous(&vec_str(&[ - "git", - "-c", - "color.ui=false", - "branch", - "-D", - "feature", - ])), - true - ); - assert_eq!( - command_might_be_dangerous(&vec_str(&["bash", "-lc", "git -C . branch -d feature",])), - true - ); + 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 rm_rf_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"]))); } #[test] fn rm_f_is_dangerous() { - assert_eq!( - command_might_be_dangerous(&vec_str(&["rm", "-f", "/"])), - true - ); + assert!(command_might_be_dangerous(&vec_str(&["rm", "-f", "/"]))); } } From 552c05cff4483132ca2ca618b6a2780f7e25e91c Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 13:41:03 -0800 Subject: [PATCH 05/10] fix(core): stop scanning git args after first subcommand --- .../src/command_safety/is_dangerous_command.rs | 14 ++++++++++++++ .../core/src/command_safety/is_safe_command.rs | 9 +++++++++ 2 files changed, 23 insertions(+) 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 211edd7da38a..5ccd25384224 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -90,6 +90,11 @@ pub(crate) fn find_git_subcommand<'a>( 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 @@ -231,6 +236,15 @@ mod tests { ]))); } + #[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 rm_rf_is_dangerous() { assert!(command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"]))); 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 7a41c2779231..87f562fe9158 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -311,6 +311,15 @@ mod tests { ); } + #[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 zsh_lc_safe_command_sequence() { assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"]))); From 6468488fd6f4c9c6f7ddfc04089178f524228e38 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 14:00:27 -0800 Subject: [PATCH 06/10] fix(core): flag force push and clean as dangerous --- .../command_safety/is_dangerous_command.rs | 66 ++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) 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 5ccd25384224..5539b8c0ce98 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -106,7 +106,7 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { match cmd0 { Some(cmd) if cmd.ends_with("git") || cmd.ends_with("/git") => { let Some((subcommand_idx, subcommand)) = - find_git_subcommand(command, &["reset", "rm", "branch"]) + find_git_subcommand(command, &["reset", "rm", "branch", "push", "clean"]) else { return false; }; @@ -114,6 +114,8 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { match subcommand { "reset" | "rm" => true, "branch" => git_branch_is_delete(&command[subcommand_idx + 1..]), + "push" => git_push_is_force(&command[subcommand_idx + 1..]), + "clean" => git_clean_is_force(&command[subcommand_idx + 1..]), other => { debug_assert!(false, "unexpected git subcommand from matcher: {other}"); false @@ -140,6 +142,29 @@ fn git_branch_is_delete(branch_args: &[String]) -> bool { }) } +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_force(push_args: &[String]) -> bool { + push_args.iter().map(String::as_str).any(|arg| { + matches!( + arg, + "--force" | "--force-with-lease" | "--force-if-includes" | "-f" + ) || arg.starts_with("--force-with-lease=") + || arg.starts_with("--force-if-includes=") + || short_flag_group_contains(arg, 'f') + }) +} + +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::*; @@ -245,6 +270,45 @@ mod tests { ]))); } + #[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_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", + ]))); + } + #[test] fn rm_rf_is_dangerous() { assert!(command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"]))); From f8c7b477d60d7b6ed2f42f79b5189df6fe6a5205 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 14:07:48 -0800 Subject: [PATCH 07/10] fix(core): tighten git and cargo safe-command rules --- .../src/command_safety/is_safe_command.rs | 88 +++++++++++++++++-- 1 file changed, 83 insertions(+), 5 deletions(-) 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 87f562fe9158..e52079c74bb7 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -136,15 +136,29 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { // Git 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; + } + 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" => true, - "branch" => git_branch_is_read_only(&command[subcommand_idx + 1..]), + "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 @@ -152,9 +166,6 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } } - // Rust - Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true, - // Special-case `sed -n {N|M,N}p` Some("sed") if { @@ -199,6 +210,32 @@ fn git_branch_is_read_only(branch_args: &[String]) -> bool { 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) /* ---------------------------------------------------------- @@ -320,6 +357,47 @@ mod tests { ]))); } + #[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"]))); From 25ce92dde7e9385e8a6b8335122ede7d392a67c2 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 15:47:48 -0800 Subject: [PATCH 08/10] fix(core): remove redundant git suffix check and document stacked branch delete flags --- .../command_safety/is_dangerous_command.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 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 5539b8c0ce98..6c520a25ae63 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -61,7 +61,7 @@ pub(crate) fn find_git_subcommand<'a>( subcommands: &[&str], ) -> Option<(usize, &'a str)> { let cmd0 = command.first().map(String::as_str)?; - if !(cmd0.ends_with("git") || cmd0.ends_with("/git")) { + if !cmd0.ends_with("git") { return None; } @@ -104,7 +104,7 @@ 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") => { + Some(cmd) if cmd.ends_with("git") => { let Some((subcommand_idx, subcommand)) = find_git_subcommand(command, &["reset", "rm", "branch", "push", "clean"]) else { @@ -134,6 +134,8 @@ 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 + // `-d*`/`-D*` group as a delete flag unless it is exactly `-d`/`-D`. branch_args.iter().any(|arg| { matches!(arg.as_str(), "-d" | "-D" | "--delete") || arg.starts_with("--delete=") @@ -241,6 +243,19 @@ mod tests { ]))); } + #[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", "-Dvv", "feature", + ]))); + } + #[test] fn git_branch_delete_with_global_options_is_dangerous() { assert!(command_might_be_dangerous(&vec_str(&[ From b3d811c3381b7f2c7828ef6e2723529ef47f57a6 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 30 Jan 2026 16:06:19 -0800 Subject: [PATCH 09/10] fix(core): detect git branch delete flags in grouped short options --- .../core/src/command_safety/is_dangerous_command.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 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 6c520a25ae63..e8b961abf3ad 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -135,12 +135,12 @@ 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 - // `-d*`/`-D*` group as a delete flag unless it is exactly `-d`/`-D`. - branch_args.iter().any(|arg| { - matches!(arg.as_str(), "-d" | "-D" | "--delete") + // 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=") - || (arg.starts_with("-d") && arg != "-d") - || (arg.starts_with("-D") && arg != "-D") + || short_flag_group_contains(arg, 'd') + || short_flag_group_contains(arg, 'D') }) } @@ -251,6 +251,9 @@ mod tests { 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", ]))); From d3ce7907bcc2129dd400acc854a4c059cbeadd8c Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Sun, 1 Feb 2026 13:37:05 -0800 Subject: [PATCH 10/10] fix(core): flag dangerous git push refspec and delete forms --- .../command_safety/is_dangerous_command.rs | 49 +++++++++++++++++-- codex-rs/core/src/exec_policy.rs | 24 +++++++++ 2 files changed, 70 insertions(+), 3 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 e8b961abf3ad..3e2c669c4419 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -114,7 +114,7 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { match subcommand { "reset" | "rm" => true, "branch" => git_branch_is_delete(&command[subcommand_idx + 1..]), - "push" => git_push_is_force(&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}"); @@ -148,17 +148,25 @@ 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_force(push_args: &[String]) -> bool { +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" | "-f" + "--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") @@ -307,6 +315,41 @@ mod tests { ]))); } + #[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(&[ diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index c0cb7f8fcf90..a2e1e8d40d8d 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() }