From db22c91e61cc80defa5bbafc22bd8a8c7672e5e1 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Tue, 5 May 2026 17:49:42 -0700 Subject: [PATCH] Share Git safe-command logic on Windows (#21275) ## Why BUGB-15601 showed that the Windows safe-command path had drifted from the generic Git classifier. The Windows-specific Git parser could classify a PowerShell-wrapped `git` command as safe as soon as it found a safelisted subcommand, without applying the generic checks for unsafe subcommand options such as `--output`, `--ext-diff`, `--textconv`, `--paginate`, or `cat-file --filters`. The generic classifier already models the Git command boundary and the read-only argument checks more carefully, so Windows should reuse that logic instead of maintaining a smaller parallel parser. ## What Changed - Extracted the existing generic Git classification logic into `is_safe_git_command`. - Updated `windows_safe_commands.rs` to call that shared helper for parsed PowerShell `git` commands. - Removed the Windows-only Git subcommand safelist, including the `cat-file` allowance that was part of the reported bypass. - Added a Windows regression test that keeps PowerShell-wrapped Git commands with side-effecting options classified unsafe. - Made the full-path PowerShell test discover the installed PowerShell executable instead of depending on one hard-coded `pwsh.exe` path. ## Verification - `cargo test -p codex-shell-command rejects_git_subcommand_options_with_side_effects` - `cargo test -p codex-shell-command git_global_override_flags_are_not_safe` - `cargo test -p codex-shell-command windows_powershell_full_path_is_safe -- --nocapture` Co-authored-by: Codex --- .../src/command_safety/is_safe_command.rs | 69 ++++++++++--------- .../command_safety/windows_safe_commands.rs | 69 +++++++++++-------- 2 files changed, 78 insertions(+), 60 deletions(-) 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 5e2ffec9f5..7200672ec1 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 @@ -151,36 +151,7 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } // Git - Some("git") => { - // Global options that redirect config, repository, or helper - // lookup can make otherwise read-only git commands execute - // attacker-controlled code, so they must never be auto-approved. - if git_has_unsafe_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" => { - 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 - } - } - } + Some("git") => is_safe_git_command(command), // Special-case `sed -n {N|M,N}p` Some("sed") @@ -198,6 +169,35 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool { } } +pub(crate) fn is_safe_git_command(command: &[String]) -> bool { + // Global options that redirect config, repository, or helper lookup can make + // otherwise read-only git commands execute attacker-controlled code, so they + // must never be auto-approved. + if git_has_unsafe_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" => 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 + } + } +} + // 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 { @@ -542,8 +542,15 @@ mod tests { return; } + let Some(powershell) = crate::powershell::try_find_pwsh_executable_blocking() + .or_else(crate::powershell::try_find_powershell_executable_blocking) + else { + return; + }; + let powershell = powershell.as_path().to_str().unwrap(); + assert!(is_known_safe_command(&vec_str(&[ - r"C:\Program Files\PowerShell\7\pwsh.exe", + powershell, "-Command", "Get-Location", ]))); 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 1dd628f427..ebe0058f75 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 @@ -1,4 +1,4 @@ -use crate::command_safety::is_dangerous_command::git_global_option_requires_prompt; +use crate::command_safety::is_safe_command::is_safe_git_command; use crate::command_safety::powershell_parser::PowershellParseOutcome; use crate::command_safety::powershell_parser::parse_with_powershell_ast; use std::path::Path; @@ -221,37 +221,11 @@ fn is_safe_ripgrep(words: &[String]) -> bool { }) } -/// Ensures a Git command sticks to whitelisted read-only subcommands and flags. -fn is_safe_git_command(words: &[String]) -> bool { - const SAFE_SUBCOMMANDS: &[&str] = &["status", "log", "show", "diff", "cat-file"]; - - for arg in words.iter().skip(1) { - let arg_lc = arg.to_ascii_lowercase(); - - if arg.starts_with('-') { - if git_global_option_requires_prompt(&arg_lc) - || arg.eq_ignore_ascii_case("--config") - || arg_lc.starts_with("--config=") - { - // Examples rejected here: "pwsh -Command 'git --git-dir=.evil-git diff'" and - // "pwsh -Command 'git -c core.pager=cat show HEAD:foo.rs'". - return false; - } - - continue; - } - - return SAFE_SUBCOMMANDS.contains(&arg_lc.as_str()); - } - - // Examples rejected here: "pwsh -Command 'git'" and "pwsh -Command 'git status --short | Remove-Item foo'". - false -} - #[cfg(all(test, windows))] mod tests { use super::*; use crate::powershell::try_find_pwsh_executable_blocking; + use pretty_assertions::assert_eq; use std::string::ToString; /// Converts a slice of string literals into owned `String`s for the tests. @@ -342,7 +316,7 @@ mod tests { assert!(is_safe_command_windows(&[ pwsh.clone(), "-Command".to_string(), - "-git cat-file -p HEAD:foo.rs".to_string() + "git show HEAD:foo.rs".to_string() ])); assert!(is_safe_command_windows(&[ @@ -393,6 +367,43 @@ mod tests { } } + #[test] + fn rejects_git_subcommand_options_with_side_effects() { + let results: Vec<(&str, bool)> = [ + "git diff --output codex_poc.txt", + "git diff --ext-diff HEAD", + "git log --textconv -1", + "git log --paginate -1", + "git show --output=codex_poc.txt HEAD", + "git cat-file --filters HEAD:a.txt", + ] + .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 diff --output codex_poc.txt", false), + ("git diff --ext-diff HEAD", false), + ("git log --textconv -1", false), + ("git log --paginate -1", false), + ("git show --output=codex_poc.txt HEAD", false), + ("git cat-file --filters HEAD:a.txt", false), + ], + results + ); + } + #[test] fn rejects_powershell_commands_with_side_effects() { assert!(!is_safe_command_windows(&vec_str(&[