diff --git a/CHANGES b/CHANGES index ba893f331..d58c6b368 100644 --- a/CHANGES +++ b/CHANGES @@ -22,6 +22,28 @@ _Notes on the upcoming release will go here._ ### Fixes +#### Reject option-like URLs, revisions, and refs across VCS backends + +libvcs now rejects an argument that a VCS binary would parse as an option in a +position where an end-of-options `--` cannot neutralize it: + +- {meth}`~libvcs.cmd.git.Git.pull` raises {exc}`~libvcs.exc.LibVCSException` + when `repository` or `reftag` begins with `-`. `git pull` re-invokes `git + fetch` without a `--`, so the separator before pull's positionals never + reached the child; a value such as `--upload-pack=` was parsed there and + ran an arbitrary command. +- {meth}`~libvcs.sync.git.GitSync.update_repo` records a `rev` error instead of + running the command when the configured revision begins with `-`. The + revision reached `git rev-list `, where `--output=` was parsed + as `git`'s diff `--output` option and truncated that file. +- {meth}`~libvcs.cmd.svn.Svn.checkout` raises when `url` begins with `-`; svn's + `--config-option` can set a tunnel command, an execution primitive. This + matches the protection the git and hg clone paths already carried. + +A report from [Harsh Raj Singhania](https://github.com/HarshRajSinghania) +prompted this audit of how arguments reach the git, hg, and svn binaries. The +reported issue did not reproduce; the fixes above are what the audit found. + #### Progress callback timestamps carry a time zone (#549) The `timestamp` passed to a diff --git a/src/libvcs/_internal/run.py b/src/libvcs/_internal/run.py index c559116fb..c74481cf2 100644 --- a/src/libvcs/_internal/run.py +++ b/src/libvcs/_internal/run.py @@ -115,6 +115,23 @@ def _normalize_command_args(args: _CMD) -> list[StrOrBytesPath]: return [os.fspath(arg) for arg in args] +def reject_option_like(value: str, *, name: str) -> str: + """Return ``value`` unless it would be parsed as a command-line option. + + A value beginning with ``-`` lands in the argv where the tool reads an + operand and is instead parsed as an option. Where an end-of-options ``--`` + cannot neutralize that — ``git pull`` re-spawns ``git fetch`` without one, + and a rev separator means a pathspec — the value must be rejected instead. + """ + if value.startswith("-"): + msg = ( + f"{name} may not begin with '-': {value!r} would be parsed as a " + "command-line option (argument injection)." + ) + raise exc.LibVCSException(msg) + return value + + def _stringify_command(args: _CMD) -> str | list[str]: """Return a human-readable command for CommandError.""" if isinstance(args, (str, bytes, os.PathLike)): diff --git a/src/libvcs/cmd/git.py b/src/libvcs/cmd/git.py index e6f695f1e..bdf225a9f 100644 --- a/src/libvcs/cmd/git.py +++ b/src/libvcs/cmd/git.py @@ -12,7 +12,12 @@ from collections.abc import Sequence from libvcs._internal.query_list import QueryList -from libvcs._internal.run import ProgressCallbackProtocol, _normalize_command_args, run +from libvcs._internal.run import ( + ProgressCallbackProtocol, + _normalize_command_args, + reject_option_like, + run, +) from libvcs._internal.types import StrOrBytesPath, StrPath _CMD = StrOrBytesPath | Sequence[StrOrBytesPath] @@ -914,9 +919,9 @@ def pull( """ required_flags: list[str] = [] if repository: - required_flags.insert(0, repository) + required_flags.insert(0, reject_option_like(repository, name="repository")) if reftag: - required_flags.insert(0, reftag) + required_flags.insert(0, reject_option_like(str(reftag), name="reftag")) local_flags: list[str] = [] # diff --git a/src/libvcs/cmd/svn.py b/src/libvcs/cmd/svn.py index 8c32a821c..13d11416d 100644 --- a/src/libvcs/cmd/svn.py +++ b/src/libvcs/cmd/svn.py @@ -16,7 +16,12 @@ from collections.abc import Sequence from libvcs import exc -from libvcs._internal.run import ProgressCallbackProtocol, _normalize_command_args, run +from libvcs._internal.run import ( + ProgressCallbackProtocol, + _normalize_command_args, + reject_option_like, + run, +) from libvcs._internal.types import StrOrBytesPath, StrPath _CMD: t.TypeAlias = StrOrBytesPath | Sequence[StrOrBytesPath] @@ -212,7 +217,7 @@ def checkout( >>> svn.checkout(url=f'file://{svn_remote_repo}', revision=10) 'svn: E160006: No such revision 10...' """ - local_flags: list[str] = [url, str(self.path)] + local_flags: list[str] = [reject_option_like(url, name="url"), str(self.path)] if revision is not None: local_flags.extend(["--revision", str(revision)]) diff --git a/src/libvcs/sync/git.py b/src/libvcs/sync/git.py index c56c14860..2d14c3b5e 100644 --- a/src/libvcs/sync/git.py +++ b/src/libvcs/sync/git.py @@ -26,6 +26,7 @@ from urllib import parse as urlparse from libvcs import exc +from libvcs._internal.run import reject_option_like from libvcs._internal.types import StrPath from libvcs.cmd.git import Git from libvcs.sync.base import ( @@ -491,6 +492,13 @@ def update_repo( # Get requested revision or tag url, git_tag = self.url, getattr(self, "rev", None) + if git_tag: + try: + reject_option_like(str(git_tag), name="rev") + except exc.LibVCSException as e: + result.add_error("rev", str(e), exception=e) + return result + if not git_tag: self.log.debug("No git revision set, defaulting to origin/master") try: diff --git a/tests/cmd/test_git.py b/tests/cmd/test_git.py index 09e9978f8..2851ce4b9 100644 --- a/tests/cmd/test_git.py +++ b/tests/cmd/test_git.py @@ -2785,3 +2785,54 @@ def test_run_failure_preserves_stderr_lines(git_repo: GitSync) -> None: # localize, rather than on translatable English error text. assert "\n" in output assert "no-such-remote" in output + + +def test_pull_rejects_option_like_repository( + create_git_remote_repo: CreateRepoFn, + git_repo: GitSync, + tmp_path: pathlib.Path, +) -> None: + """Reject a repository argument that git would parse as an option. + + ``git pull`` re-spawns ``git fetch`` without an end-of-options ``--``, so + the separator libvcs places before its positionals does not protect this + command. A ``reftag`` beginning with ``-`` reaches the child ``git fetch`` + as an option; ``--upload-pack=`` there is arbitrary command execution. + """ + remote_repo = create_git_remote_repo() + git_repo.cmd.remotes.add(name="origin", url=f"file://{remote_repo}") + canary = tmp_path / "PULL_PWNED" + + with pytest.raises(exc.LibVCSException): + git_repo.cmd.pull( + reftag=f"--upload-pack=touch {canary}", + repository="origin", + check_returncode=True, + ) + + assert not canary.exists(), "Prevent argument injection via git pull" + + +def test_clone_places_end_of_options_before_url( + tmp_path: pathlib.Path, + mocker: MockerFixture, +) -> None: + """Pin the ``--`` separator ahead of the clone URL. + + ``git clone`` reads the URL as a positional; the ``--`` libvcs emits before + it is what stops a ``--upload-pack=`` URL from being parsed as an + option. No other test covers this separator, so a refactor dropping it would + otherwise pass -- the single-positional clone shape does not execute the + payload, making an end-to-end canary test vacuous here. + """ + repo = git.Git(path=tmp_path) + mock_run = mocker.patch("libvcs.cmd.git.run", return_value="") + + repo.clone(url="https://example.com/repo.git") + + _args, kwargs = mock_run.call_args + argv = [os.fspath(a) for a in kwargs["args"]] + assert "--" in argv, "clone must emit an end-of-options separator" + assert argv[argv.index("--") + 1] == "https://example.com/repo.git", ( + "URL must follow the -- separator, not precede it" + ) diff --git a/tests/cmd/test_svn.py b/tests/cmd/test_svn.py index 0ecf06049..452b04fec 100644 --- a/tests/cmd/test_svn.py +++ b/tests/cmd/test_svn.py @@ -8,6 +8,7 @@ import pytest +from libvcs import exc from libvcs.cmd.svn import Svn if t.TYPE_CHECKING: @@ -38,3 +39,16 @@ def test_svn_run_timeout_propagates_to_runner( _args, kwargs = mock_run.call_args assert kwargs.get("timeout") == 2.5 + + +def test_checkout_rejects_option_like_url(tmp_path: pathlib.Path) -> None: + """Reject a checkout URL that svn would parse as an option. + + ``svn checkout`` takes the URL as its first positional. A value beginning + with ``-`` -- e.g. ``--config-option=config:tunnels:ssh=`` -- is parsed + as an option instead, and svn's tunnel config is an execution primitive. + """ + svn = Svn(path=tmp_path) + + with pytest.raises(exc.LibVCSException, match="may not begin with"): + svn.checkout(url="--config-option=config:tunnels:ssh=touch ./PWNED") diff --git a/tests/sync/test_git.py b/tests/sync/test_git.py index 235eae3f3..1fb8abff8 100644 --- a/tests/sync/test_git.py +++ b/tests/sync/test_git.py @@ -1847,3 +1847,24 @@ def test_remote_swallows_libvcs_exception( ) assert git_repo.remote("origin") is None + + +def test_update_repo_rejects_option_like_rev( + git_repo: GitSync, + tmp_path: pathlib.Path, +) -> None: + """Reject a revision that git would parse as an option. + + A `rev` reaches `git rev-list `, which has no end-of-options `--` + before the operand. A value such as `--output=` is parsed there as + the diff `--output` option, whose callback truncates the file during + option parsing -- arbitrary file destruction from a config-supplied rev. + """ + victim = tmp_path / "victim.txt" + victim.write_text("important\n") + git_repo.rev = f"--output={victim}" + + result = git_repo.update_repo() + + assert not result.ok, "update_repo() should fail for an option-like rev" + assert victim.read_text() == "important\n", "Prevent rev argument injection"