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.

Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>

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)