Merge pull request #2168 from gitpython-developers/advisory-fix-1

Byron · web-flow · commit 65a72839c927 · 2026-07-12T14:57:08.000+02:00
Reject abbreviated forms of unsafe git options
diff --git a/git/cmd.py b/git/cmd.py
@@ -961,17 +961,66 @@ def _canonicalize_option_name(cls, option: str) -> str:
 
     @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.
+        """Raise :class:`~git.exc.UnsafeOptionError` for blocked option spellings.
+
+        In addition to exact matches, this rejects abbreviated long options accepted
+        by Git (for example, ``--upl`` for ``--upload-pack``) and unsafe short options
+        whose values are joined to the same token, including after clusterable flags
+        (for example, ``-uVALUE`` and ``-fuVALUE``).
+
+        A list containing only bare names is treated as normalized keyword arguments,
+        so multi-character names such as ``upload_p`` are checked as long-option
+        abbreviations. If any item starts with ``-``, the list is treated as tokenized
+        command-line input: bare items can be option values and are not checked as
+        abbreviations. Thus ``["--origin", "upload"]`` is allowed. Single-dash options
+        use short-option parsing rather than broad prefix matching, preserving safe
+        attached values such as ``-oupstream`` and ``-bcurrent``.
+
+        Some options passed to ``git <command>`` can execute arbitrary commands and
+        are therefore blocked by default unless the caller explicitly allows them.
         """
         # Options can be of the form `foo`, `--foo`, `--foo bar`, or `--foo=bar`.
+        # Git accepts any unambiguous prefix of a long option, so an abbreviated
+        # spelling such as `--upl` for `--upload-pack` must be rejected too. An
+        # option is unsafe if its canonical name is a prefix of any blocked
+        # option's canonical name. Only long options and multi-character kwargs
+        # can be abbreviations; single-character short options remain exact-match
+        # only.
         canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options}
+        unsafe_short_options = {
+            canonical: option
+            for canonical, option in canonical_unsafe_options.items()
+            if option.startswith("-") and not option.startswith("--") and len(canonical) == 1
+        }
+        # These value-less Git flags can be clustered before another short option
+        # (for example, ``-fuVALUE``). Stop at any other character because it may
+        # begin an attached value, as ``o`` does in the safe option ``-oupstream``.
+        clusterable_short_options = frozenset("46flnqsv")
+        options_are_kwargs = all(not option.startswith("-") for option in options)
         for option in options:
-            unsafe_option = canonical_unsafe_options.get(cls._canonicalize_option_name(option))
+            candidate = cls._canonicalize_option_name(option)
+            if not candidate:
+                continue
+            unsafe_option = canonical_unsafe_options.get(candidate)
             if unsafe_option is not None:
                 raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.")
+            option_token = option.split("=", 1)[0].split(None, 1)[0]
+            if option_token.startswith("-") and not option_token.startswith("--"):
+                for option_char in option_token[1:]:
+                    unsafe_option = unsafe_short_options.get(option_char)
+                    if unsafe_option is not None:
+                        raise UnsafeOptionError(
+                            f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
+                        )
+                    if option_char not in clusterable_short_options:
+                        break
+            if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)):
+                continue
+            for canonical, unsafe_option in canonical_unsafe_options.items():
+                if canonical.startswith(candidate):
+                    raise UnsafeOptionError(
+                        f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it."
+                    )
 
     AutoInterrupt: TypeAlias = _AutoInterrupt
 
diff --git a/test/test_clone.py b/test/test_clone.py
@@ -118,8 +118,13 @@ def test_clone_unsafe_options(self, rw_repo):
             unsafe_options = [
                 f"--upload-pack='touch {tmp_file}'",
                 f"-u 'touch {tmp_file}'",
+                f"-utouch {tmp_file}; false",
+                f"-futouch${{IFS}}{tmp_file}; false",
+                f"-qutouch${{IFS}}{tmp_file}; false",
                 "--config=protocol.ext.allow=always",
                 "-c protocol.ext.allow=always",
+                "-cprotocol.ext.allow=always",
+                "-vcprotocol.ext.allow=always",
             ]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
@@ -138,6 +143,31 @@ def test_clone_unsafe_options(self, rw_repo):
                     rw_repo.clone(tmp_dir, **unsafe_option)
                 assert not tmp_file.exists()
 
+    @with_rw_repo("HEAD")
+    def test_clone_unsafe_options_abbreviated(self, rw_repo):
+        with tempfile.TemporaryDirectory() as tdir:
+            tmp_dir = pathlib.Path(tdir)
+            tmp_file = tmp_dir / "pwn"
+            unsafe_options = [
+                f"--upl='touch {tmp_file}'",
+                f"--upload-pac='touch {tmp_file}'",
+                "--conf=protocol.ext.allow=always",
+            ]
+            for unsafe_option in unsafe_options:
+                with self.assertRaises(UnsafeOptionError):
+                    rw_repo.clone(tmp_dir, multi_options=[unsafe_option])
+                assert not tmp_file.exists()
+
+            unsafe_kwargs = [
+                {"upl": f"touch {tmp_file}"},
+                {"upload_pac": f"touch {tmp_file}"},
+                {"conf": "protocol.ext.allow=always"},
+            ]
+            for unsafe_option in unsafe_kwargs:
+                with self.assertRaises(UnsafeOptionError):
+                    rw_repo.clone(tmp_dir, **unsafe_option)
+                assert not tmp_file.exists()
+
     @with_rw_repo("HEAD")
     def test_clone_unsafe_options_are_checked_after_splitting_multi_options(self, rw_repo):
         with tempfile.TemporaryDirectory() as tdir:
@@ -191,7 +221,9 @@ def test_clone_safe_options(self, rw_repo):
             options = [
                 "--depth=1",
                 "--single-branch",
+                "--origin upload",
                 "-q",
+                "-oupstream",
             ]
             for option in options:
                 destination = tmp_dir / option
@@ -207,8 +239,13 @@ def test_clone_from_unsafe_options(self, rw_repo):
             unsafe_options = [
                 f"--upload-pack='touch {tmp_file}'",
                 f"-u 'touch {tmp_file}'",
+                f"-utouch {tmp_file}; false",
+                f"-futouch${{IFS}}{tmp_file}; false",
+                f"-qutouch${{IFS}}{tmp_file}; false",
                 "--config=protocol.ext.allow=always",
                 "-c protocol.ext.allow=always",
+                "-cprotocol.ext.allow=always",
+                "-vcprotocol.ext.allow=always",
             ]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
diff --git a/test/test_git.py b/test/test_git.py
@@ -170,6 +170,34 @@ def test_check_unsafe_options_normalizes_kwargs(self):
             with self.assertRaises(UnsafeOptionError):
                 Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
 
+    def test_check_unsafe_options_does_not_treat_short_options_as_abbreviations(self):
+        unsafe_options = ["--upload-pack"]
+
+        Git.check_unsafe_options(options=["-u", "u", "-upl"], unsafe_options=unsafe_options)
+        with self.assertRaises(UnsafeOptionError):
+            Git.check_unsafe_options(options=["--u"], unsafe_options=unsafe_options)
+
+    def test_check_unsafe_options_does_not_treat_option_values_as_abbreviations(self):
+        Git.check_unsafe_options(options=["--branch", "conf"], unsafe_options=["--config"])
+
+    def test_check_unsafe_options_rejects_joined_unsafe_short_options(self):
+        cases = [
+            (["-utouch /tmp/pwn"], ["-u"]),
+            (["-futouch /tmp/pwn"], ["-u"]),
+            (["-qutouch${IFS}/tmp/pwn"], ["-u"]),
+            (["-cprotocol.ext.allow=always"], ["-c"]),
+            (["-vcprotocol.ext.allow=always"], ["-c"]),
+        ]
+
+        for options, unsafe_options in cases:
+            with self.assertRaises(UnsafeOptionError):
+                Git.check_unsafe_options(options=options, unsafe_options=unsafe_options)
+
+    def test_check_unsafe_options_allows_attached_safe_short_option_values(self):
+        unsafe_options = ["--upload-pack", "-u", "--config", "-c"]
+
+        Git.check_unsafe_options(options=["-oupstream", "-bcurrent"], unsafe_options=unsafe_options)
+
     _shell_cases = (
         # value_in_call, value_from_class, expected_popen_arg
         (None, False, False),
diff --git a/test/test_remote.py b/test/test_remote.py
@@ -832,7 +832,11 @@ 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}"}, {"upload_pack": f"touch {tmp_file}"}]
+            unsafe_options = [
+                {"upload-pack": f"touch {tmp_file}"},
+                {"upload_pack": f"touch {tmp_file}"},
+                {"upload_p": f"touch {tmp_file}"},
+            ]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
                     remote.fetch(**unsafe_option)
@@ -900,7 +904,11 @@ 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}"}, {"upload_pack": f"touch {tmp_file}"}]
+            unsafe_options = [
+                {"upload-pack": f"touch {tmp_file}"},
+                {"upload_pack": f"touch {tmp_file}"},
+                {"upload_p": f"touch {tmp_file}"},
+            ]
             for unsafe_option in unsafe_options:
                 with self.assertRaises(UnsafeOptionError):
                     remote.pull(**unsafe_option)
@@ -971,7 +979,9 @@ def test_push_unsafe_options(self, rw_repo):
             unsafe_options = [
                 {"receive-pack": f"touch {tmp_file}"},
                 {"receive_pack": f"touch {tmp_file}"},
+                {"receive_p": f"touch {tmp_file}"},
                 {"exec": f"touch {tmp_file}"},
+                {"exe": f"touch {tmp_file}"},
             ]
             for unsafe_option in unsafe_options:
                 assert not tmp_file.exists()
