Merge pull request #2180 from gitpython-developers/single-char-kwarg

Harden unsafe Git option validation

Author: Byron

git/cmd.py

@@ -1039,11 +1039,18 @@ def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[s
             option for option in cls._unpack_args([arg for arg in args if arg is not None]) if option.startswith("-")
         ]
         if kwargs:
+            split_single_char_options = kwargs.get("split_single_char_options", True)
             for key, value in kwargs.items():
                 values = value if isinstance(value, (list, tuple)) else (value,)
                 if any(value is True or (value is not False and value is not None) for value in values):
                     key = str(key)
                     options.append(f"-{key}" if len(key) == 1 else f"--{dashify(key)}")
+                    if len(key) == 1 and split_single_char_options:
+                        options.extend(
+                            str(value)
+                            for value in values
+                            if value is not True and value not in (False, None) and str(value).startswith("-")
+                        )
         return options
 
     AutoInterrupt: TypeAlias = _AutoInterrupt

git/diff.py

@@ -9,7 +9,7 @@
 import re
 import warnings
 
-from git.cmd import handle_process_output
+from git.cmd import Git, handle_process_output
 from git.compat import defenc
 from git.objects.blob import Blob
 from git.objects.util import mode_str_to_int
@@ -35,7 +35,6 @@
 if TYPE_CHECKING:
     from subprocess import Popen
 
-    from git.cmd import Git
     from git.objects.base import IndexObject
     from git.objects.commit import Commit
     from git.objects.tree import Tree
@@ -190,6 +189,7 @@ def diff(
         other: Union[DiffConstants, "Tree", "Commit", str, None] = INDEX,
         paths: Union[PathLike, List[PathLike], Tuple[PathLike, ...], None] = None,
         create_patch: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> "DiffIndex[Diff]":
         """Create diffs between two items being trees, trees and index or an index and
@@ -219,6 +219,10 @@ def diff(
             applied makes the self to other. Patches are somewhat costly as blobs have
             to be read and diffed.
 
+        :param allow_unsafe_options:
+            If ``True``, allow options such as ``--output`` that can write to arbitrary
+            filesystem paths.
+
         :param kwargs:
             Additional arguments passed to :manpage:`git-diff(1)`, such as ``R=True`` to
             swap both sides of the diff.
@@ -231,6 +235,12 @@ def diff(
             an instance of :class:`~git.objects.tree.Tree` or
             :class:`~git.objects.commit.Commit`, or a git command error will occur.
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([other], kwargs),
+                unsafe_options=self.repo.unsafe_git_revision_options,
+            )
+
         args: List[Union[PathLike, Diffable]] = []
         args.append("--abbrev=40")  # We need full shas.
         args.append("--full-index")  # Get full index paths, not only filenames.

git/index/base.py

@@ -23,6 +23,7 @@
 from gitdb.db import MemoryDB
 
 from git.compat import defenc, force_bytes
+from git.cmd import Git
 import git.diff as git_diff
 from git.exc import CheckoutError, GitCommandError, GitError, InvalidGitRepositoryError
 from git.objects import Blob, Commit, Object, Submodule, Tree
@@ -1492,6 +1493,7 @@ def diff(
         ] = git_diff.INDEX,
         paths: Union[PathLike, List[PathLike], Tuple[PathLike, ...], None] = None,
         create_patch: bool = False,
+        allow_unsafe_options: bool = False,
         **kwargs: Any,
     ) -> git_diff.DiffIndex[git_diff.Diff]:
         """Diff this index against the working copy or a :class:`~git.objects.tree.Tree`
@@ -1504,6 +1506,12 @@ def diff(
             Will only work with indices that represent the default git index as they
             have not been initialized with a stream.
         """
+        if not allow_unsafe_options:
+            Git.check_unsafe_options(
+                options=Git._option_candidates([other], kwargs),
+                unsafe_options=self.repo.unsafe_git_revision_options,
+            )
+
         # Only run if we are the default repository index.
         if self._file_path != self._index_path():
             raise AssertionError("Cannot call %r on indices that do not represent the default git index" % self.diff())
@@ -1560,12 +1568,18 @@ def diff(
             # Invert the existing R flag.
             cur_val = kwargs.get("R", False)
             kwargs["R"] = not cur_val
-            return other.diff(self.INDEX, paths, create_patch, **kwargs)
+            return other.diff(
+                self.INDEX,
+                paths,
+                create_patch,
+                allow_unsafe_options=allow_unsafe_options,
+                **kwargs,
+            )
         # END diff against other item handling
 
         # If other is not None here, something is wrong.
         if other is not None:
             raise ValueError("other must be None, Diffable.INDEX, a Tree or Commit, was %r" % other)
 
         # Diff against working copy - can be handled by superclass natively.
-        return super().diff(other, paths, create_patch, **kwargs)
+        return super().diff(other, paths, create_patch, allow_unsafe_options=allow_unsafe_options, **kwargs)

git/repo/base.py

@@ -149,6 +149,8 @@ class Repo:
         # Can override configuration variables that execute arbitrary commands:
         "--config",
         "-c",
+        # Can install hooks that execute during clone:
+        "--template",
     ]
     """Options to :manpage:`git-clone(1)` that allow arbitrary commands to be executed.
 
@@ -159,6 +161,9 @@ class Repo:
     The ``--config``/``-c`` option allows users to override configuration variables like
     ``protocol.allow`` and ``core.gitProxy`` to execute arbitrary commands:
     https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt
+
+    The ``--template`` option can install hooks that execute during clone:
+    https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---templatetemplate-directory
     """
 
     unsafe_git_archive_options = [

test/test_clone.py

@@ -128,6 +128,7 @@ def test_clone_unsafe_options(self, rw_repo):
                 "-c protocol.ext.allow=always",
                 "-cprotocol.ext.allow=always",
                 "-vcprotocol.ext.allow=always",
+                f"--template={tmp_dir}",
             ]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
@@ -142,6 +143,7 @@ def test_clone_unsafe_options(self, rw_repo):
                 {"config": "protocol.ext.allow=always"},
                 {"conf": "protocol.ext.allow=always"},
                 {"c": "protocol.ext.allow=always"},
+                {"template": tmp_dir},
             ]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):

test/test_diff.py

@@ -14,6 +14,7 @@
 
 from git import NULL_TREE, Diff, DiffIndex, Diffable, GitCommandError, Repo, Submodule
 from git.cmd import Git
+from git.exc import UnsafeOptionError
 
 from test.lib import StringProcessAdapter, TestBase, fixture, with_rw_directory
 
@@ -352,6 +353,25 @@ def test_diff_submodule(self):
         self.assertIsInstance(diff.a_blob.size, int)
         self.assertIsInstance(diff.b_blob.size, int)
 
+    def test_diff_rejects_unsafe_output_options(self):
+        commit = self.rorepo.head.commit
+
+        calls = (
+            lambda target: commit.diff(output=target),
+            lambda target: commit.diff(other=f"--output={target}"),
+            lambda target: self.rorepo.index.diff(NULL_TREE, output=target),
+            lambda target: self.rorepo.index.diff(f"--output={target}"),
+        )
+        for index, call in enumerate(calls):
+            target = osp.join(self.repo_dir, f"diff-output-{index}")
+            with self.assertRaises(UnsafeOptionError):
+                call(target)
+            self.assertFalse(osp.exists(target))
+
+        allowed_target = osp.join(self.repo_dir, "allowed-diff-output")
+        commit.diff(output=allowed_target, allow_unsafe_options=True)
+        self.assertTrue(osp.isfile(allowed_target))
+
     def test_diff_interface(self):
         """Test a few variations of the main diff routine."""
         assertion_map = {}

test/test_git.py

@@ -214,6 +214,23 @@ def test_option_candidates_ignore_untransformed_kwargs(self):
 
         self.assertEqual(options, ["--max-count"])
 
+    def test_option_candidates_include_split_single_char_option_values(self):
+        cases = [
+            ({"n": "--upload-pack=helper"}, ["-n", "--upload-pack=helper"], ["--upload-pack"]),
+            ({"g": ("safe", "--out=target")}, ["-g", "--out=target"], ["--output"]),
+        ]
+
+        for kwargs, candidates, unsafe_options in cases:
+            self.assertEqual(Git._option_candidates(kwargs=kwargs), candidates)
+            with self.assertRaises(UnsafeOptionError):
+                Git.check_unsafe_options(options=candidates, unsafe_options=unsafe_options)
+
+        self.assertEqual(self.git.transform_kwargs(n="--upload-pack=helper"), ["-n", "--upload-pack=helper"])
+
+        unsplit_kwargs = {"n": "--upload-pack=helper", "split_single_char_options": False}
+        self.assertEqual(self.git.transform_kwargs(**unsplit_kwargs), ["-n--upload-pack=helper"])
+        self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n"])
+
     _shell_cases = (
         # value_in_call, value_from_class, expected_popen_arg
         (None, False, False),