guard unsafe git command options by Byron · Pull Request #2163 · gitpython-developers/GitPython

Byron · 2026-06-17T04:32:21Z

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: https://github.com/gitpython-developers/GitPython/security/advisories/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

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.

Copilot

Pull request overview

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

Copilot

Pull request overview

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

EliahKagan

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 · 2026-06-17T07:19:01Z

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 · 2026-06-17T07:36:13Z

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).

Centralize unsafe-option candidate gathering so callers consistently validate keyword spellings before Git command execution, and validate only positional values that actually look like options. This makes the existing canonical unsafe-option checker explicit at each call site and avoids duplicating ad hoc kwargs handling. Clean up the archive unsafe option list to only include git-archive options, add unsafe protocol validation for archive remotes without changing the existing positional allow_unsafe_options API, accept tuple-style blame rev_opts by normalizing them once, and update regression tests to use non-existent marker paths so they assert blocked commands do not create output. Validation: - uv run --with-requirements requirements.txt --with-requirements test-requirements.txt pytest test/test_git.py::TestGit::test_check_unsafe_options_normalizes_kwargs test/test_commit.py::TestCommit::test_iter_items_rejects_unsafe_revision test/test_commit.py::TestCommit::test_iter_items_rejects_unsafe_options test/test_repo.py::TestRepo::test_archive_rejects_unsafe_options test/test_repo.py::TestRepo::test_archive_rejects_unsafe_remote_protocol test/test_repo.py::TestRepo::test_archive_preserves_positional_allow_unsafe_options test/test_repo.py::TestRepo::test_archive_accepts_stringifiable_remote test/test_repo.py::TestRepo::test_iter_commits_rejects_unsafe_revision test/test_repo.py::TestRepo::test_blame_rejects_unsafe_revision test/test_repo.py::TestRepo::test_blame_rejects_unsafe_options test/test_repo.py::TestRepo::test_blame_rejects_unsafe_rev_opts test/test_remote.py::TestRemote::test_ls_remote_unsafe_options test/test_remote.py::TestRemote::test_ls_remote_allows_operand_named_like_unsafe_option - uv run --with ruff ruff check git/cmd.py git/repo/base.py git/objects/commit.py git/remote.py test/test_repo.py test/test_remote.py

Copilot

Pull request overview

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

+    def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[str, Any]] = None) -> List[str]:
+        """Collect possible option spellings before command-line transformation."""
+        options = [
+            option for option in cls._unpack_args([arg for arg in args if arg is not None]) if option.startswith("-")
+        ]
+        if kwargs:
+            options.extend(str(key) for key in kwargs)
+        return options


+    unsafe_git_rev_options = [
+        # This option can truncate or write arbitrary files before revision parsing.
+        "--output",
+        "-o",
+    ]
+    """Options to :manpage:`git-rev-list(1)` that can overwrite files."""


Byron · 2026-06-17T09:27:13Z

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 June 17, 2026 04:32

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Copilot started reviewing on behalf of Byron June 17, 2026 04:44 View session

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 force-pushed the fix-unguarded-blame branch from 0a52e59 to 550d74e Compare June 17, 2026 05:55

Copilot AI reviewed Jun 17, 2026

View reviewed changes

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 force-pushed the fix-unguarded-blame branch from 550d74e to cf7ea86 Compare June 17, 2026 06:03

guard unsafe git command options (GHSA-956x-8gvw-wg5v)

114cd76

Copilot started reviewing on behalf of Byron June 17, 2026 06:06 View session

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

Byron force-pushed the fix-unguarded-blame branch from cf7ea86 to 114cd76 Compare June 17, 2026 06:06

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Comment thread git/repo/base.py

Comment thread git/repo/base.py Outdated

Comment thread git/cmd.py

EliahKagan reviewed Jun 17, 2026

View reviewed changes

Copilot started reviewing on behalf of Byron June 17, 2026 06:25 View session

Fix unsafe option assertion in Commit.iter_items test

c1da935

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

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

Copilot started reviewing on behalf of Byron June 17, 2026 09:18 View session

Byron requested a review from EliahKagan June 17, 2026 09:19

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Byron added the waiting for feedback label Jun 17, 2026

Uh oh!

Conversation

Byron commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

Advisory summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EliahKagan left a comment

Choose a reason for hiding this comment

Uh oh!

Byron commented Jun 17, 2026

Uh oh!

EliahKagan commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Byron commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Byron commented Jun 17, 2026 •

edited

Loading

EliahKagan commented Jun 17, 2026 •

edited

Loading