FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: complete safety checks in Git command wrappers · gitpython-developers/GitPython@9ad28cc · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Commit 9ad28cc

Browse files
Byron
and
codex
committed
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. <!-- agent --> 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 <codex@openai.com>
1 parent a54d159 commit 9ad28cc

5 files changed

Lines changed: 195 additions & 4 deletions

File tree

‎doc/source/changes.rst‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@
22
Changelog
33
=========
44

5+
3.2.1
6+
=====
7+
8+
Security fixes for
9+
10+
* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-w8jc-g24h-crhw
11+
512
3.2.0
613
=====
714

‎git/cmd.py‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -645,7 +645,8 @@ class Git(metaclass=_GitMeta):
645645
"_version_info_token",
646646
)
647647

648-
re_unsafe_protocol = re.compile(r"(.+)::.+")
648+
# Match Git's leading transport selector, including an empty helper name.
649+
re_unsafe_protocol = re.compile(r"([A-Za-z0-9][A-Za-z0-9+.-]*|)::")
649650

650651
unsafe_git_ls_remote_options = [
651652
# This option allows arbitrary command execution in git-ls-remote.
@@ -1129,16 +1130,28 @@ def ls_remote(
11291130
self,
11301131
*args: Any,
11311132
allow_unsafe_options: bool = False,
1133+
allow_unsafe_protocols: bool = False,
11321134
**kwargs: Any,
11331135
) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], "Git.AutoInterrupt"]:
11341136
"""List references in a remote repository.
11351137
11361138
:param allow_unsafe_options:
11371139
Allow unsafe options, like ``--upload-pack`` or ``--exec``.
1140+
1141+
:param allow_unsafe_protocols:
1142+
Allow unsafe protocols to be used, like ``ext``. Positional arguments
1143+
and split short-option values are checked.
11381144
"""
11391145
if not allow_unsafe_options:
11401146
candidate_options = self._option_candidates(args, kwargs)
11411147
Git.check_unsafe_options(options=candidate_options, unsafe_options=self.unsafe_git_ls_remote_options)
1148+
if not allow_unsafe_protocols:
1149+
protocol_args = list(args)
1150+
if kwargs.get("split_single_char_options", True):
1151+
# Split short-option values can become the URL after parsing earlier options.
1152+
protocol_args.extend(value for key, value in kwargs.items() if len(key) == 1)
1153+
for arg in self._unpack_args(protocol_args):
1154+
self.check_unsafe_protocols(arg)
11421155
return self._call_process("ls_remote", *args, **kwargs)
11431156

11441157
@property

‎git/index/base.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,6 +1093,7 @@ def move(
10931093
self,
10941094
items: Union[PathLike, Sequence[Union[PathLike, Blob, BaseIndexEntry, "Submodule"]]],
10951095
skip_errors: bool = False,
1096+
allow_unsafe_options: bool = False,
10961097
**kwargs: Any,
10971098
) -> List[Tuple[str, str]]:
10981099
"""Rename/move the items, whereas the last item is considered the destination of
@@ -1113,6 +1114,10 @@ def move(
11131114
If ``True``, errors such as ones resulting from missing source files will be
11141115
skipped.
11151116
1117+
:param allow_unsafe_options:
1118+
Allow unsafe options such as ``--pathspec-from-file`` to be passed to
1119+
:manpage:`git-mv(1)`.
1120+
11161121
:param kwargs:
11171122
Additional arguments you would like to pass to :manpage:`git-mv(1)`, such as
11181123
``dry_run`` or ``force``.
@@ -1129,6 +1134,11 @@ def move(
11291134
:raise git.exc.GitCommandError:
11301135
If git could not handle your request.
11311136
"""
1137+
if not allow_unsafe_options:
1138+
Git.check_unsafe_options(
1139+
options=Git._option_candidates([], kwargs),
1140+
unsafe_options=Git.unsafe_git_pathspec_from_file_options,
1141+
)
11321142
args = []
11331143
if skip_errors:
11341144
args.append("-k")

‎git/repo/base.py‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -892,14 +892,17 @@ def iter_commits(
892892
**kwargs,
893893
)
894894

895-
def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]:
895+
def merge_base(self, *rev: TBD, allow_unsafe_options: bool = False, **kwargs: Any) -> List[Commit]:
896896
R"""Find the closest common ancestor for the given revision
897897
(:class:`~git.objects.commit.Commit`\s, :class:`~git.refs.tag.Tag`\s,
898898
:class:`~git.refs.reference.Reference`\s, etc.).
899899
900900
:param rev:
901901
At least two revs to find the common ancestor for.
902902
903+
:param allow_unsafe_options:
904+
Allow unsafe options in the revision arguments, like ``--output``.
905+
903906
:param kwargs:
904907
Additional arguments to be passed to the ``repo.git.merge_base()`` command
905908
which does all the work.
@@ -912,18 +915,25 @@ def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]:
912915
913916
:raise ValueError:
914917
If fewer than two revisions are provided.
918+
919+
:raise git.exc.GitCommandError:
920+
If git fails for a reason other than having no common merge base.
915921
"""
916922
if len(rev) < 2:
917923
raise ValueError("Please specify at least two revs, got only %i" % len(rev))
918924
# END handle input
919925

926+
if not allow_unsafe_options:
927+
Git.check_unsafe_options(
928+
options=Git._option_candidates(rev, kwargs), unsafe_options=self.unsafe_git_revision_options
929+
)
930+
920931
res: List[Commit] = []
921932
try:
922933
lines: List[str] = self.git.merge_base(*rev, **kwargs).splitlines()
923934
except GitCommandError as err:
924-
if err.status == 128:
935+
if err.status != 1:
925936
raise
926-
# END handle invalid rev
927937
# Status code 1 is returned if there is no merge-base.
928938
# (See: https://github.com/git/git/blob/v2.44.0/builtin/merge-base.c#L19)
929939
return res

‎test/test_command_guards.py‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
"""Command wrappers apply safety checks before starting Git."""
2+
3+
from pathlib import Path
4+
from unittest import mock
5+
6+
import pytest
7+
8+
from git import Actor, Git, GitCommandError, Repo
9+
from git.exc import UnsafeOptionError, UnsafeProtocolError
10+
11+
12+
@pytest.mark.parametrize("allow_unsafe_options", [False, True])
13+
@pytest.mark.parametrize(
14+
"args, kwargs",
15+
[
16+
(("ext::helper",), {}),
17+
(("ext::",), {}),
18+
(("custom::address",), {}),
19+
(("1custom+v2.test-name::address",), {}),
20+
(("::address",), {}),
21+
(("custom::\naddress",), {}),
22+
((["--refs", ("ext::helper",)],), {}),
23+
((None, "--", "ext::helper", "HEAD"), {}),
24+
((Path("ext::helper"),), {}),
25+
((), {"q": "ext::helper"}),
26+
((), {"h": ["ext::helper"]}),
27+
((), {"-": "ext::helper"}),
28+
((), {"o": [True, "ext::helper"]}),
29+
(("--server-option",), {"o": "ext::helper", "insert_kwargs_after": "--server-option"}),
30+
],
31+
)
32+
def test_ls_remote_rejects_unsafe_protocols(args, kwargs, allow_unsafe_options):
33+
with mock.patch.object(Git, "execute", side_effect=AssertionError("Git must not run")) as run:
34+
with pytest.raises(UnsafeProtocolError):
35+
Git().ls_remote(*args, allow_unsafe_options=allow_unsafe_options, **kwargs)
36+
run.assert_not_called()
37+
38+
39+
@pytest.mark.parametrize(
40+
"args, kwargs",
41+
[
42+
((), {}),
43+
((None,), {}),
44+
(("origin", "HEAD"), {"h": True}),
45+
(("https://example.com/repo.git",), {}),
46+
(("git@example.com:repo.git",), {}),
47+
(("origin",), {"o": "key=value"}),
48+
(("origin",), {"server_option": "key::value"}),
49+
],
50+
)
51+
def test_ls_remote_preserves_safe_arguments(args, kwargs):
52+
with mock.patch.object(Git, "execute", return_value="refs") as run:
53+
assert Git().ls_remote(*args, **kwargs) == "refs"
54+
run.assert_called_once()
55+
56+
57+
@pytest.mark.parametrize(
58+
"url",
59+
[
60+
"https://[::1]/repo.git",
61+
"ssh://git@[2001:db8::1]/repo.git",
62+
"https://example.com/repo::name",
63+
"git@example.com:repo::name",
64+
"./repo::name",
65+
],
66+
)
67+
def test_ls_remote_preserves_double_colons_outside_helper_selector(url):
68+
assert Git().ls_remote(url, get_url=True) == url
69+
70+
71+
def test_ls_remote_unsafe_opt_ins_are_independent():
72+
with mock.patch.object(Git, "execute", return_value="refs") as run:
73+
with pytest.raises(UnsafeOptionError):
74+
Git().ls_remote("origin", upload_pack="helper", allow_unsafe_protocols=True)
75+
run.assert_not_called()
76+
assert (
77+
Git().ls_remote("ext::helper", upload_pack="helper", allow_unsafe_protocols=True, allow_unsafe_options=True)
78+
== "refs"
79+
)
80+
run.assert_called_once_with([Git.GIT_PYTHON_GIT_EXECUTABLE, "ls-remote", "--upload-pack=helper", "ext::helper"])
81+
82+
83+
@pytest.mark.parametrize("allow_unsafe_options", [False, True])
84+
@pytest.mark.parametrize(
85+
"revs, kwargs",
86+
[
87+
(("HEAD", "--output=unused"), {}),
88+
(("HEAD", ["--out=unused"]), {}),
89+
(("HEAD", "-ounused"), {}),
90+
(("HEAD", "HEAD"), {"output": "unused"}),
91+
(("HEAD", "HEAD"), {"out": "unused"}),
92+
(("HEAD", "HEAD"), {"o": "unused"}),
93+
],
94+
)
95+
def test_merge_base_checks_unsafe_options(tmp_path, revs, kwargs, allow_unsafe_options):
96+
repo = Repo.init(tmp_path)
97+
with mock.patch.object(Git, "execute", return_value="") as run:
98+
if allow_unsafe_options:
99+
assert repo.merge_base(*revs, allow_unsafe_options=True, **kwargs) == []
100+
run.assert_called_once()
101+
assert "--allow-unsafe-options" not in run.call_args[0][0]
102+
else:
103+
with pytest.raises(UnsafeOptionError):
104+
repo.merge_base(*revs, **kwargs)
105+
run.assert_not_called()
106+
107+
108+
@pytest.mark.parametrize("status", [-9, 2, 128, 129])
109+
def test_merge_base_propagates_errors(tmp_path, status):
110+
repo = Repo.init(tmp_path)
111+
error = GitCommandError("git merge-base", status)
112+
with mock.patch.object(Git, "execute", side_effect=error):
113+
with pytest.raises(GitCommandError) as raised:
114+
repo.merge_base("HEAD", "HEAD")
115+
assert raised.value is error
116+
117+
118+
def test_merge_base_distinguishes_unrelated_history_from_invalid_options(tmp_path):
119+
repo = Repo.init(tmp_path)
120+
actor = Actor("Test", "test@example.com")
121+
first = repo.index.commit("first", author=actor, committer=actor)
122+
second = repo.index.commit("second", parent_commits=[], head=False, author=actor, committer=actor)
123+
assert repo.merge_base(first, first) == [first]
124+
assert repo.merge_base(first, second) == []
125+
with pytest.raises(GitCommandError) as raised:
126+
repo.merge_base(first, second, invalid_option=True)
127+
assert raised.value.status == 129
128+
129+
130+
@pytest.mark.parametrize("allow_unsafe_options", [False, True])
131+
@pytest.mark.parametrize("option", ["pathspec_from_file", "pathspec-from-file", "pathspec_from"])
132+
@pytest.mark.parametrize("dry_run", [False, True])
133+
def test_move_checks_unsafe_options(tmp_path, option, dry_run, allow_unsafe_options):
134+
repo = Repo.init(tmp_path)
135+
with mock.patch.object(Git, "execute", return_value="Renaming source to destination\n") as run:
136+
kwargs = {option: "unused", "dry_run": dry_run}
137+
if allow_unsafe_options:
138+
assert repo.index.move(["source", "destination"], True, allow_unsafe_options=True, **kwargs) == [
139+
("source", "destination")
140+
]
141+
assert run.call_count == (1 if dry_run else 2)
142+
for call in run.call_args_list:
143+
argv = call[0][0]
144+
assert "-k" in argv
145+
assert f"--{option.replace('_', '-')}=unused" in argv
146+
assert "--allow-unsafe-options" not in argv
147+
assert argv[-3:] == ["--", "source", "destination"]
148+
else:
149+
with pytest.raises(UnsafeOptionError):
150+
repo.index.move(["source", "destination"], **kwargs)
151+
run.assert_not_called()

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL