From 6575026d85e9bf8037caf847619ac770e4f4633b Mon Sep 17 00:00:00 2001 From: Codex GPT-5 Date: Wed, 15 Jul 2026 16:44:13 +0200 Subject: [PATCH] fix: prevent environment expansion in clone URLs Repo.clone_from() expanded environment-variable references in caller-supplied URLs before passing them to git clone. This could expose process environment values to an untrusted remote and made protocol validation apply to a different value than Git received. Polish clone URLs without variable or home expansion on native and Cygwin Git, then apply unsafe-protocol validation to that exact normalized value. Preserve the literal URL when normalizing the stored origin after a successful clone, while retaining the existing Git.polish_url() default for callers that intentionally normalize local paths. Add regression coverage for POSIX and Windows variable syntax, Cygwin conversion, stored origins, and post-normalization protocol validation. Git baseline: git clone passes URL arguments through literally; t/t5601-clone.sh covers the accepted URL forms without shell-style environment expansion. Security: GHSA-rwj8-pgh3-r573. Validation: - pytest test/test_clone.py -q (21 passed, 1 skipped) - pytest test/test_util.py -q - ruff check on changed files - ruff format --check on changed files - full pytest suite (681 passed, 73 skipped, 1 xfailed; 7 unrelated environment/baseline failures) --- git/cmd.py | 19 ++++++++++++------- git/repo/base.py | 7 ++++--- git/util.py | 15 +++++++++------ test/test_clone.py | 41 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 65 insertions(+), 17 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 2a4f36024..150c9d16f 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -903,29 +903,34 @@ def is_cygwin(cls) -> bool: @overload @classmethod - def polish_url(cls, url: str, is_cygwin: Literal[False] = ...) -> str: ... + def polish_url(cls, url: str, is_cygwin: Literal[False] = ..., expand_vars: bool = ...) -> str: ... @overload @classmethod - def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None) -> str: ... + def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None, expand_vars: bool = True) -> str: ... @classmethod - def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None) -> PathLike: + def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None, expand_vars: bool = True) -> PathLike: """Remove any backslashes from URLs to be written in config files. Windows might create config files containing paths with backslashes, but git stops liking them as it will escape the backslashes. Hence we undo the escaping just to be sure. + + :param expand_vars: + Expand environment variables and an initial ``~``. Disable this for values + obtained from an untrusted source, such as remote URLs. """ if is_cygwin is None: is_cygwin = cls.is_cygwin() if is_cygwin: - url = cygpath(url) + url = cygpath(url, expand_vars=expand_vars) else: - url = os.path.expandvars(url) - if url.startswith("~"): - url = os.path.expanduser(url) + if expand_vars: + url = os.path.expandvars(url) + if url.startswith("~"): + url = os.path.expanduser(url) url = url.replace("\\\\", "\\").replace("\\", "/") return url diff --git a/git/repo/base.py b/git/repo/base.py index 913984975..e478396b2 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -1452,8 +1452,9 @@ def _clone( if multi_options: multi = shlex.split(" ".join(multi_options)) + clone_url = Git.polish_url(url, expand_vars=False) if not allow_unsafe_protocols: - Git.check_unsafe_protocols(url) + Git.check_unsafe_protocols(clone_url) if not allow_unsafe_options: Git.check_unsafe_options( options=Git._option_candidates([], kwargs), @@ -1465,7 +1466,7 @@ def _clone( proc = git.clone( multi, "--", - Git.polish_url(url), + clone_url, clone_path, with_extended_output=True, as_process=True, @@ -1505,7 +1506,7 @@ def _clone( # escape the backslashes. Hence we undo the escaping just to be sure. if repo.remotes: with repo.remotes[0].config_writer as writer: - writer.set_value("url", Git.polish_url(repo.remotes[0].url)) + writer.set_value("url", Git.polish_url(repo.remotes[0].url, expand_vars=False)) # END handle remote repo return repo diff --git a/git/util.py b/git/util.py index 712fabe85..11c1a0b15 100644 --- a/git/util.py +++ b/git/util.py @@ -382,13 +382,13 @@ def is_exec(fpath: str) -> bool: return progs -def _cygexpath(drive: Optional[str], path: str) -> str: +def _cygexpath(drive: Optional[str], path: str, expand_vars: bool = True) -> str: if osp.isabs(path) and not drive: # Invoked from `cygpath()` directly with `D:Apps\123`? # It's an error, leave it alone just slashes) p = path # convert to str if AnyPath given else: - p = path and osp.normpath(osp.expandvars(osp.expanduser(path))) + p = path and osp.normpath(osp.expandvars(osp.expanduser(path)) if expand_vars else path) if osp.isabs(p): if drive: # Confusing, maybe a remote system should expand vars. @@ -416,7 +416,7 @@ def _cygexpath(drive: Optional[str], path: str) -> str: ) -def cygpath(path: str) -> str: +def cygpath(path: str, expand_vars: bool = True) -> str: """Use :meth:`git.cmd.Git.polish_url` instead, that works on any environment.""" path = os.fspath(path) # Ensure is str and not AnyPath. # Fix to use Paths when 3.5 dropped. Or to be just str if only for URLs? @@ -424,12 +424,15 @@ def cygpath(path: str) -> str: for regex, parser, recurse in _cygpath_parsers: match = regex.match(path) if match: - path = parser(*match.groups()) + if parser is _cygexpath: + path = parser(*match.groups(), expand_vars=expand_vars) + else: + path = parser(*match.groups()) if recurse: - path = cygpath(path) + path = cygpath(path, expand_vars=expand_vars) break else: - path = _cygexpath(None, path) + path = _cygexpath(None, path, expand_vars=expand_vars) return path diff --git a/test/test_clone.py b/test/test_clone.py index 79d63dfdc..5fd59b3a3 100644 --- a/test/test_clone.py +++ b/test/test_clone.py @@ -7,8 +7,9 @@ import sys import tempfile from unittest import skip +from unittest import mock -from git import GitCommandError, Repo +from git import Git, GitCommandError, Repo from git.exc import UnsafeOptionError, UnsafeProtocolError from test.lib import TestBase, with_rw_directory, with_rw_repo, PathLikeMock @@ -346,6 +347,44 @@ def test_clone_from_unsafe_protocol(self): Repo.clone_from(url, tmp_dir / "repo") assert not tmp_file.exists() + def test_clone_from_does_not_expand_environment_variables_in_url(self): + urls = [ + "https://example.com/$GITPYTHON_TEST_SECRET/repo.git", + "https://example.com/${GITPYTHON_TEST_SECRET}/repo.git", + "https://example.com/%GITPYTHON_TEST_SECRET%/repo.git", + ] + with mock.patch.dict(os.environ, {"GITPYTHON_TEST_SECRET": "sensitive-value"}): + for url in urls: + with mock.patch.object(Git, "_call_process", side_effect=RuntimeError) as call_process: + with self.assertRaises(RuntimeError): + Repo.clone_from(url, "unused") + + assert call_process.call_args[0][3] == url + + @with_rw_directory + def test_clone_from_does_not_expand_environment_variables_in_stored_url(self, rw_dir): + url = pathlib.Path(rw_dir) / "$GITPYTHON_TEST_SECRET" / "source" + Git().init(url) + + with mock.patch.dict(os.environ, {"GITPYTHON_TEST_SECRET": "sensitive-value"}): + cloned = Repo.clone_from(url, pathlib.Path(rw_dir) / "clone") + + assert cloned.remotes.origin.url == Git.polish_url(str(url), expand_vars=False) + + def test_clone_from_checks_polished_url_for_unsafe_protocol(self): + with mock.patch.object(Git, "polish_url", return_value="ext::command"): + with mock.patch.object(Git, "_call_process") as call_process: + with self.assertRaises(UnsafeProtocolError): + Repo.clone_from("$GITPYTHON_TEST_URL", "unused") + + call_process.assert_not_called() + + def test_polish_url_does_not_expand_environment_variables_for_cygwin(self): + urls = ["$GITPYTHON_TEST_SECRET/repo", "user@example.com:$GITPYTHON_TEST_SECRET/repo"] + with mock.patch.dict(os.environ, {"GITPYTHON_TEST_SECRET": "sensitive-value"}): + for url in urls: + assert Git.polish_url(url, is_cygwin=True, expand_vars=False) == url + def test_clone_from_unsafe_protocol_allowed(self): with tempfile.TemporaryDirectory() as tdir: tmp_dir = pathlib.Path(tdir)