Skip to content

Share Git safe-command logic on Windows#21275

Merged
iceweasel-oai merged 1 commit intomainfrom
codex/fix-windows-git-safe-command
May 6, 2026
Merged

Share Git safe-command logic on Windows#21275
iceweasel-oai merged 1 commit intomainfrom
codex/fix-windows-git-safe-command

Conversation

@iceweasel-oai
Copy link
Copy Markdown
Collaborator

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

@iceweasel-oai iceweasel-oai marked this pull request as ready for review May 6, 2026 00:22
Copy link
Copy Markdown
Contributor

@evawong-oai evawong-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes the shared Git classifier the Windows source of truth, but that classifier need to also consider pager options when they appear before the subcommand.

Examples:
git --paginate log -1
git -p log -1

The current check rejects --paginate only after the matched subcommand. Git treats this as a global option too, so find_git_subcommand skips it, then the read only arg check never sees it.

Since the PR is specifically consolidating Git safety logic, I think we should either reject -p and --paginate as unsafe global Git options in this PR, or create a clear follow up before landing.

@iceweasel-oai iceweasel-oai merged commit db22c91 into main May 6, 2026
26 checks passed
@iceweasel-oai iceweasel-oai deleted the codex/fix-windows-git-safe-command branch May 6, 2026 00:49
@github-actions github-actions Bot locked and limited conversation to collaborators May 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants