diff --git a/doc/source/changes.rst b/doc/source/changes.rst index df003c7d3..7fcd68537 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -9,6 +9,7 @@ Security fixes for * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58 * https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-23mf-xhv8-69c2 +* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-f9j4-qggq-h239 If you can, also try and provide feedback on the upcoming v4 branch https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome. diff --git a/git/cmd.py b/git/cmd.py index 3d7446906..773231edd 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1795,6 +1795,12 @@ def _call_process( This allows your commands to call git more conveniently, as ``None`` is realized as non-existent. + Positional arguments may intentionally contain command options. Higher-level + APIs must separate their operands with ``--`` where the Git command supports + it, or reject option-shaped operands where Git reparses them internally (for + example, ``pull`` and ``remote update``). Shell quoting cannot prevent Git + from interpreting a leading-dash argument as an option. + :param kwargs: Contains key-values for the following: diff --git a/git/index/base.py b/git/index/base.py index 560fc5e2c..b86ba5748 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1133,6 +1133,7 @@ def move( args = [] if skip_errors: args.append("-k") + args.append("--") paths = self._items_to_rela_paths(items) if len(paths) < 2: diff --git a/git/refs/head.py b/git/refs/head.py index 5dd4a5a0a..c08ce61b3 100644 --- a/git/refs/head.py +++ b/git/refs/head.py @@ -166,7 +166,7 @@ def delete(cls, repo: "Repo", *heads: "Union[Head, str]", force: bool = False, * flag = "-d" if force: flag = "-D" - repo.git.branch(flag, *heads) + repo.git.branch(flag, "--", *heads) def set_tracking_branch(self, remote_reference: Union["RemoteReference", None]) -> "Head": """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": if force: flag = "-M" - self.repo.git.branch(flag, self, new_path) + self.repo.git.branch(flag, "--", self, new_path) self.path = "%s/%s" % (self._common_path_default, new_path) return self diff --git a/git/refs/remote.py b/git/refs/remote.py index e16ae70f8..60da3ec0d 100644 --- a/git/refs/remote.py +++ b/git/refs/remote.py @@ -61,7 +61,7 @@ def delete(cls, repo: "Repo", *refs: "RemoteReference", **kwargs: Any) -> None: for ref in refs: cls._check_ref_name_valid(ref.path) - repo.git.branch("-d", "-r", *refs) + repo.git.branch("-d", "-r", "--", *refs) # The official deletion method will ignore remote symbolic refs - these are # generally ignored in the refs/ folder. We don't though and delete remainders # manually. diff --git a/git/refs/tag.py b/git/refs/tag.py index 3a7d946c1..50b8a9cb2 100644 --- a/git/refs/tag.py +++ b/git/refs/tag.py @@ -155,7 +155,7 @@ def create( if force: kwargs["f"] = True - args = (path, reference) + args = ("--", path, reference) repo.git.tag(*args, **kwargs) return TagReference(repo, "%s/%s" % (cls._common_path_default, path)) @@ -163,7 +163,7 @@ def create( @classmethod def delete(cls, repo: "Repo", *tags: "TagReference") -> None: # type: ignore[override] """Delete the given existing tag or tags.""" - repo.git.tag("-d", *tags) + repo.git.tag("-d", "--", *tags) # Provide an alias. diff --git a/git/remote.py b/git/remote.py index fd58c8bd5..94f3be987 100644 --- a/git/remote.py +++ b/git/remote.py @@ -14,7 +14,7 @@ from git.cmd import Git, handle_process_output from git.compat import defenc, force_text from git.config import GitConfigParser, SectionConstraint, cp -from git.exc import GitCommandError +from git.exc import GitCommandError, UnsafeOptionError from git.refs import Head, Reference, RemoteReference, SymbolicReference, TagReference from git.util import ( CallableRemoteProgress, @@ -871,6 +871,9 @@ def update(self, **kwargs: Any) -> "Remote": :return: self """ + # Like pull, remote update forwards operands to fetch without `--`. + if self.name.startswith("-"): + raise UnsafeOptionError("Remote names used by update must not start with '-'.") scmd = "update" kwargs["insert_kwargs_after"] = scmd self.repo.git.remote(scmd, self.name, **kwargs) @@ -1101,7 +1104,8 @@ def pull( merge of branch with your local branch. :param refspec: - See :meth:`fetch` method. + See :meth:`fetch` method. Values starting with ``-`` are rejected, + even when ``allow_unsafe_options`` is enabled. Pass options as keywords. :param progress: See :meth:`push` method. @@ -1127,6 +1131,12 @@ def pull( kwargs = add_progress(kwargs, self.repo.git, progress) refspec = Git._unpack_args(refspec or []) + # Git pull forwards these operands to fetch without preserving `--`. + # Reject every option-shaped operand, including with unsafe options enabled: + # opting into an explicit option must not turn a refspec into an option. + for operand in [self.name, *refspec]: + if operand.startswith("-"): + raise UnsafeOptionError("Remote names and pull refspecs must not start with '-'.") if not allow_unsafe_protocols: for ref in refspec: Git.check_unsafe_protocols(ref) diff --git a/git/repo/base.py b/git/repo/base.py index 29922e118..cf496201b 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -1137,7 +1137,7 @@ def ignored(self, *paths: PathLike) -> List[str]: Subset of those paths which are ignored """ try: - proc: str = self.git.check_ignore(*paths) + proc: str = self.git.check_ignore("--", *paths) except GitCommandError as err: if err.status == 1: # If return code is 1, this means none of the items in *paths are diff --git a/test/test_positional_args.py b/test/test_positional_args.py new file mode 100644 index 000000000..2a7bb405e --- /dev/null +++ b/test/test_positional_args.py @@ -0,0 +1,129 @@ +"""High-level operands must not become Git options.""" + +from unittest import mock + +import pytest + +from git import Actor, Git, GitCommandError, Head, Remote, RemoteReference, Repo, TagReference +from git.exc import UnsafeOptionError + + +@pytest.mark.parametrize("allow_unsafe_options", [False, True]) +@pytest.mark.parametrize( + "refspec", + ["--upload-pack=helper", ["--upl=helper"], ["main", "--dry-run"], "-uhelper", "--arg value", "--"], +) +def test_pull_rejects_option_shaped_refspec(tmp_path, refspec, allow_unsafe_options): + repo = Repo.init(tmp_path) + remote = Remote(repo, "origin") + with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeOptionError): + remote.pull(refspec, allow_unsafe_options=allow_unsafe_options) + run.assert_not_called() + + +def test_pull_rejects_option_shaped_remote(tmp_path): + repo = Repo.init(tmp_path) + remote = Remote(repo, "--upload-pack=helper") + with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeOptionError): + remote.pull("main") + run.assert_not_called() + + +def test_pull_preserves_operand_and_explicit_option_values(tmp_path): + repo = Repo.init(tmp_path) + remote = Remote(repo, "origin") + with mock.patch.object(Git, "_call_process") as run, mock.patch.object( + Remote, "_get_fetch_info_from_stderr", return_value=[] + ): + remote.pull("refs/heads/topic", upload_pack="helper with spaces", allow_unsafe_options=True) + assert run.call_args[0] == ("pull", "--", remote, ["refs/heads/topic"]) + assert run.call_args[1]["upload_pack"] == "helper with spaces" + + +def test_delete_head_cannot_override_force(tmp_path): + repo = Repo.init(tmp_path) + actor = Actor("Test", "test@example.com") + initial = repo.index.commit("initial", author=actor, committer=actor) + branch = repo.create_head("unmerged", initial) + branch.commit = repo.index.commit("unmerged", head=False, author=actor, committer=actor) + with pytest.raises(GitCommandError): + repo.delete_head("--force", branch, force=False) + assert branch.is_valid() + repo.delete_head(branch, force=True) + assert not branch.is_valid() + + +def test_rename_head_cannot_select_current_branch(tmp_path): + repo = Repo.init(tmp_path) + actor = Actor("Test", "test@example.com") + repo.index.commit("initial", author=actor, committer=actor) + original = repo.active_branch.name + with pytest.raises(GitCommandError): + Head(repo, "refs/heads/--force").rename("renamed") + assert repo.active_branch.name == original + + +def test_tag_operands_follow_option_terminator(tmp_path): + repo = Repo.init(tmp_path) + with mock.patch.object(Git, "_call_process") as run: + TagReference.create(repo, "topic", "HEAD") + assert run.call_args[0] == ("tag", "--", "topic", "HEAD") + TagReference.delete(repo, "--list") + assert run.call_args[0] == ("tag", "-d", "--", "--list") + + +def test_remote_ref_delete_preserves_operand(tmp_path): + repo = Repo.init(tmp_path) + ref = RemoteReference(repo, "refs/remotes/--force") + with mock.patch.object(Git, "_call_process") as run: + RemoteReference.delete(repo, ref) + assert run.call_args[0] == ("branch", "-d", "-r", "--", ref) + + +def test_move_treats_option_shaped_source_as_filename(tmp_path): + repo = Repo.init(tmp_path) + (tmp_path / "--force").write_text("literal source") + repo.index.add(["--force"]) + assert repo.index.move(["--force", "destination"]) == [("--force", "destination")] + assert (tmp_path / "destination").read_text() == "literal source" + assert not (tmp_path / "--force").exists() + + +def test_move_cannot_override_overwrite_protection(tmp_path): + repo = Repo.init(tmp_path) + for name in ("--force", "source", "destination"): + (tmp_path / name).write_text(name) + repo.index.add(["--force", "source", "destination"]) + with pytest.raises(GitCommandError): + repo.index.move(["--force", "source", "destination"]) + assert (tmp_path / "source").read_text() == "source" + assert (tmp_path / "destination").read_text() == "destination" + repo.index.move(["source", "destination"], force=True) + assert (tmp_path / "destination").read_text() == "source" + + +def test_ignored_treats_option_shaped_path_as_filename(tmp_path): + repo = Repo.init(tmp_path) + (tmp_path / ".gitignore").write_text("--verbose\n--arg value\n") + assert repo.ignored("--verbose", "--arg value") == ["--verbose", "--arg value"] + + +def test_move_cannot_override_dry_run(tmp_path): + repo = Repo.init(tmp_path) + (tmp_path / "source").write_text("source") + repo.index.add(["source"]) + with pytest.raises(GitCommandError): + repo.index.move(["--no-dry-run", "source", "destination"], dry_run=True) + assert (tmp_path / "source").read_text() == "source" + assert not (tmp_path / "destination").exists() + + +@pytest.mark.parametrize("name", ["--prune", "--all", "--upload-pack=helper"]) +def test_remote_update_rejects_option_shaped_name(tmp_path, name): + repo = Repo.init(tmp_path) + with mock.patch.object(Git, "_call_process", side_effect=AssertionError("Git must not run")) as run: + with pytest.raises(UnsafeOptionError): + Remote(repo, name).update() + run.assert_not_called()