GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: complete safety checks in Git command wrappers · gitpython-developers/GitPython@c480bd5

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • git
    • cmd.py
    • index
      • base.py
    • repo
      • base.py
  • test
    • test_command_guards.py
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎git/cmd.py‎

Copy file name to clipboardExpand all lines: git/cmd.py
+14-1Lines 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
Collapse file

‎git/index/base.py‎

Copy file name to clipboardExpand all lines: git/index/base.py
+10Lines 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")
Collapse file

‎git/repo/base.py‎

Copy file name to clipboardExpand all lines: git/repo/base.py
+13-3Lines 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
Collapse file

‎test/test_command_guards.py‎

Copy file name to clipboard
+151Lines 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+
(("[email protected]: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+
"[email protected]: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", "[email protected]")
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 (0)
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.