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)