mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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 <codex@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
794c240f25
commit
db22c91e61
@@ -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",
|
||||
])));
|
||||
|
||||
Reference in New Issue
Block a user