Repository navigation
Commit 790bb31
fix(remote): reject option-shaped pull operands
<!-- agent -->
`Remote.pull()` validated keyword options but forwarded refspecs through
`git pull`, whose internal fetch invocation loses the `--` separator.
GHSA-f9j4-qggq-h239 reports the resulting positional validation bypass.
Reject all leading-dash refspecs and remote names before spawning Git,
rather than trying to enumerate dangerous option spellings. This also
applies with `allow_unsafe_options=True`: explicit options still belong in
keyword arguments. Document the behavior and add an unreleased changelog
entry. No shell quoting change is needed; the problem is Git's own option
parsing, not splitting arguments in Python.
Git reference: `builtin/pull.c:run_fetch()` at Git commit
`12cb6293d6288865c1a133cf22accbaf99d13eb6` forwards the remote and refspecs
without an option terminator. Tested with Apple Git 2.54.0.
Assisted-by: GPT 6.0
Co-authored-by: GPT 6.0 <codex@openai.com>1 parent cf43820 commit 790bb31
3 files changed
Lines changed: 52 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -9,6 +9,7 @@ Security fixes for | |||
| 9 | 9 | ||
| 10 | 10 | * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58 | |
| 11 | 11 | * 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 | ||
| 12 | 13 | ||
| 13 | 14 | If you can, also try and provide feedback on the upcoming v4 branch | |
| 14 | 15 | https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -14,7 +14,7 @@ | |||
| 14 | 14 | from git.cmd import Git, handle_process_output | |
| 15 | 15 | from git.compat import defenc, force_text | |
| 16 | 16 | from git.config import GitConfigParser, SectionConstraint, cp | |
| 17 | - from git.exc import GitCommandError | ||
| 17 | + from git.exc import GitCommandError, UnsafeOptionError | ||
| 18 | 18 | from git.refs import Head, Reference, RemoteReference, SymbolicReference, TagReference | |
| 19 | 19 | from git.util import ( | |
| 20 | 20 | CallableRemoteProgress, | |
@@ -1101,7 +1101,8 @@ def pull( | |||
| 1101 | 1101 | merge of branch with your local branch. | |
| 1102 | 1102 | ||
| 1103 | 1103 | :param refspec: | |
| 1104 | - See :meth:`fetch` method. | ||
| 1104 | + See :meth:`fetch` method. Values starting with ``-`` are rejected, | ||
| 1105 | + even when ``allow_unsafe_options`` is enabled. Pass options as keywords. | ||
| 1105 | 1106 | ||
| 1106 | 1107 | :param progress: | |
| 1107 | 1108 | See :meth:`push` method. | |
@@ -1127,6 +1128,12 @@ def pull( | |||
| 1127 | 1128 | kwargs = add_progress(kwargs, self.repo.git, progress) | |
| 1128 | 1129 | ||
| 1129 | 1130 | refspec = Git._unpack_args(refspec or []) | |
| 1131 | + # Git pull forwards these operands to fetch without preserving `--`. | ||
| 1132 | + # Reject every option-shaped operand, including with unsafe options enabled: | ||
| 1133 | + # opting into an explicit option must not turn a refspec into an option. | ||
| 1134 | + for operand in [self.name, *refspec]: | ||
| 1135 | + if operand.startswith("-"): | ||
| 1136 | + raise UnsafeOptionError("Remote names and pull refspecs must not start with '-'.") | ||
| 1130 | 1137 | if not allow_unsafe_protocols: | |
| 1131 | 1138 | for ref in refspec: | |
| 1132 | 1139 | Git.check_unsafe_protocols(ref) | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,42 @@ | |||
| 1 | + """High-level operands must not become Git options.""" | ||
| 2 | + | ||
| 3 | + from unittest import mock | ||
| 4 | + | ||
| 5 | + import pytest | ||
| 6 | + | ||
| 7 | + from git import Git, Remote, Repo | ||
| 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" | ||
0 commit comments