From 790bb3164948625ffb043d2a162425d55923fe9a Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 20 Sep 2026 10:54:19 +0200 Subject: [PATCH 1/4] fix(remote): reject option-shaped pull operands `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 --- doc/source/changes.rst | 1 + git/remote.py | 11 ++++++++-- test/test_positional_args.py | 42 ++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 test/test_positional_args.py diff --git a/doc/source/changes.rst b/doc/source/changes.rst index df003c7d3..7fcd68537 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -9,6 +9,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58 * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-23mf-xhv8-69c2 +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-f9j4-qggq-h239 If you can, also try and provide feedback on the upcoming v4 branch https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. diff --git a/git/remote.py b/git/remote.py index fd58c8bd5..df13a4ee7 100644 --- a/git/remote.py +++ b/git/remote.py @@ -14,7 +14,7 @@ from git.cmd import Git, handle_process_output from git.compat import defenc, force_text from git.config import GitConfigParser, SectionConstraint, cp -from git.exc import GitCommandError +from git.exc import GitCommandError, UnsafeOptionError from git.refs import Head, Reference, RemoteReference, SymbolicReference, TagReference from git.util import ( CallableRemoteProgress, @@ -1101,7 +1101,8 @@ def pull( merge of branch with your local branch. :param refspec: - See :meth:`fetch` method. + See :meth:`fetch` method. Values starting with ``-`` are rejected, + even when ``allow_unsafe_options`` is enabled. Pass options as keywords. :param progress: See :meth:`push` method. @@ -1127,6 +1128,12 @@ def pull( kwargs = add_progress(kwargs, self.repo.git, progress) refspec = Git._unpack_args(refspec or []) + # Git pull forwards these operands to fetch without preserving `--`. + # Reject every option-shaped operand, including with unsafe options enabled: + # opting into an explicit option must not turn a refspec into an option. + for operand in [self.name, *refspec]: + if operand.startswith("-"): + raise UnsafeOptionError("Remote names and pull refspecs must not start with '-'.") if not allow_unsafe_protocols: for ref in refspec: Git.check_unsafe_protocols(ref) diff --git a/test/test_positional_args.py b/test/test_positional_args.py new file mode 100644 index 000000000..668e9ae6e --- /dev/null +++ b/test/test_positional_args.py @@ -0,0 +1,42 @@ +"""High-level operands must not become Git options.""" + +from unittest import mock + +import pytest + +from git import Git, Remote, Repo +from git.exc import UnsafeOptionError + + +@pytest.mark.parametrize("allow_unsafe_options", [False, True]) +@pytest.mark.parametrize( + "refspec", + ["--upload-pack=helper", ["--upl=helper"], ["main", "--dry-run"], "-uhelper", "--arg value", "--"], +) +def test_pull_rejects_option_shaped_refspec(tmp_path, refspec, allow_unsafe_options): + repo = Repo.init(tmp_path) + remote = Remote(repo, "origin") + with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeOptionError): + remote.pull(refspec, allow_unsafe_options=allow_unsafe_options) + run.assert_not_called() + + +def test_pull_rejects_option_shaped_remote(tmp_path): + repo = Repo.init(tmp_path) + remote = Remote(repo, "--upload-pack=helper") + with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeOptionError): + remote.pull("main") + run.assert_not_called() + + +def test_pull_preserves_operand_and_explicit_option_values(tmp_path): + repo = Repo.init(tmp_path) + remote = Remote(repo, "origin") + with mock.patch.object(Git, "_call_process") as run, mock.patch.object( + Remote, "_get_fetch_info_from_stderr", return_value=[] + ): + remote.pull("refs/heads/topic", upload_pack="helper with spaces", allow_unsafe_options=True) + assert run.call_args[0] == ("pull", "--", remote, ["refs/heads/topic"]) + assert run.call_args[1]["upload_pack"] == "helper with spaces" From 03b5db24091504d683bea43c60f3f210dc367baa Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 20 Sep 2026 10:56:48 +0200 Subject: [PATCH 2/4] fix(refs): keep branch and tag operands out of option parsing 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 --- git/refs/head.py | 4 ++-- git/refs/remote.py | 2 +- git/refs/tag.py | 4 ++-- test/test_positional_args.py | 42 +++++++++++++++++++++++++++++++++++- 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/git/refs/head.py b/git/refs/head.py index 5dd4a5a0a..c08ce61b3 100644 --- a/git/refs/head.py +++ b/git/refs/head.py @@ -166,7 +166,7 @@ def delete(cls, repo: "Repo", *heads: "Union[Head, str]", force: bool = False, * flag = "-d" if force: flag = "-D" - repo.git.branch(flag, *heads) + repo.git.branch(flag, "--", *heads) def set_tracking_branch(self, remote_reference: Union["RemoteReference", None]) -> "Head": """Configure this branch to track the given remote reference. This will @@ -241,7 +241,7 @@ def rename(self, new_path: PathLike, force: bool = False) -> "Head": if force: flag = "-M" - self.repo.git.branch(flag, self, new_path) + self.repo.git.branch(flag, "--", self, new_path) self.path = "%s/%s" % (self._common_path_default, new_path) return self diff --git a/git/refs/remote.py b/git/refs/remote.py index e16ae70f8..60da3ec0d 100644 --- a/git/refs/remote.py +++ b/git/refs/remote.py @@ -61,7 +61,7 @@ def delete(cls, repo: "Repo", *refs: "RemoteReference", **kwargs: Any) -> None: for ref in refs: cls._check_ref_name_valid(ref.path) - repo.git.branch("-d", "-r", *refs) + repo.git.branch("-d", "-r", "--", *refs) # The official deletion method will ignore remote symbolic refs - these are # generally ignored in the refs/ folder. We don't though and delete remainders # manually. diff --git a/git/refs/tag.py b/git/refs/tag.py index 3a7d946c1..50b8a9cb2 100644 --- a/git/refs/tag.py +++ b/git/refs/tag.py @@ -155,7 +155,7 @@ def create( if force: kwargs["f"] = True - args = (path, reference) + args = ("--", path, reference) repo.git.tag(*args, **kwargs) return TagReference(repo, "%s/%s" % (cls._common_path_default, path)) @@ -163,7 +163,7 @@ def create( @classmethod def delete(cls, repo: "Repo", *tags: "TagReference") -> None: # type: ignore[override] """Delete the given existing tag or tags.""" - repo.git.tag("-d", *tags) + repo.git.tag("-d", "--", *tags) # Provide an alias. diff --git a/test/test_positional_args.py b/test/test_positional_args.py index 668e9ae6e..89e00506d 100644 --- a/test/test_positional_args.py +++ b/test/test_positional_args.py @@ -4,7 +4,7 @@ import pytest -from git import Git, Remote, Repo +from git import Actor, Git, GitCommandError, Head, Remote, RemoteReference, Repo, TagReference from git.exc import UnsafeOptionError @@ -40,3 +40,43 @@ def test_pull_preserves_operand_and_explicit_option_values(tmp_path): remote.pull("refs/heads/topic", upload_pack="helper with spaces", allow_unsafe_options=True) assert run.call_args[0] == ("pull", "--", remote, ["refs/heads/topic"]) assert run.call_args[1]["upload_pack"] == "helper with spaces" + + +def test_delete_head_cannot_override_force(tmp_path): + repo = Repo.init(tmp_path) + actor = Actor("Test", "test@example.com") + initial = repo.index.commit("initial", author=actor, committer=actor) + branch = repo.create_head("unmerged", initial) + branch.commit = repo.index.commit("unmerged", head=False, author=actor, committer=actor) + with pytest.raises(GitCommandError): + repo.delete_head("--force", branch, force=False) + assert branch.is_valid() + repo.delete_head(branch, force=True) + assert not branch.is_valid() + + +def test_rename_head_cannot_select_current_branch(tmp_path): + repo = Repo.init(tmp_path) + actor = Actor("Test", "test@example.com") + repo.index.commit("initial", author=actor, committer=actor) + original = repo.active_branch.name + with pytest.raises(GitCommandError): + Head(repo, "refs/heads/--force").rename("renamed") + assert repo.active_branch.name == original + + +def test_tag_operands_follow_option_terminator(tmp_path): + repo = Repo.init(tmp_path) + with mock.patch.object(Git, "_call_process") as run: + TagReference.create(repo, "topic", "HEAD") + assert run.call_args[0] == ("tag", "--", "topic", "HEAD") + TagReference.delete(repo, "--list") + assert run.call_args[0] == ("tag", "-d", "--", "--list") + + +def test_remote_ref_delete_preserves_operand(tmp_path): + repo = Repo.init(tmp_path) + ref = RemoteReference(repo, "refs/remotes/--force") + with mock.patch.object(Git, "_call_process") as run: + RemoteReference.delete(repo, ref) + assert run.call_args[0] == ("branch", "-d", "-r", "--", ref) From 0d8d84500200e910238b7048356e608f2d26098c Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 20 Sep 2026 11:00:00 +0200 Subject: [PATCH 3/4] fix(index): preserve path operands when moving and checking ignores `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 --- git/index/base.py | 1 + git/repo/base.py | 2 +- test/test_positional_args.py | 38 ++++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/git/index/base.py b/git/index/base.py index 560fc5e2c..b86ba5748 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1133,6 +1133,7 @@ def move( args = [] if skip_errors: args.append("-k") + args.append("--") paths = self._items_to_rela_paths(items) if len(paths) < 2: diff --git a/git/repo/base.py b/git/repo/base.py index 29922e118..cf496201b 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -1137,7 +1137,7 @@ def ignored(self, *paths: PathLike) -> List[str]: Subset of those paths which are ignored """ try: - proc: str = self.git.check_ignore(*paths) + proc: str = self.git.check_ignore("--", *paths) except GitCommandError as err: if err.status == 1: # If return code is 1, this means none of the items in *paths are diff --git a/test/test_positional_args.py b/test/test_positional_args.py index 89e00506d..48d8c2f23 100644 --- a/test/test_positional_args.py +++ b/test/test_positional_args.py @@ -80,3 +80,41 @@ def test_remote_ref_delete_preserves_operand(tmp_path): with mock.patch.object(Git, "_call_process") as run: RemoteReference.delete(repo, ref) assert run.call_args[0] == ("branch", "-d", "-r", "--", ref) + + +def test_move_treats_option_shaped_source_as_filename(tmp_path): + repo = Repo.init(tmp_path) + (tmp_path / "--force").write_text("literal source") + repo.index.add(["--force"]) + assert repo.index.move(["--force", "destination"]) == [("--force", "destination")] + assert (tmp_path / "destination").read_text() == "literal source" + assert not (tmp_path / "--force").exists() + + +def test_move_cannot_override_overwrite_protection(tmp_path): + repo = Repo.init(tmp_path) + for name in ("--force", "source", "destination"): + (tmp_path / name).write_text(name) + repo.index.add(["--force", "source", "destination"]) + with pytest.raises(GitCommandError): + repo.index.move(["--force", "source", "destination"]) + assert (tmp_path / "source").read_text() == "source" + assert (tmp_path / "destination").read_text() == "destination" + repo.index.move(["source", "destination"], force=True) + assert (tmp_path / "destination").read_text() == "source" + + +def test_ignored_treats_option_shaped_path_as_filename(tmp_path): + repo = Repo.init(tmp_path) + (tmp_path / ".gitignore").write_text("--verbose\n--arg value\n") + assert repo.ignored("--verbose", "--arg value") == ["--verbose", "--arg value"] + + +def test_move_cannot_override_dry_run(tmp_path): + repo = Repo.init(tmp_path) + (tmp_path / "source").write_text("source") + repo.index.add(["source"]) + with pytest.raises(GitCommandError): + repo.index.move(["--no-dry-run", "source", "destination"], dry_run=True) + assert (tmp_path / "source").read_text() == "source" + assert not (tmp_path / "destination").exists() From edcd66f80c645a05f48eb714ab6915441110e225 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 20 Sep 2026 11:01:35 +0200 Subject: [PATCH 4/4] fix(remote): reject option-shaped update targets `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 --- git/cmd.py | 6 ++++++ git/remote.py | 3 +++ test/test_positional_args.py | 9 +++++++++ 3 files changed, 18 insertions(+) diff --git a/git/cmd.py b/git/cmd.py index 3d7446906..773231edd 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1795,6 +1795,12 @@ def _call_process( This allows your commands to call git more conveniently, as ``None`` is realized as non-existent. + Positional arguments may intentionally contain command options. Higher-level + APIs must separate their operands with ``--`` where the Git command supports + it, or reject option-shaped operands where Git reparses them internally (for + example, ``pull`` and ``remote update``). Shell quoting cannot prevent Git + from interpreting a leading-dash argument as an option. + :param kwargs: Contains key-values for the following: diff --git a/git/remote.py b/git/remote.py index df13a4ee7..94f3be987 100644 --- a/git/remote.py +++ b/git/remote.py @@ -871,6 +871,9 @@ def update(self, **kwargs: Any) -> "Remote": :return: self """ + # Like pull, remote update forwards operands to fetch without `--`. + if self.name.startswith("-"): + raise UnsafeOptionError("Remote names used by update must not start with '-'.") scmd = "update" kwargs["insert_kwargs_after"] = scmd self.repo.git.remote(scmd, self.name, **kwargs) diff --git a/test/test_positional_args.py b/test/test_positional_args.py index 48d8c2f23..2a7bb405e 100644 --- a/test/test_positional_args.py +++ b/test/test_positional_args.py @@ -118,3 +118,12 @@ def test_move_cannot_override_dry_run(tmp_path): repo.index.move(["--no-dry-run", "source", "destination"], dry_run=True) assert (tmp_path / "source").read_text() == "source" assert not (tmp_path / "destination").exists() + + +@pytest.mark.parametrize("name", ["--prune", "--all", "--upload-pack=helper"]) +def test_remote_update_rejects_option_shaped_name(tmp_path, name): + repo = Repo.init(tmp_path) + with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeOptionError): + Remote(repo, name).update() + run.assert_not_called()