From 0d88094a72204a00802015e5611dc261aaf638ed Mon Sep 17 00:00:00 2001 From: Codex GPT-5 Date: Wed, 17 Jun 2026 12:26:44 +0800 Subject: [PATCH] fix: block unsafe long-option prefixes (GHSA-2f96-g7mh-g2hx) [ruff-format] --- git/cmd.py | 81 ++++++++++++++++++++++++++++++++++++++++++--- git/repo/base.py | 4 ++- test/test_clone.py | 14 ++++++++ test/test_git.py | 40 ++++++++++++++++++++++ test/test_remote.py | 14 ++++++-- 5 files changed, 145 insertions(+), 8 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 92ca09c2a..97427badc 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -960,18 +960,89 @@ def _canonicalize_option_name(cls, option: str) -> str: return dashify(option_tokens[0]) @classmethod - def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None: + def _option_token(cls, option: str) -> str: + option_name = option.split("=", 1)[0] + option_tokens = option_name.split(None, 1) + if not option_tokens: + return "" + return option_tokens[0] + + @classmethod + def _is_long_option(cls, option: str, bare_options_are_long: bool = True) -> bool: + """Return whether an option is a long option before canonicalization.""" + option_token = cls._option_token(option) + if not option_token: + return False + + if option_token.startswith("--"): + return True + if option_token.startswith("-"): + return False + return bare_options_are_long and len(option_token) > 1 + + @classmethod + def _matches_unsafe_short_option( + cls, option: str, canonical_unsafe_option: str, unsafe_option_is_long: bool + ) -> bool: + """Return whether a single-dash option matches an unsafe short option.""" + if unsafe_option_is_long or len(canonical_unsafe_option) != 1: + return False + + option_token = cls._option_token(option) + if not option_token: + return False + + if not option_token.startswith("-") or option_token.startswith("--"): + return False + + clusterable_flags = frozenset("46flnqsv") + for option_char in option_token[1:]: + if option_char == canonical_unsafe_option: + return True + if option_char not in clusterable_flags: + return False + return False + + @classmethod + def check_unsafe_options( + cls, options: List[str], unsafe_options: List[str], bare_options_are_long: bool = True + ) -> None: """Check for unsafe options. Some options that are passed to ``git `` can be used to execute arbitrary commands. These are blocked by default. """ # 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} + # We also treat long-option prefix forms as unsafe, matching Git's option + # parser behavior for long options. + canonical_unsafe_options = [ + (cls._canonicalize_option_name(option), option, cls._is_long_option(option)) for option in unsafe_options + ] for option in options: - 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.") + option_token = cls._option_token(option) + if not bare_options_are_long and not option_token.startswith("-"): + continue + canonical_option = cls._canonicalize_option_name(option) + if not canonical_option: + continue + option_is_long = cls._is_long_option(option, bare_options_are_long=bare_options_are_long) + for ( + canonical_unsafe_option, + unsafe_option, + unsafe_option_is_long, + ) in canonical_unsafe_options: + if ( + canonical_option == canonical_unsafe_option + or ( + option_is_long + and unsafe_option_is_long + and canonical_unsafe_option.startswith(canonical_option) + ) + or cls._matches_unsafe_short_option(option, canonical_unsafe_option, unsafe_option_is_long) + ): + raise UnsafeOptionError( + f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it." + ) AutoInterrupt: TypeAlias = _AutoInterrupt diff --git a/git/repo/base.py b/git/repo/base.py index 2d3cf24f0..c430db25f 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -1410,7 +1410,9 @@ def _clone( if not allow_unsafe_options: Git.check_unsafe_options(options=list(kwargs.keys()), 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) + Git.check_unsafe_options( + options=multi, unsafe_options=cls.unsafe_git_clone_options, bare_options_are_long=False + ) proc = git.clone( multi, diff --git a/test/test_clone.py b/test/test_clone.py index 653d50aa3..4d4b0bd85 100644 --- 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): @@ -129,6 +134,7 @@ def test_clone_unsafe_options(self, rw_repo): unsafe_options = [ {"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}, + {"upload_p": f"touch {tmp_file}"}, {"u": f"touch {tmp_file}"}, {"config": "protocol.ext.allow=always"}, {"c": "protocol.ext.allow=always"}, @@ -191,7 +197,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 +215,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): @@ -218,6 +231,7 @@ def test_clone_from_unsafe_options(self, rw_repo): unsafe_options = [ {"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}, + {"upload_p": f"touch {tmp_file}"}, {"u": f"touch {tmp_file}"}, {"config": "protocol.ext.allow=always"}, {"c": "protocol.ext.allow=always"}, diff --git a/test/test_git.py b/test/test_git.py index 24b60af9d..c8443d3ac 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -158,10 +158,21 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): def test_check_unsafe_options_normalizes_kwargs(self): cases = [ (["upload_pack"], ["--upload-pack"]), + (["upload_p"], ["--upload-pack"]), + (["upload_pac"], ["--upload-pack"]), (["receive_pack"], ["--receive-pack"]), + (["receive_p"], ["--receive-pack"]), (["exec"], ["--exec"]), + (["exe"], ["--exec"]), (["u"], ["-u"]), + (["-utouch /tmp/pwn"], ["-u"]), + (["-futouch /tmp/pwn"], ["-u"]), + (["-qutouch${IFS}/tmp/pwn"], ["-u"]), (["c"], ["-c"]), + (["-cprotocol.ext.allow=always"], ["-c"]), + (["-vcprotocol.ext.allow=always"], ["-c"]), + (["conf"], ["--config"]), + (["confi"], ["--config"]), (["--upload-pack=/tmp/helper"], ["--upload-pack"]), (["--config core.filemode=false"], ["--config"]), ] @@ -170,6 +181,35 @@ 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_keeps_short_options_exact(self): + cases = [ + (["u"], ["--upload-pack"]), + (["-u helper"], ["--upload-pack"]), + (["c"], ["--config"]), + (["-c protocol.ext.allow=always"], ["--config"]), + (["e"], ["--exec"]), + (["r"], ["--receive-pack"]), + ] + + for options, unsafe_options in cases: + Git.check_unsafe_options(options=options, unsafe_options=unsafe_options) + + def test_check_unsafe_options_ignores_multi_option_values(self): + Git.check_unsafe_options( + options=["--origin", "upload"], + unsafe_options=["--upload-pack", "--config"], + bare_options_are_long=False, + ) + + def test_check_unsafe_options_allows_attached_safe_short_option_values(self): + cases = [ + ["-oupstream"], + ["-bcurrent"], + ] + + for options in cases: + Git.check_unsafe_options(options=options, unsafe_options=["--upload-pack", "-u", "--config", "-c"]) + _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 index 2230c8df4..81446aec2 100644 --- 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()