Skip to content

guard unsafe git command options#2163

Merged
Byron merged 1 commit into
mainfrom
fix-unguarded-blame
Jul 12, 2026
Merged

guard unsafe git command options#2163
Byron merged 1 commit into
mainfrom
fix-unguarded-blame

Conversation

@Byron

@Byron Byron commented Jun 17, 2026

Copy link
Copy Markdown
Member

Tasks

  • basic review of the Codex 5.5 based rework of the previous PR. Didn't even look at the copilot comments and their auto-resolution.
  • @EliahKagan 's in-depth review and validation of the advisory

Advisory summary

  • Advisory: GHSA-956x-8gvw-wg5v
  • GHSA: GHSA-956x-8gvw-wg5v
  • Severity: high
  • Affected package: GitPython (pip)
  • Vulnerable range: <= 3.1.50
  • Patched versions: not specified in advisory
  • CVE: not listed
  • Issue pattern: command execution / argument injection via unguarded command options

Validation:

  • Added regression tests for archive, iter_commits, blame, and git ls-remote unsafe option handling.
  • Focused validation not runnable here due missing pytest module in environment.

Created by Codex on behalf of Byron. Byron will review before this is ready to merge.

Copilot AI review requested due to automatic review settings June 17, 2026 04:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds protections against unsafe git options (notably options that can lead to command execution or arbitrary file writes) by introducing default rejection behavior plus an explicit allow_unsafe_options opt-in, along with regression tests.

Changes:

  • Add unsafe-option checks for archive, rev-based traversal APIs (iter_commits, Commit.iter_items), and blame APIs with allow_unsafe_options overrides.
  • Add a dedicated Git.ls_remote() wrapper that rejects unsafe options by default (opt-in supported).
  • Add regression tests covering unsafe-option rejection and opt-in behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
git/repo/base.py Adds unsafe option lists and default guarding for archive, iter_commits, and blame APIs via allow_unsafe_options.
git/objects/commit.py Adds unsafe revision option guarding to Commit.iter_items plus allow_unsafe_options.
git/cmd.py Adds ls_remote() wrapper and blocks unsafe options by default with opt-in.
test/test_repo.py Adds regression tests ensuring unsafe options are rejected for archive, iter_commits, and blame.
test/test_remote.py Adds regression tests for unsafe ls-remote options rejection and opt-in behavior.
test/test_commit.py Adds regression test ensuring Commit.iter_items rejects unsafe revisions.
Comments suppressed due to low confidence (2)

git/repo/base.py:788

  • For rev-based wrappers, the unsafe-option check only inspects the rev argument string. However, --output/-o are primarily options and can also be supplied via kwargs (and via rev_opts in blame()), which this change currently doesn’t guard. Mandatory: when allow_unsafe_options is false, validate unsafe options across all user-controlled option channels for these commands (e.g., kwargs option keys and rev_opts entries), not just rev.
    def iter_commits(

git/repo/base.py:1208

  • For rev-based wrappers, the unsafe-option check only inspects the rev argument string. However, --output/-o are primarily options and can also be supplied via kwargs (and via rev_opts in blame()), which this change currently doesn’t guard. Mandatory: when allow_unsafe_options is false, validate unsafe options across all user-controlled option channels for these commands (e.g., kwargs option keys and rev_opts entries), not just rev.
    def blame(

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread git/repo/base.py
Comment thread git/repo/base.py Outdated
Comment thread git/cmd.py
Comment thread git/cmd.py
Comment thread git/cmd.py
Comment thread git/repo/base.py
Comment thread git/repo/base.py Outdated
Comment thread git/repo/base.py Outdated
Comment thread test/test_repo.py Outdated
Comment thread test/test_repo.py Outdated
@Byron Byron force-pushed the fix-unguarded-blame branch from d0a9007 to 0a52e59 Compare June 17, 2026 05:52
Copilot AI review requested due to automatic review settings June 17, 2026 05:55
@Byron Byron force-pushed the fix-unguarded-blame branch from 0a52e59 to 550d74e Compare June 17, 2026 05:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comment thread git/repo/base.py
Comment thread git/cmd.py
Comment thread git/objects/commit.py Outdated
Comment thread git/repo/base.py Outdated
Comment thread test/test_repo.py Outdated
@Byron Byron force-pushed the fix-unguarded-blame branch from 550d74e to cf7ea86 Compare June 17, 2026 06:03
Copilot AI review requested due to automatic review settings June 17, 2026 06:06
@Byron Byron force-pushed the fix-unguarded-blame branch from cf7ea86 to 114cd76 Compare June 17, 2026 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment thread git/repo/base.py
Comment thread git/repo/base.py Outdated
Comment thread git/cmd.py

@EliahKagan EliahKagan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Many of the changes here seem good and even necessary, but I'm not sure there's really a way to improve things overall by selectively blocking or redirecting dynamic commands on Git instances. The purpose of Git instances is to pass arguments straightforwardly into git commands via Git.execute. I've posted this comment, which references that previous decision. To be clear, as noted there, I am not confident that I fully understand all aspects of this situation; but just in case I am not totally mistaken, I wanted to raise this concern rather than waiting until after a release that might introduce new potentially unsatisfiable guarantees.

@Byron Byron marked this pull request as draft June 17, 2026 07:17
@Byron

Byron commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

My apologies, this PR isn't supposed to be undrafted, nor is Codex 5.3 spark (too dumb) supposed to write into my checkboxes.

I didn't even take a look at it yet.

@EliahKagan

EliahKagan commented Jun 17, 2026

Copy link
Copy Markdown
Member

Thanks for clarifying. I didn't look at the checkboxes, but the non-draft status was a factor that nudged me to comment here (rather than only in the linked thread).

@Byron Byron marked this pull request as ready for review June 17, 2026 09:17
Copilot AI review requested due to automatic review settings June 17, 2026 09:17
@Byron Byron requested a review from EliahKagan June 17, 2026 09:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment thread git/cmd.py
Comment thread git/objects/commit.py Outdated
@Byron

Byron commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

This might at least be a partial solution, as it would fix iter_items(). The other commands are probably not exploitable, but I leave this to the second and more thorough review.

Copilot AI review requested due to automatic review settings July 9, 2026 09:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment thread git/cmd.py Outdated
Comment thread git/repo/base.py
Copilot AI review requested due to automatic review settings July 12, 2026 12:59
@Byron Byron force-pushed the fix-unguarded-blame branch 2 times, most recently from 5037f36 to 3af2b29 Compare July 12, 2026 13:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Byron Byron force-pushed the fix-unguarded-blame branch from 3af2b29 to 9450bf6 Compare July 12, 2026 13:30
Reject command options that can execute arbitrary helpers or write files when passed through archive, revision traversal, blame, clone, fetch, pull, push, and ls-remote APIs. Validate positional option tokens and transformed keyword options consistently, including Git-supported long-option abbreviations, attached short-option values, and falsey stringifiable remotes. Preserve explicit allow_unsafe_options and allow_unsafe_protocols escape hatches.

Add regression coverage for each guarded API, option spelling and abbreviation forms, inert keyword values, mixed positional and keyword candidates, tuple blame options, stringifiable remotes, and unsafe commands that must not create marker files.

Reviewed-by: Sebastian Thiel <sebastian.thiel@icloud.com>
@Byron

Byron commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

Let's opt for a post-release fix as needed rather than holding this up for much longer.

Once again, I ran the tests locally and rely on the few jobs that pass, knowing the other ones won't be picked up due to runner shortage or GH issues.

Lastly, I have used GPT 5.6 on all of this, and let its review (high thinking) found yet another issue that is fixed here.
This doesn't mean it's perfect, but it's probably better than anything before it.

Lastly, I only performed a review, no refackiew, so admittedly this is mostly agent work and I probably wouldn't see issues.

@Byron Byron merged commit af027be into main Jul 12, 2026
23 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants