From 9ad28cc4c7176d22cd6181d69805058a719dd03b Mon Sep 17 00:00:00 2001 From: Byron Date: Sat, 26 Sep 2026 20:45:34 +0200 Subject: [PATCH] fix: complete safety checks in Git command wrappers Mostly a rubber-stamp, particularly the tests I just skimmed. It's also a common fix just with unsafe-options guards, and a regex fix which hopefully is truly better than before. I didn't question it in the interest of time. Address GHSA-w8jc-g24h-crhw by applying the existing guard policy to `Git.ls_remote()`, `Repo.merge_base()`, and `IndexFile.move()`. These wrappers previously omitted protocol or option checks already used by sibling APIs. Check flattened positional arguments and split short-option values before `ls_remote()` starts Git, including values that become the repository after option parsing. Add the independent `allow_unsafe_protocols` opt-in and recognize helper selectors even when their address is empty or begins with a newline. Match Git's scheme-character rules at the start of the address so ordinary IPv6 URLs and double colons in repository paths retain their meaning. This addresses the review finding that the broad matcher rejected valid IPv6 remotes. The protocol check is conservative for positional and split option values; long-form `server_option` values remain available without a protocol opt-in. Reuse the revision and pathspec option guards in `merge_base()` and `move()`, with explicit `allow_unsafe_options` opt-ins. Git currently rejects these denylisted options for those subcommands; the checks keep their policy aligned with sibling APIs. Preserve literal move operands behind `--` and validate options before either the dry run or actual move. Only treat exit status 1 from `merge_base()` as no common ancestor. Other failures, including invalid options, now propagate as `GitCommandError` instead of silently returning an empty list. Git reference: `git/git@d38352cd43ab9745686d697872408bc3249a153f`, inspected in `builtin/ls-remote.c`, `transport.c`, `builtin/merge-base.c`, `builtin/mv.c`, `url.c`, and `parse-options.c`. These confirm option parsing before the remote operand, helper selection independently of address contents, long-option abbreviations, and status 1 for unrelated histories. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- doc/source/changes.rst | 7 ++ git/cmd.py | 15 +++- git/index/base.py | 10 +++ git/repo/base.py | 16 +++- test/test_command_guards.py | 151 ++++++++++++++++++++++++++++++++++++ 5 files changed, 195 insertions(+), 4 deletions(-) create mode 100644 test/test_command_guards.py diff --git a/doc/source/changes.rst b/doc/source/changes.rst index ea0543f5b..5b809bd9e 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,13 @@ Changelog ========= +3.2.1 +===== + +Security fixes for + +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-w8jc-g24h-crhw + 3.2.0 ===== diff --git a/git/cmd.py b/git/cmd.py index 773231edd..02e3b157a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -645,7 +645,8 @@ class Git(metaclass=_GitMeta): "_version_info_token", ) - re_unsafe_protocol = re.compile(r"(.+)::.+") + # Match Git's leading transport selector, including an empty helper name. + re_unsafe_protocol = re.compile(r"([A-Za-z0-9][A-Za-z0-9+.-]*|)::") unsafe_git_ls_remote_options = [ # This option allows arbitrary command execution in git-ls-remote. @@ -1129,16 +1130,28 @@ def ls_remote( self, *args: Any, allow_unsafe_options: bool = False, + allow_unsafe_protocols: bool = False, **kwargs: Any, ) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], "Git.AutoInterrupt"]: """List references in a remote repository. :param allow_unsafe_options: Allow unsafe options, like ``--upload-pack`` or ``--exec``. + + :param allow_unsafe_protocols: + Allow unsafe protocols to be used, like ``ext``. Positional arguments + and split short-option values are checked. """ if not allow_unsafe_options: candidate_options = self._option_candidates(args, kwargs) Git.check_unsafe_options(options=candidate_options, unsafe_options=self.unsafe_git_ls_remote_options) + if not allow_unsafe_protocols: + protocol_args = list(args) + if kwargs.get("split_single_char_options", True): + # Split short-option values can become the URL after parsing earlier options. + protocol_args.extend(value for key, value in kwargs.items() if len(key) == 1) + for arg in self._unpack_args(protocol_args): + self.check_unsafe_protocols(arg) return self._call_process("ls_remote", *args, **kwargs) @property diff --git a/git/index/base.py b/git/index/base.py index d14e17e2a..334a4aae6 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1093,6 +1093,7 @@ def move( self, items: Union[PathLike, Sequence[Union[PathLike, Blob, BaseIndexEntry, "Submodule"]]], skip_errors: bool = False, + allow_unsafe_options: bool = False, **kwargs: Any, ) -> List[Tuple[str, str]]: """Rename/move the items, whereas the last item is considered the destination of @@ -1113,6 +1114,10 @@ def move( If ``True``, errors such as ones resulting from missing source files will be skipped. + :param allow_unsafe_options: + Allow unsafe options such as ``--pathspec-from-file`` to be passed to + :manpage:`git-mv(1)`. + :param kwargs: Additional arguments you would like to pass to :manpage:`git-mv(1)`, such as ``dry_run`` or ``force``. @@ -1129,6 +1134,11 @@ def move( :raise git.exc.GitCommandError: If git could not handle your request. """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=Git.unsafe_git_pathspec_from_file_options, + ) args = [] if skip_errors: args.append("-k") diff --git a/git/repo/base.py b/git/repo/base.py index cf496201b..c542e8783 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -892,7 +892,7 @@ def iter_commits( **kwargs, ) - def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]: + def merge_base(self, *rev: TBD, allow_unsafe_options: bool = False, **kwargs: Any) -> List[Commit]: R"""Find the closest common ancestor for the given revision (:class:`~git.objects.commit.Commit`\s, :class:`~git.refs.tag.Tag`\s, :class:`~git.refs.reference.Reference`\s, etc.). @@ -900,6 +900,9 @@ def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]: :param rev: At least two revs to find the common ancestor for. + :param allow_unsafe_options: + Allow unsafe options in the revision arguments, like ``--output``. + :param kwargs: Additional arguments to be passed to the ``repo.git.merge_base()`` command which does all the work. @@ -912,18 +915,25 @@ def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]: :raise ValueError: If fewer than two revisions are provided. + + :raise git.exc.GitCommandError: + If git fails for a reason other than having no common merge base. """ if len(rev) < 2: raise ValueError("Please specify at least two revs, got only %i" % len(rev)) # END handle input + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates(rev, kwargs), unsafe_options=self.unsafe_git_revision_options + ) + res: List[Commit] = [] try: lines: List[str] = self.git.merge_base(*rev, **kwargs).splitlines() except GitCommandError as err: - if err.status == 128: + if err.status != 1: raise - # END handle invalid rev # Status code 1 is returned if there is no merge-base. # (See: https://github.com/git/git/blob/v2.44.0/builtin/merge-base.c#L19) return res diff --git a/test/test_command_guards.py b/test/test_command_guards.py new file mode 100644 index 000000000..1782231cb --- /dev/null +++ b/test/test_command_guards.py @@ -0,0 +1,151 @@ +"""Command wrappers apply safety checks before starting Git.""" + +from pathlib import Path +from unittest import mock + +import pytest + +from git import Actor, Git, GitCommandError, Repo +from git.exc import UnsafeOptionError, UnsafeProtocolError + + +@pytest.mark.parametrize("allow_unsafe_options", [False, True]) +@pytest.mark.parametrize( + "args, kwargs", + [ + (("ext::helper",), {}), + (("ext::",), {}), + (("custom::address",), {}), + (("1custom+v2.test-name::address",), {}), + (("::address",), {}), + (("custom::\naddress",), {}), + ((["--refs", ("ext::helper",)],), {}), + ((None, "--", "ext::helper", "HEAD"), {}), + ((Path("ext::helper"),), {}), + ((), {"q": "ext::helper"}), + ((), {"h": ["ext::helper"]}), + ((), {"-": "ext::helper"}), + ((), {"o": [True, "ext::helper"]}), + (("--server-option",), {"o": "ext::helper", "insert_kwargs_after": "--server-option"}), + ], +) +def test_ls_remote_rejects_unsafe_protocols(args, kwargs, allow_unsafe_options): + with mock.patch.object(Git, "execute", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeProtocolError): + Git().ls_remote(*args, allow_unsafe_options=allow_unsafe_options, **kwargs) + run.assert_not_called() + + +@pytest.mark.parametrize( + "args, kwargs", + [ + ((), {}), + ((None,), {}), + (("origin", "HEAD"), {"h": True}), + (("https://example.com/repo.git",), {}), + (("git@example.com:repo.git",), {}), + (("origin",), {"o": "key=value"}), + (("origin",), {"server_option": "key::value"}), + ], +) +def test_ls_remote_preserves_safe_arguments(args, kwargs): + with mock.patch.object(Git, "execute", return_value="refs") as run: + assert Git().ls_remote(*args, **kwargs) == "refs" + run.assert_called_once() + + +@pytest.mark.parametrize( + "url", + [ + "https://[::1]/repo.git", + "ssh://git@[2001:db8::1]/repo.git", + "https://example.com/repo::name", + "git@example.com:repo::name", + "./repo::name", + ], +) +def test_ls_remote_preserves_double_colons_outside_helper_selector(url): + assert Git().ls_remote(url, get_url=True) == url + + +def test_ls_remote_unsafe_opt_ins_are_independent(): + with mock.patch.object(Git, "execute", return_value="refs") as run: + with pytest.raises(UnsafeOptionError): + Git().ls_remote("origin", upload_pack="helper", allow_unsafe_protocols=True) + run.assert_not_called() + assert ( + Git().ls_remote("ext::helper", upload_pack="helper", allow_unsafe_protocols=True, allow_unsafe_options=True) + == "refs" + ) + run.assert_called_once_with([Git.GIT_PYTHON_GIT_EXECUTABLE, "ls-remote", "--upload-pack=helper", "ext::helper"]) + + +@pytest.mark.parametrize("allow_unsafe_options", [False, True]) +@pytest.mark.parametrize( + "revs, kwargs", + [ + (("HEAD", "--output=unused"), {}), + (("HEAD", ["--out=unused"]), {}), + (("HEAD", "-ounused"), {}), + (("HEAD", "HEAD"), {"output": "unused"}), + (("HEAD", "HEAD"), {"out": "unused"}), + (("HEAD", "HEAD"), {"o": "unused"}), + ], +) +def test_merge_base_checks_unsafe_options(tmp_path, revs, kwargs, allow_unsafe_options): + repo = Repo.init(tmp_path) + with mock.patch.object(Git, "execute", return_value="") as run: + if allow_unsafe_options: + assert repo.merge_base(*revs, allow_unsafe_options=True, **kwargs) == [] + run.assert_called_once() + assert "--allow-unsafe-options" not in run.call_args[0][0] + else: + with pytest.raises(UnsafeOptionError): + repo.merge_base(*revs, **kwargs) + run.assert_not_called() + + +@pytest.mark.parametrize("status", [-9, 2, 128, 129]) +def test_merge_base_propagates_errors(tmp_path, status): + repo = Repo.init(tmp_path) + error = GitCommandError("git merge-base", status) + with mock.patch.object(Git, "execute", side_effect=error): + with pytest.raises(GitCommandError) as raised: + repo.merge_base("HEAD", "HEAD") + assert raised.value is error + + +def test_merge_base_distinguishes_unrelated_history_from_invalid_options(tmp_path): + repo = Repo.init(tmp_path) + actor = Actor("Test", "test@example.com") + first = repo.index.commit("first", author=actor, committer=actor) + second = repo.index.commit("second", parent_commits=[], head=False, author=actor, committer=actor) + assert repo.merge_base(first, first) == [first] + assert repo.merge_base(first, second) == [] + with pytest.raises(GitCommandError) as raised: + repo.merge_base(first, second, invalid_option=True) + assert raised.value.status == 129 + + +@pytest.mark.parametrize("allow_unsafe_options", [False, True]) +@pytest.mark.parametrize("option", ["pathspec_from_file", "pathspec-from-file", "pathspec_from"]) +@pytest.mark.parametrize("dry_run", [False, True]) +def test_move_checks_unsafe_options(tmp_path, option, dry_run, allow_unsafe_options): + repo = Repo.init(tmp_path) + with mock.patch.object(Git, "execute", return_value="Renaming source to destination\n") as run: + kwargs = {option: "unused", "dry_run": dry_run} + if allow_unsafe_options: + assert repo.index.move(["source", "destination"], True, allow_unsafe_options=True, **kwargs) == [ + ("source", "destination") + ] + assert run.call_count == (1 if dry_run else 2) + for call in run.call_args_list: + argv = call[0][0] + assert "-k" in argv + assert f"--{option.replace('_', '-')}=unused" in argv + assert "--allow-unsafe-options" not in argv + assert argv[-3:] == ["--", "source", "destination"] + else: + with pytest.raises(UnsafeOptionError): + repo.index.move(["source", "destination"], **kwargs) + run.assert_not_called()