fix: preserve positional operands in high-level Git commands

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.

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:
 

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:

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
 

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.

git/refs/tag.py

@@ -155,15 +155,15 @@ 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))
 
     @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.

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,
@@ -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)
@@ -1101,7 +1104,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 +1131,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)

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

test/test_positional_args.py

@@ -0,0 +1,129 @@
+"""High-level operands must not become Git options."""
+
+from unittest import mock
+
+import pytest
+
+from git import Actor, Git, GitCommandError, Head, Remote, RemoteReference, Repo, TagReference
+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"
+
+
+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)
+
+
+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()
+
+
+@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()