Skip to content

Commit dafafff

Browse files
authored
Merge pull request #2247 from gitpython-developers/pos-arg-sanitization
fix: preserve positional operands in high-level Git commands
2 parents 583ecaa + edcd66f commit dafafff

9 files changed

Lines changed: 155 additions & 8 deletions

File tree

doc/source/changes.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ Security fixes for
99

1010
* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58
1111
* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-23mf-xhv8-69c2
12+
* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-f9j4-qggq-h239
1213

1314
If you can, also try and provide feedback on the upcoming v4 branch
1415
https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome.

git/cmd.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,6 +1795,12 @@ def _call_process(
17951795
This allows your commands to call git more conveniently, as ``None`` is
17961796
realized as non-existent.
17971797
1798+
Positional arguments may intentionally contain command options. Higher-level
1799+
APIs must separate their operands with ``--`` where the Git command supports
1800+
it, or reject option-shaped operands where Git reparses them internally (for
1801+
example, ``pull`` and ``remote update``). Shell quoting cannot prevent Git
1802+
from interpreting a leading-dash argument as an option.
1803+
17981804
:param kwargs:
17991805
Contains key-values for the following:
18001806

git/index/base.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,7 @@ def move(
11321132
args = []
11331133
if skip_errors:
11341134
args.append("-k")
1135+
args.append("--")
11351136

11361137
paths = self._items_to_rela_paths(items)
11371138
if len(paths) < 2:

git/refs/head.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ def delete(cls, repo: "Repo", *heads: "Union[Head, str]", force: bool = False, *
166166
flag = "-d"
167167
if force:
168168
flag = "-D"
169-
repo.git.branch(flag, *heads)
169+
repo.git.branch(flag, "--", *heads)
170170

171171
def set_tracking_branch(self, remote_reference: Union["RemoteReference", None]) -> "Head":
172172
"""Configure this branch to track the given remote reference. This will
@@ -241,7 +241,7 @@ def rename(self, new_path: PathLike, force: bool = False) -> "Head":
241241
if force:
242242
flag = "-M"
243243

244-
self.repo.git.branch(flag, self, new_path)
244+
self.repo.git.branch(flag, "--", self, new_path)
245245
self.path = "%s/%s" % (self._common_path_default, new_path)
246246
return self
247247

git/refs/remote.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def delete(cls, repo: "Repo", *refs: "RemoteReference", **kwargs: Any) -> None:
6161
for ref in refs:
6262
cls._check_ref_name_valid(ref.path)
6363

64-
repo.git.branch("-d", "-r", *refs)
64+
repo.git.branch("-d", "-r", "--", *refs)
6565
# The official deletion method will ignore remote symbolic refs - these are
6666
# generally ignored in the refs/ folder. We don't though and delete remainders
6767
# manually.

git/refs/tag.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,15 +155,15 @@ def create(
155155
if force:
156156
kwargs["f"] = True
157157

158-
args = (path, reference)
158+
args = ("--", path, reference)
159159

160160
repo.git.tag(*args, **kwargs)
161161
return TagReference(repo, "%s/%s" % (cls._common_path_default, path))
162162

163163
@classmethod
164164
def delete(cls, repo: "Repo", *tags: "TagReference") -> None: # type: ignore[override]
165165
"""Delete the given existing tag or tags."""
166-
repo.git.tag("-d", *tags)
166+
repo.git.tag("-d", "--", *tags)
167167

168168

169169
# Provide an alias.

git/remote.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
from git.cmd import Git, handle_process_output
1515
from git.compat import defenc, force_text
1616
from git.config import GitConfigParser, SectionConstraint, cp
17-
from git.exc import GitCommandError
17+
from git.exc import GitCommandError, UnsafeOptionError
1818
from git.refs import Head, Reference, RemoteReference, SymbolicReference, TagReference
1919
from git.util import (
2020
CallableRemoteProgress,
@@ -871,6 +871,9 @@ def update(self, **kwargs: Any) -> "Remote":
871871
:return:
872872
self
873873
"""
874+
# Like pull, remote update forwards operands to fetch without `--`.
875+
if self.name.startswith("-"):
876+
raise UnsafeOptionError("Remote names used by update must not start with '-'.")
874877
scmd = "update"
875878
kwargs["insert_kwargs_after"] = scmd
876879
self.repo.git.remote(scmd, self.name, **kwargs)
@@ -1101,7 +1104,8 @@ def pull(
11011104
merge of branch with your local branch.
11021105
11031106
:param refspec:
1104-
See :meth:`fetch` method.
1107+
See :meth:`fetch` method. Values starting with ``-`` are rejected,
1108+
even when ``allow_unsafe_options`` is enabled. Pass options as keywords.
11051109
11061110
:param progress:
11071111
See :meth:`push` method.
@@ -1127,6 +1131,12 @@ def pull(
11271131
kwargs = add_progress(kwargs, self.repo.git, progress)
11281132

11291133
refspec = Git._unpack_args(refspec or [])
1134+
# Git pull forwards these operands to fetch without preserving `--`.
1135+
# Reject every option-shaped operand, including with unsafe options enabled:
1136+
# opting into an explicit option must not turn a refspec into an option.
1137+
for operand in [self.name, *refspec]:
1138+
if operand.startswith("-"):
1139+
raise UnsafeOptionError("Remote names and pull refspecs must not start with '-'.")
11301140
if not allow_unsafe_protocols:
11311141
for ref in refspec:
11321142
Git.check_unsafe_protocols(ref)

git/repo/base.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1137,7 +1137,7 @@ def ignored(self, *paths: PathLike) -> List[str]:
11371137
Subset of those paths which are ignored
11381138
"""
11391139
try:
1140-
proc: str = self.git.check_ignore(*paths)
1140+
proc: str = self.git.check_ignore("--", *paths)
11411141
except GitCommandError as err:
11421142
if err.status == 1:
11431143
# If return code is 1, this means none of the items in *paths are

test/test_positional_args.py

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
"""High-level operands must not become Git options."""
2+
3+
from unittest import mock
4+
5+
import pytest
6+
7+
from git import Actor, Git, GitCommandError, Head, Remote, RemoteReference, Repo, TagReference
8+
from git.exc import UnsafeOptionError
9+
10+
11+
@pytest.mark.parametrize("allow_unsafe_options", [False, True])
12+
@pytest.mark.parametrize(
13+
"refspec",
14+
["--upload-pack=helper", ["--upl=helper"], ["main", "--dry-run"], "-uhelper", "--arg value", "--"],
15+
)
16+
def test_pull_rejects_option_shaped_refspec(tmp_path, refspec, allow_unsafe_options):
17+
repo = Repo.init(tmp_path)
18+
remote = Remote(repo, "origin")
19+
with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run:
20+
with pytest.raises(UnsafeOptionError):
21+
remote.pull(refspec, allow_unsafe_options=allow_unsafe_options)
22+
run.assert_not_called()
23+
24+
25+
def test_pull_rejects_option_shaped_remote(tmp_path):
26+
repo = Repo.init(tmp_path)
27+
remote = Remote(repo, "--upload-pack=helper")
28+
with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run:
29+
with pytest.raises(UnsafeOptionError):
30+
remote.pull("main")
31+
run.assert_not_called()
32+
33+
34+
def test_pull_preserves_operand_and_explicit_option_values(tmp_path):
35+
repo = Repo.init(tmp_path)
36+
remote = Remote(repo, "origin")
37+
with mock.patch.object(Git, "_call_process") as run, mock.patch.object(
38+
Remote, "_get_fetch_info_from_stderr", return_value=[]
39+
):
40+
remote.pull("refs/heads/topic", upload_pack="helper with spaces", allow_unsafe_options=True)
41+
assert run.call_args[0] == ("pull", "--", remote, ["refs/heads/topic"])
42+
assert run.call_args[1]["upload_pack"] == "helper with spaces"
43+
44+
45+
def test_delete_head_cannot_override_force(tmp_path):
46+
repo = Repo.init(tmp_path)
47+
actor = Actor("Test", "test@example.com")
48+
initial = repo.index.commit("initial", author=actor, committer=actor)
49+
branch = repo.create_head("unmerged", initial)
50+
branch.commit = repo.index.commit("unmerged", head=False, author=actor, committer=actor)
51+
with pytest.raises(GitCommandError):
52+
repo.delete_head("--force", branch, force=False)
53+
assert branch.is_valid()
54+
repo.delete_head(branch, force=True)
55+
assert not branch.is_valid()
56+
57+
58+
def test_rename_head_cannot_select_current_branch(tmp_path):
59+
repo = Repo.init(tmp_path)
60+
actor = Actor("Test", "test@example.com")
61+
repo.index.commit("initial", author=actor, committer=actor)
62+
original = repo.active_branch.name
63+
with pytest.raises(GitCommandError):
64+
Head(repo, "refs/heads/--force").rename("renamed")
65+
assert repo.active_branch.name == original
66+
67+
68+
def test_tag_operands_follow_option_terminator(tmp_path):
69+
repo = Repo.init(tmp_path)
70+
with mock.patch.object(Git, "_call_process") as run:
71+
TagReference.create(repo, "topic", "HEAD")
72+
assert run.call_args[0] == ("tag", "--", "topic", "HEAD")
73+
TagReference.delete(repo, "--list")
74+
assert run.call_args[0] == ("tag", "-d", "--", "--list")
75+
76+
77+
def test_remote_ref_delete_preserves_operand(tmp_path):
78+
repo = Repo.init(tmp_path)
79+
ref = RemoteReference(repo, "refs/remotes/--force")
80+
with mock.patch.object(Git, "_call_process") as run:
81+
RemoteReference.delete(repo, ref)
82+
assert run.call_args[0] == ("branch", "-d", "-r", "--", ref)
83+
84+
85+
def test_move_treats_option_shaped_source_as_filename(tmp_path):
86+
repo = Repo.init(tmp_path)
87+
(tmp_path / "--force").write_text("literal source")
88+
repo.index.add(["--force"])
89+
assert repo.index.move(["--force", "destination"]) == [("--force", "destination")]
90+
assert (tmp_path / "destination").read_text() == "literal source"
91+
assert not (tmp_path / "--force").exists()
92+
93+
94+
def test_move_cannot_override_overwrite_protection(tmp_path):
95+
repo = Repo.init(tmp_path)
96+
for name in ("--force", "source", "destination"):
97+
(tmp_path / name).write_text(name)
98+
repo.index.add(["--force", "source", "destination"])
99+
with pytest.raises(GitCommandError):
100+
repo.index.move(["--force", "source", "destination"])
101+
assert (tmp_path / "source").read_text() == "source"
102+
assert (tmp_path / "destination").read_text() == "destination"
103+
repo.index.move(["source", "destination"], force=True)
104+
assert (tmp_path / "destination").read_text() == "source"
105+
106+
107+
def test_ignored_treats_option_shaped_path_as_filename(tmp_path):
108+
repo = Repo.init(tmp_path)
109+
(tmp_path / ".gitignore").write_text("--verbose\n--arg value\n")
110+
assert repo.ignored("--verbose", "--arg value") == ["--verbose", "--arg value"]
111+
112+
113+
def test_move_cannot_override_dry_run(tmp_path):
114+
repo = Repo.init(tmp_path)
115+
(tmp_path / "source").write_text("source")
116+
repo.index.add(["source"])
117+
with pytest.raises(GitCommandError):
118+
repo.index.move(["--no-dry-run", "source", "destination"], dry_run=True)
119+
assert (tmp_path / "source").read_text() == "source"
120+
assert not (tmp_path / "destination").exists()
121+
122+
123+
@pytest.mark.parametrize("name", ["--prune", "--all", "--upload-pack=helper"])
124+
def test_remote_update_rejects_option_shaped_name(tmp_path, name):
125+
repo = Repo.init(tmp_path)
126+
with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run:
127+
with pytest.raises(UnsafeOptionError):
128+
Remote(repo, name).update()
129+
run.assert_not_called()

0 commit comments

Comments
 (0)