diff --git a/git/cmd.py b/git/cmd.py index 92ca09c2a..6462aba32 100644 --- 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 `` 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 `` 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 index 653d50aa3..81c08a8df 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): @@ -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 index 24b60af9d..bfee71297 100644 --- 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 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()