Merge pull request #2131 from WesR/main

Block unsafe underscored git kwargs / Fix for GHSA-rpm5-65cw-6hj4

Author: Byron

git/cmd.py

@@ -944,22 +944,34 @@ def check_unsafe_protocols(cls, url: str) -> None:
                 f"The `{protocol}::` protocol looks suspicious, use `allow_unsafe_protocols=True` to allow it."
             )
 
+    @classmethod
+    def _canonicalize_option_name(cls, option: str) -> str:
+        """Return the option name used for unsafe-option checks.
+
+        Examples:
+            ``"--upload-pack=/tmp/helper"`` -> ``"upload-pack"``
+            ``"upload_pack"`` -> ``"upload-pack"``
+            ``"--config core.filemode=false"`` -> ``"config"``
+        """
+        option_name = option.lstrip("-").split("=", 1)[0]
+        option_tokens = option_name.split(None, 1)
+        if not option_tokens:
+            return ""
+        return dashify(option_tokens[0])
+
     @classmethod
     def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None:
         """Check for unsafe options.
 
         Some options that are passed to ``git <command>`` can be used to execute
         arbitrary commands. These are blocked by default.
         """
-        # Options can be of the form `foo`, `--foo bar`, or `--foo=bar`, so we need to
-        # check if they start with "--foo" or if they are equal to "foo".
-        bare_unsafe_options = [option.lstrip("-") for option in unsafe_options]
+        # Options can be of the form `foo`, `--foo`, `--foo bar`, or `--foo=bar`.
+        canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options}
         for option in options:
-            for unsafe_option, bare_option in zip(unsafe_options, bare_unsafe_options):
-                if option.startswith(unsafe_option) or option == bare_option:
-                    raise UnsafeOptionError(
-                        f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
-                    )
+            unsafe_option = canonical_unsafe_options.get(cls._canonicalize_option_name(option))
+            if unsafe_option is not None:
+                raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.")
 
     AutoInterrupt: TypeAlias = _AutoInterrupt
 

test/test_clone.py

@@ -128,6 +128,7 @@ def test_clone_unsafe_options(self, rw_repo):
 
             unsafe_options = [
                 {"upload-pack": f"touch {tmp_file}"},
+                {"upload_pack": f"touch {tmp_file}"},
                 {"u": f"touch {tmp_file}"},
                 {"config": "protocol.ext.allow=always"},
                 {"c": "protocol.ext.allow=always"},
@@ -216,6 +217,7 @@ def test_clone_from_unsafe_options(self, rw_repo):
 
             unsafe_options = [
                 {"upload-pack": f"touch {tmp_file}"},
+                {"upload_pack": f"touch {tmp_file}"},
                 {"u": f"touch {tmp_file}"},
                 {"config": "protocol.ext.allow=always"},
                 {"c": "protocol.ext.allow=always"},

test/test_git.py

@@ -27,6 +27,7 @@
 import ddt
 
 from git import Git, GitCommandError, GitCommandNotFound, Repo, cmd, refresh
+from git.exc import UnsafeOptionError
 from git.util import cwd, finalize_process
 
 from test.lib import TestBase, fixture_path, with_rw_directory
@@ -154,6 +155,21 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
         res = self.git.transform_kwargs(**{"s": True, "t": True})
         self.assertEqual({"-s", "-t"}, set(res))
 
+    def test_check_unsafe_options_normalizes_kwargs(self):
+        cases = [
+            (["upload_pack"], ["--upload-pack"]),
+            (["receive_pack"], ["--receive-pack"]),
+            (["exec"], ["--exec"]),
+            (["u"], ["-u"]),
+            (["c"], ["-c"]),
+            (["--upload-pack=/tmp/helper"], ["--upload-pack"]),
+            (["--config core.filemode=false"], ["--config"]),
+        ]
+
+        for options, unsafe_options in cases:
+            with self.assertRaises(UnsafeOptionError):
+                Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
+
     _shell_cases = (
         # value_in_call, value_from_class, expected_popen_arg
         (None, False, False),

test/test_remote.py

@@ -827,7 +827,7 @@ def test_fetch_unsafe_options(self, rw_repo):
             remote = rw_repo.remote("origin")
             tmp_dir = Path(tdir)
             tmp_file = tmp_dir / "pwn"
-            unsafe_options = [{"upload-pack": f"touch {tmp_file}"}]
+            unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
                     remote.fetch(**unsafe_option)
@@ -895,7 +895,7 @@ def test_pull_unsafe_options(self, rw_repo):
             remote = rw_repo.remote("origin")
             tmp_dir = Path(tdir)
             tmp_file = tmp_dir / "pwn"
-            unsafe_options = [{"upload-pack": f"touch {tmp_file}"}]
+            unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
                     remote.pull(**unsafe_option)
@@ -964,10 +964,9 @@ def test_push_unsafe_options(self, rw_repo):
             tmp_dir = Path(tdir)
             tmp_file = tmp_dir / "pwn"
             unsafe_options = [
-                {
-                    "receive-pack": f"touch {tmp_file}",
-                    "exec": f"touch {tmp_file}",
-                }
+                {"receive-pack": f"touch {tmp_file}"},
+                {"receive_pack": f"touch {tmp_file}"},
+                {"exec": f"touch {tmp_file}"},
             ]
             for unsafe_option in unsafe_options:
                 assert not tmp_file.exists()
@@ -991,10 +990,9 @@ def test_push_unsafe_options_allowed(self, rw_repo):
             tmp_dir = Path(tdir)
             tmp_file = tmp_dir / "pwn"
             unsafe_options = [
-                {
-                    "receive-pack": f"touch {tmp_file}",
-                    "exec": f"touch {tmp_file}",
-                }
+                {"receive-pack": f"touch {tmp_file}"},
+                {"receive_pack": f"touch {tmp_file}"},
+                {"exec": f"touch {tmp_file}"},
             ]
             for unsafe_option in unsafe_options:
                 # The options will be allowed, but the command will fail.