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()