fix: preserve positional operands in high-level Git commands - #2247
Merged
Merged
Conversation
<!-- agent --> `Remote.pull()` validated keyword options but forwarded refspecs through `git pull`, whose internal fetch invocation loses the `--` separator. GHSA-f9j4-qggq-h239 reports the resulting positional validation bypass. Reject all leading-dash refspecs and remote names before spawning Git, rather than trying to enumerate dangerous option spellings. This also applies with `allow_unsafe_options=True`: explicit options still belong in keyword arguments. Document the behavior and add an unreleased changelog entry. No shell quoting change is needed; the problem is Git's own option parsing, not splitting arguments in Python. Git reference: `builtin/pull.c:run_fetch()` at Git commit `12cb6293d6288865c1a133cf22accbaf99d13eb6` forwards the remote and refspecs without an option terminator. Tested with Apple Git 2.54.0. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>
<!-- agent -->
A leading-dash name could change the operation requested by a high-level
API. In particular, `delete_head("--force", branch, force=False)` deleted
an unmerged branch, and renaming a `Head` named `--force` renamed the
current branch instead of rejecting the missing source.
Terminate options before names in branch deletion and rename, remote
reference deletion, and tag creation and deletion. Keep explicit keyword
options and the existing unsafe-option checks. This protects operands as
a class instead of enumerating individual dangerous flags.
Git reference: `builtin/branch.c` and `builtin/tag.c` use `parse_options()`
with support for `--` at Git commit
`12cb6293d6288865c1a133cf22accbaf99d13eb6`.
Assisted-by: GPT 6.0
Co-authored-by: GPT 6.0 <codex@openai.com>
<!-- agent --> `IndexFile.move()` forwarded paths without an option separator in both its preliminary dry run and its actual move. A `--force` operand could overwrite an existing destination, and `--no-dry-run` could move files even when the caller explicitly requested `dry_run=True`. `Repo.ignored()` likewise interpreted leading-dash filenames as options. Insert `--` before paths in both wrappers. The shared move argument list protects both invocations; explicit keyword options still work. Literal leading-dash filenames, including values containing spaces, remain usable. Update the unreleased changelog for both APIs. Git reference: `builtin/mv.c` and `builtin/check-ignore.c` use `parse_options()` with `--` support at Git commit `12cb6293d6288865c1a133cf22accbaf99d13eb6`. Validation: all four new path regressions failed before the fix. The positional, index, and repository suites pass 153 tests and 14 subtests, with four skips, using test-local `init.defaultBranch=master` and `core.quotePath=true`. One unrelated revision-parsing test was excluded: it traverses pre-existing checkpoint refs pointing at trees. Tested with Apple Git 2.54.0; `git diff --check` passes. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>
<!-- agent --> `Remote.update()` also crosses an internal Git command boundary: Git's `remote update` parses options, then forwards the remaining operands to `fetch --multiple` without an option terminator. A leading-dash remote name can therefore change the requested operation or its target set. Reject such names before dispatch. Document at `_call_process()` why high-level wrappers must protect operands, while the low-level runner must continue supporting deliberately positional options. Shell quoting does not affect Git's own option parsing. Add an unreleased changelog entry. Git reference: `builtin/remote.c:update()` at Git commit `12cb6293d6288865c1a133cf22accbaf99d13eb6` constructs the internal fetch command without preserving `--`. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>
Byron
force-pushed
the
pos-arg-sanitization
branch
from
September 20, 2026 09:27
ffd601c to
edcd66f
Compare
Byron
marked this pull request as ready for review
September 20, 2026 10:16
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Address the moderate Remote.pull() validation ordering issue; strengthen the affected regression test.
Review effort: Lite
Findings: None
What changed in this PR
This security-focused PR protects high-level GitPython commands from option-shaped operands by preserving operand boundaries or rejecting unsafe values.
Changes:
- Adds
--separators for references, tags, paths, moves, and ignore checks. - Rejects unsafe
pullandremote updateoperands. - Adds regression tests and security advisory documentation.
Review findings:
- moderate (1 vote):
Remote.pull()may raise a missing-refspec assertion before rejecting an option-shaped remote. - nit (1 vote): A rename regression test does not use an option-shaped operand and may pass before the fix.
| File | Summary |
|---|---|
test/test_positional_args.py |
Adds positional-argument regression coverage. |
git/repo/base.py |
Protects ignore path operands. |
git/remote.py |
Validates unsafe pull and update operands. |
git/refs/tag.py |
Protects tag operands. |
git/refs/remote.py |
Protects remote-reference deletion. |
git/refs/head.py |
Protects branch operands. |
git/index/base.py |
Protects move path operands. |
git/cmd.py |
Documents operand-boundary behavior. |
doc/source/changes.rst |
Records the security advisory. |
💡 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.
High-level refspec, reference-name, and path arguments can reach Git's option parser. This change preserves those values as operands, and rejects option-shaped values when an internal Git invocation cannot preserve that boundary.
da2fc3dd: reject option-shapedRemote.pull()operands, including when unsafe keyword options are explicitly enabled.f5dd10ce: separate branch and tag operands from command options.65ed3f8f: preserve path operands in index moves and ignore checks, including explicit overwrite and dry-run settings.ffd601cb: reject option-shapedRemote.update()targets and document the high-level operand boundary.Each fix updates
doc/source/changes.rst. Explicit keyword options retain their behavior. The low-levelGitrunner still accepts intentional positional options.Advisory summary
GHSA-f9j4-qggq-h239: high severity; PyPI package
GitPython; reported affected range>= 3.1.30; no patched release or CVE currently recorded. The advisory is unpublished; this summary omits its reproduction and exploit details.Validation
init.defaultBranch=masterandcore.quotePath=trueto match existing test assumptions.git diff --check, pass.Git behavior was checked against
builtin/pull.c,builtin/remote.c, and the corresponding branch, tag, move, and ignore parsers in Git commit12cb6293d6288865c1a133cf22accbaf99d13eb6. Local runtime: Apple Git 2.54.0 and Python 3.14.