Merge pull request #2251 from gitpython-developers/arg-bypass

fix: complete safety checks in Git command wrappers

Author: Byron

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
 =====
 

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

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

git/repo/base.py

@@ -892,14 +892,17 @@ 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.).
 
         :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

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