Merge pull request #2172 from gitpython-developers/fix-clone-expandvars

fix: preserve literal clone URLs

Author: Byron

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
 

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
 

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,20 +416,23 @@ 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?
     if not path.startswith(("/cygdrive", "//", "/proc/cygdrive")):
         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
 

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)