Refactor unsafe option candidate checks

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

Author: codex

git/cmd.py

@@ -978,6 +978,16 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) ->
             if unsafe_option is not None:
                 raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.")
 
+    @classmethod
+    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
+
     AutoInterrupt: TypeAlias = _AutoInterrupt
 
     CatFileContentStream: TypeAlias = _CatFileContentStream
@@ -1047,8 +1057,8 @@ def ls_remote(
             Allow unsafe options, like ``--upload-pack``.
         """
         if not allow_unsafe_options:
-            unsafe_options = list(kwargs.keys()) + self._unpack_args([a for a in args if a is not None])
-            Git.check_unsafe_options(options=unsafe_options, unsafe_options=self.unsafe_git_ls_remote_options)
+            candidate_options = self._option_candidates(args, kwargs)
+            Git.check_unsafe_options(options=candidate_options, unsafe_options=self.unsafe_git_ls_remote_options)
         return self._call_process("ls_remote", *args, **kwargs)
 
     @property
@@ -1557,7 +1567,7 @@ def transform_kwargs(self, split_single_char_options: bool = True, **kwargs: Any
         return args
 
     @classmethod
-    def _unpack_args(cls, arg_list: Sequence[str]) -> List[str]:
+    def _unpack_args(cls, arg_list: Sequence[Any]) -> List[str]:
         outlist = []
         if isinstance(arg_list, (list, tuple)):
             for arg in arg_list:

git/objects/commit.py

@@ -340,7 +340,7 @@ def iter_items(
 
         if not allow_unsafe_options:
             Git.check_unsafe_options(
-                options=[str(rev)] + list(kwargs.keys()), unsafe_options=cls.unsafe_git_rev_options
+                options=Git._option_candidates([rev], kwargs), unsafe_options=cls.unsafe_git_rev_options
             )
 
         # Use -- in all cases, to prevent possibility of ambiguous arguments.

git/remote.py

@@ -1071,7 +1071,10 @@ def fetch(
                     Git.check_unsafe_protocols(ref)
 
         if not allow_unsafe_options:
-            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_fetch_options)
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=self.unsafe_git_fetch_options,
+            )
 
         proc = self.repo.git.fetch(
             "--", self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs
@@ -1125,7 +1128,10 @@ def pull(
                 Git.check_unsafe_protocols(ref)
 
         if not allow_unsafe_options:
-            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_pull_options)
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=self.unsafe_git_pull_options,
+            )
 
         proc = self.repo.git.pull(
             "--", self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs
@@ -1198,7 +1204,10 @@ def push(
                 Git.check_unsafe_protocols(ref)
 
         if not allow_unsafe_options:
-            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_push_options)
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=self.unsafe_git_push_options,
+            )
 
         proc = self.repo.git.push(
             "--",

git/repo/base.py

@@ -162,20 +162,18 @@ class Repo:
     """
 
     unsafe_git_archive_options = [
-        # This option allows arbitrary command execution in git-archive.
-        "--upload-pack",
+        # Allows arbitrary command execution through the remote git-upload-archive command.
         "--exec",
+        # Writes output to a caller-controlled filesystem path.
         "--output",
         "-o",
     ]
-    """Options to :manpage:`git-archive(1)` that can be dangerous."""
 
     unsafe_git_revision_options = [
         # This option allows output to be written to arbitrary files before revision parsing.
         "--output",
         "-o",
     ]
-    """Options to :manpage:`git-rev-list(1)` / :manpage:`git-blame(1)` that can overwrite files."""
 
     # Invariants
     config_level: ConfigLevels_Tup = ("system", "user", "global", "repository")
@@ -824,7 +822,7 @@ def iter_commits(
 
         if not allow_unsafe_options:
             Git.check_unsafe_options(
-                options=[str(rev)] + list(kwargs.keys()), unsafe_options=self.unsafe_git_revision_options
+                options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options
             )
 
         return Commit.iter_items(
@@ -1136,7 +1134,7 @@ def blame_incremental(
         """
         if not allow_unsafe_options:
             Git.check_unsafe_options(
-                options=[str(rev)] + list(kwargs.keys()), unsafe_options=self.unsafe_git_revision_options
+                options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options
             )
 
         data: bytes = self.git.blame(rev, "--", file, p=True, incremental=True, stdout_as_string=False, **kwargs)
@@ -1216,7 +1214,7 @@ def blame(
         rev: Union[str, HEAD, None],
         file: str,
         incremental: bool = False,
-        rev_opts: Optional[List[str]] = None,
+        rev_opts: Optional[Sequence[str]] = None,
         allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> List[List[Commit | List[str | bytes] | None]] | Iterator[BlameEntry] | None:
@@ -1240,13 +1238,13 @@ def blame(
         """
         if incremental:
             return self.blame_incremental(rev, file, allow_unsafe_options=allow_unsafe_options, **kwargs)
+        rev_opts_list = list(rev_opts or [])
         if not allow_unsafe_options:
-            rev_opts = rev_opts or []
             Git.check_unsafe_options(
-                options=[str(rev)] + rev_opts + list(kwargs.keys()), unsafe_options=self.unsafe_git_revision_options
+                options=Git._option_candidates([rev, rev_opts_list], kwargs),
+                unsafe_options=self.unsafe_git_revision_options,
             )
-        rev_opts = rev_opts or []
-        data: bytes = self.git.blame(rev, *rev_opts, "--", file, p=True, stdout_as_string=False, **kwargs)
+        data: bytes = self.git.blame(rev, *rev_opts_list, "--", file, p=True, stdout_as_string=False, **kwargs)
         commits: Dict[str, Commit] = {}
         blames: List[List[Commit | List[str | bytes] | None]] = []
 
@@ -1457,7 +1455,10 @@ def _clone(
         if not allow_unsafe_protocols:
             Git.check_unsafe_protocols(url)
         if not allow_unsafe_options:
-            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=cls.unsafe_git_clone_options)
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=cls.unsafe_git_clone_options,
+            )
         if not allow_unsafe_options and multi:
             Git.check_unsafe_options(options=multi, unsafe_options=cls.unsafe_git_clone_options)
 
@@ -1633,6 +1634,7 @@ def archive(
         treeish: Optional[str] = None,
         prefix: Optional[str] = None,
         allow_unsafe_options: bool = False,
+        allow_unsafe_protocols: bool = False,
         **kwargs: Any,
     ) -> Repo:
         """Archive the tree at the given revision.
@@ -1656,7 +1658,10 @@ def archive(
               or a list or tuple of multiple paths.
 
         :param allow_unsafe_options:
-            Allow unsafe options in ``kwargs``, like ``--exec`` or ``--upload-pack``.
+            Allow unsafe options, like ``--exec`` or ``--output``.
+
+        :param allow_unsafe_protocols:
+            Allow unsafe protocols to be used in ``remote``, like ``ext``.
 
         :raise git.exc.GitCommandError:
             If something went wrong.
@@ -1668,8 +1673,14 @@ def archive(
             treeish = self.head.commit
         if prefix and "prefix" not in kwargs:
             kwargs["prefix"] = prefix
+        remote = kwargs.get("remote")
+        if not allow_unsafe_protocols and remote:
+            Git.check_unsafe_protocols(str(remote))
         if not allow_unsafe_options:
-            Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_archive_options)
+            Git.check_unsafe_options(
+                options=Git._option_candidates([], kwargs),
+                unsafe_options=self.unsafe_git_archive_options,
+            )
         kwargs["output_stream"] = ostream
         path = kwargs.pop("path", [])
         path = cast(Union[PathLike, List[PathLike], Tuple[PathLike, ...]], path)

test/test_remote.py

@@ -9,7 +9,7 @@
 import random
 import sys
 import tempfile
-from unittest import skipIf
+from unittest import mock, skipIf
 
 import pytest
 
@@ -1023,6 +1023,11 @@ def test_ls_remote_unsafe_options(self, rw_repo):
             with self.assertRaises(UnsafeOptionError):
                 rw_repo.git.ls_remote("--upload-pack", "touch", ".")
 
+    def test_ls_remote_allows_operand_named_like_unsafe_option(self):
+        with mock.patch.object(Git, "_call_process") as git:
+            Git().ls_remote("upload-pack")
+        git.assert_called_once()
+
     @with_rw_repo("HEAD")
     def test_ls_remote_unsafe_options_allowed(self, rw_repo):
         with tempfile.TemporaryDirectory() as tdir:

test/test_repo.py

@@ -38,6 +38,7 @@
     Tree,
 )
 from git.exc import UnsafeOptionError
+from git.exc import UnsafeProtocolError
 from git.exc import BadObject
 from git.repo.fun import touch
 from git.util import bin_to_hex, cwd, cygpath, join_path_native, rmfile, rmtree
@@ -424,11 +425,35 @@ def test_archive(self):
         os.remove(stream.name)  # Do it this way so we can inspect the file on failure.
 
     def test_archive_rejects_unsafe_options(self):
-        with tempfile.NamedTemporaryFile("w", suffix="-unsafe-option") as output_marker:
+        with tempfile.TemporaryDirectory() as tdir:
+            output_marker = osp.join(tdir, "pwn")
             with self.assertRaises(UnsafeOptionError):
-                self.rorepo.archive(io.BytesIO(), "0.1.6", exec=f"touch {output_marker.name}")
+                self.rorepo.archive(io.BytesIO(), "0.1.6", exec=f"touch {output_marker}")
+            assert not osp.exists(output_marker)
             with self.assertRaises(UnsafeOptionError):
-                self.rorepo.archive(io.BytesIO(), "0.1.6", output=f"touch {output_marker.name}")
+                self.rorepo.archive(io.BytesIO(), "0.1.6", output=output_marker)
+            assert not osp.exists(output_marker)
+
+    def test_archive_rejects_unsafe_remote_protocol(self):
+        with tempfile.TemporaryDirectory() as tdir:
+            output_marker = osp.join(tdir, "pwn")
+            with self.assertRaises(UnsafeProtocolError):
+                self.rorepo.archive(io.BytesIO(), "HEAD", remote=f"ext::sh -c touch% {output_marker}")
+            assert not osp.exists(output_marker)
+
+    def test_archive_preserves_positional_allow_unsafe_options(self):
+        with mock.patch.object(Git, "_call_process") as git:
+            self.rorepo.archive(io.BytesIO(), "HEAD", None, True, exec="git-upload-archive")
+        git.assert_called_once()
+
+    def test_archive_accepts_stringifiable_remote(self):
+        class StringifiableRemote:
+            def __str__(self):
+                return "origin"
+
+        with mock.patch.object(Git, "_call_process") as git:
+            self.rorepo.archive(io.BytesIO(), "HEAD", remote=StringifiableRemote())
+        git.assert_called_once()
 
     def test_iter_commits_rejects_unsafe_revision(self):
         with tempfile.TemporaryDirectory() as tdir:
@@ -486,19 +511,25 @@ def test_blame_real(self):
         assert nml, "There should at least be one blame commit that contains multiple lines"
 
     def test_blame_rejects_unsafe_revision(self):
-        with tempfile.NamedTemporaryFile("w", suffix="-unsafe-option") as output_marker:
+        with tempfile.TemporaryDirectory() as tdir:
+            output_marker = osp.join(tdir, "pwn")
             with self.assertRaises(UnsafeOptionError):
-                self.rorepo.blame(f"--output={output_marker.name}", "README.md")
+                self.rorepo.blame(f"--output={output_marker}", "README.md")
+            assert not osp.exists(output_marker)
 
     def test_blame_rejects_unsafe_options(self):
-        with tempfile.NamedTemporaryFile("w", suffix="-unsafe-option") as output_marker:
+        with tempfile.TemporaryDirectory() as tdir:
+            output_marker = osp.join(tdir, "pwn")
             with self.assertRaises(UnsafeOptionError):
-                self.rorepo.blame("HEAD", "README.md", output=f"touch {output_marker.name}")
+                self.rorepo.blame("HEAD", "README.md", output=output_marker)
+            assert not osp.exists(output_marker)
 
     def test_blame_rejects_unsafe_rev_opts(self):
-        with tempfile.NamedTemporaryFile("w", suffix="-unsafe-option") as output_marker:
+        with tempfile.TemporaryDirectory() as tdir:
+            output_marker = osp.join(tdir, "pwn")
             with self.assertRaises(UnsafeOptionError):
-                self.rorepo.blame("HEAD", "README.md", rev_opts=[f"--output={output_marker.name}"])
+                self.rorepo.blame("HEAD", "README.md", rev_opts=(f"--output={output_marker}",))
+            assert not osp.exists(output_marker)
 
     @mock.patch.object(Git, "_call_process")
     def test_blame_incremental(self, git):