fix: complete safety checks in Git command wrappers - #2251
Merged
Merged
Conversation
Mostly a rubber-stamp, particularly the tests I just skimmed. It's also a common fix just with unsafe-options guards, and a regex fix which hopefully is truly better than before. I didn't question it in the interest of time. <!-- agent --> Address GHSA-w8jc-g24h-crhw by applying the existing guard policy to `Git.ls_remote()`, `Repo.merge_base()`, and `IndexFile.move()`. These wrappers previously omitted protocol or option checks already used by sibling APIs. Check flattened positional arguments and split short-option values before `ls_remote()` starts Git, including values that become the repository after option parsing. Add the independent `allow_unsafe_protocols` opt-in and recognize helper selectors even when their address is empty or begins with a newline. Match Git's scheme-character rules at the start of the address so ordinary IPv6 URLs and double colons in repository paths retain their meaning. This addresses the review finding that the broad matcher rejected valid IPv6 remotes. The protocol check is conservative for positional and split option values; long-form `server_option` values remain available without a protocol opt-in. Reuse the revision and pathspec option guards in `merge_base()` and `move()`, with explicit `allow_unsafe_options` opt-ins. Git currently rejects these denylisted options for those subcommands; the checks keep their policy aligned with sibling APIs. Preserve literal move operands behind `--` and validate options before either the dry run or actual move. Only treat exit status 1 from `merge_base()` as no common ancestor. Other failures, including invalid options, now propagate as `GitCommandError` instead of silently returning an empty list. Git reference: `git/git@d38352cd43ab9745686d697872408bc3249a153f`, inspected in `builtin/ls-remote.c`, `transport.c`, `builtin/merge-base.c`, `builtin/mv.c`, `url.c`, and `parse-options.c`. These confirm option parsing before the remote operand, helper selection independently of address contents, long-option abbreviations, and status 1 for unrelated histories. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical release consistency and moderate correctness issues remain.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR adds safety guards to Git command wrappers and improves merge_base() error handling.
Changes:
- Guards
ls_remote(),merge_base(), andIndexFile.move(). - Adds regression tests for unsafe protocols, options, and merge-base failures.
- Documents the security fixes.
| File | Summary | Review notes |
|---|---|---|
test/test_command_guards.py |
Adds regression coverage for command guards. | No issues noted. |
git/repo/base.py |
Validates merge-base options and propagates failures. | Moderate issue: status 1 is mishandled with is_ancestor. |
git/index/base.py |
Adds unsafe option validation for moves. | Nit: reference git-mv(1) instead of git-rm(1). |
git/cmd.py |
Adds protocol and option checks to ls_remote(). |
Moderate issue: boolean keyword values can cause a TypeError. |
doc/source/changes.rst |
Documents the security release. | Critical version mismatch; nit: add the required release URL. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by
Codex GPT-6.Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Apply the existing safety guards consistently to
Git.ls_remote(),Repo.merge_base(), andIndexFile.move(), retaining explicit unsafe opt-ins.merge_base()now propagates command failures except for Git's status 1 result when no common ancestor exists.The shared protocol check follows Git's transport-selector syntax, preserving ordinary IPv6 URLs and double colons in repository paths. Protocol-like positional arguments and split short-option values require an opt-in; long-form server options remain available. The option checks for
merge_base()andmove()align their policy with sibling APIs; current Git versions already reject those denylisted options for these two subcommands.Advisory summary
GHSA-w8jc-g24h-crhw
GitPythonon PyPI; reported affected range<= 3.1.62.The advisory is unpublished. Private reproduction and exploit details are omitted.
Validation
Regression cases failed before the fix. On Python 3.14.7 with Git 2.54.0 (Apple Git-157):
mypy: clean across 46 source files.basedpyright --warnings: no errors or warnings.codex review --commit c480bd5: no actionable findings.Git behavior was checked against
git/git@d38352cd43ab9745686d697872408bc3249a153f, specificallybuiltin/ls-remote.c,transport.c,builtin/merge-base.c,builtin/mv.c,url.c, andparse-options.c.Commit:
c480bd5—fix: complete safety checks in Git command wrappers.