fix(submodule): resolve relative submodule URLs the way git does

`Submodule.update` resolved a `.gitmodules` URL starting with `./` or
`../` with `urllib.parse.urljoin` against the superproject's `origin`
URL, and `Submodule._clone_repo` appended a `../` URL to the tracking
remote's URL with `os.path.join`. Neither follows git's `relative_url`,
and `urljoin` only resolves against the schemes it knows: for an
`ssh://` or `git://` origin it returns the relative URL unchanged, and
for an scp-like `host:path` origin more than one `../` collapses to a
bare path. `git clone` then received `./planted`, a path inside the
superproject's checkout, where git resolves the same entry to
`ssh://host/org/super.git/planted`.

A submodule was therefore cloned from a directory committed by whoever
authored the superproject, through git's local clone path: `git
upload-pack` ran inside that directory and its objects were copied
without the `transfer.fsckObjects` checks a transport fetch applies (an
object that `git clone file://...` rejects as `badDate` is accepted this
way). Relative submodule URLs are common in multi-repository setups,
and the superproject is untrusted input to any service that updates
submodules of repositories it is given.

Add `resolve_relative_url`, a port of git's `relative_url`: each leading
`../` strips one component of the remote URL, a path component first,
then the scp-like host before the colon, and running out of components
is an error; `./` prefixes are dropped and the rest is appended. It agrees
with what `git submodule init` records over 272 remote and URL pairs,
including the error cases. Use it in `Submodule.update`,
`Submodule._clone_repo` (which now also resolves `./` URLs, previously
passed to `git clone` verbatim), and the URL change path of
`RootModule.update`, which created a remote from the raw relative URL
and fetched it as a path relative to the module's working tree.

The remote used for resolution is unchanged: `origin` in `update`, the
tracking branch's remote in `_clone_repo`. Adds regression tests in
`test/test_submodule.py`; the two behavioral ones fail before the change
with `./planted` reaching the clone unchanged.

Author: Keerthana-64

git/objects/submodule/base.py

@@ -12,7 +12,6 @@
 import shlex
 import stat
 import sys
-import urllib.parse
 import uuid
 from io import BytesIO
 from pathlib import Path
@@ -58,7 +57,9 @@
 from .util import (
     SubmoduleConfigParser,
     find_first_remote_branch,
+    is_relative_url,
     mkhead,
+    resolve_relative_url,
     sm_name,
     sm_section,
 )
@@ -428,10 +429,10 @@ def _clone_repo(
             if not osp.isdir(module_abspath_dir):
                 os.makedirs(module_abspath_dir)
 
-        if url.startswith("../"):
+        if is_relative_url(url):
             remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name
             repo_remote_url = repo.remote(remote_name).url
-            url = os.path.join(repo_remote_url, url)
+            url = resolve_relative_url(repo_remote_url, url)
 
         clone = git.Repo.clone_from(
             url,
@@ -982,13 +983,9 @@ def fetch_remotes(module_repo: "Repo") -> None:
                     if not dry_run:
                         if no_fetch:
                             raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None
-                        if self.url.startswith("."):
-                            url = urllib.parse.urljoin(self.repo.remotes.origin.url + "/", self.url)
-                        else:
-                            url = self.url
                         mrepo = self._clone_repo(
                             self.repo,
-                            url,
+                            self._resolved_url(),
                             self.path,
                             self.name,
                             n=True,
@@ -1738,6 +1735,13 @@ def url(self) -> str:
         """:return: The url to the repository our submodule's repository refers to"""
         return self._url
 
+    def _resolved_url(self) -> str:
+        """:return: :attr:`url`, with a ``./`` or ``../`` prefix resolved against the
+        url of the superproject's ``origin`` remote the way git does"""
+        if not is_relative_url(self.url):
+            return self.url
+        return resolve_relative_url(self.repo.remotes.origin.url, self.url)
+
     @property
     def parent_commit(self) -> "Commit":
         """

git/objects/submodule/root.py

@@ -264,10 +264,14 @@ def update(  # type: ignore[override]
                         nn = "__new_origin__"
                         smm = sm.module()
                         rmts = smm.remotes
+                        # Relative urls are resolved against the superproject's remote,
+                        # which is what the module's remotes hold.
+                        sm_url = sm._resolved_url()
+                        psm_url = psm._resolved_url()
 
                         # Don't do anything if we already have the url we search in
                         # place.
-                        if len([r for r in rmts if r.url == sm.url]) == 0:
+                        if len([r for r in rmts if r.url == sm_url]) == 0:
                             progress.update(
                                 BEGIN | URLCHANGE,
                                 i,
@@ -278,7 +282,7 @@ def update(  # type: ignore[override]
                             if not dry_run:
                                 previous_remote = None
                                 for remote in rmts:
-                                    if remote.url == psm.url:
+                                    if remote.url == psm_url:
                                         previous_remote = remote
                                         break
                                     # END if urls match
@@ -305,10 +309,10 @@ def update(  # type: ignore[override]
                                     # A new remote would have no cached refs. Preserve
                                     # the existing refs and tracking configuration for
                                     # offline updates instead of replacing the remote.
-                                    previous_remote.set_url(git.Git.polish_url(sm.url, expand_vars=False))
+                                    previous_remote.set_url(git.Git.polish_url(sm_url, expand_vars=False))
                                 else:
                                     assert nn not in [r.name for r in rmts]
-                                    smr = smm.create_remote(nn, sm.url)
+                                    smr = smm.create_remote(nn, sm_url)
                                     smr.fetch(progress=progress)
 
                                     # If we have a tracking branch, it should be available

git/objects/submodule/util.py

@@ -6,10 +6,13 @@
     "sm_name",
     "mkhead",
     "find_first_remote_branch",
+    "is_relative_url",
+    "resolve_relative_url",
     "SubmoduleConfigParser",
 ]
 
 from io import BytesIO
+import sys
 import weakref
 
 import git
@@ -63,6 +66,97 @@ def find_first_remote_branch(remotes: Sequence["Remote"], branch_name: str) -> "
     raise InvalidGitRepositoryError("Didn't find remote branch '%r' in any of the given remotes" % branch_name)
 
 
+def _is_dir_sep(char: str) -> bool:
+    return char == "/" or (sys.platform == "win32" and char == "\\")
+
+
+def _starts_with_dot_slash(url: str) -> bool:
+    return len(url) >= 2 and url[0] == "." and _is_dir_sep(url[1])
+
+
+def _starts_with_dot_dot_slash(url: str) -> bool:
+    return len(url) >= 3 and url[0:2] == ".." and _is_dir_sep(url[2])
+
+
+def _has_dos_drive_prefix(url: str) -> bool:
+    return sys.platform == "win32" and len(url) >= 2 and url[0].isalpha() and url[1] == ":"
+
+
+def _is_absolute_path(url: str) -> bool:
+    return (len(url) >= 1 and _is_dir_sep(url[0])) or _has_dos_drive_prefix(url)
+
+
+def _url_is_local_not_ssh(url: str) -> bool:
+    colon = url.find(":")
+    slash = url.find("/")
+    return colon == -1 or (slash != -1 and slash < colon) or _has_dos_drive_prefix(url)
+
+
+def _find_last_dir_sep(url: str) -> int:
+    for index in range(len(url) - 1, -1, -1):
+        if _is_dir_sep(url[index]):
+            return index
+    return -1
+
+
+def is_relative_url(url: str) -> bool:
+    """:return: ``True`` if `url` starts with ``./`` or ``../``, which git resolves
+    against the superproject's remote rather than treating as a path"""
+    return _starts_with_dot_slash(url) or _starts_with_dot_dot_slash(url)
+
+
+def resolve_relative_url(remote_url: str, url: str) -> str:
+    """Resolve a submodule URL that starts with ``./`` or ``../`` against the
+    superproject's remote URL, the way git's ``relative_url`` does.
+
+    Git treats a relative submodule URL like a relative directory: each leading
+    ``../`` removes the last component of `remote_url`, whether that is a path
+    component, the host of a URL, or the host of an scp-like ``host:path`` address.
+    The remainder is appended with ``/``, or with ``:`` once an scp-like host was
+    reached. A URL that is not relative is returned unchanged.
+
+    :raise ValueError:
+        If more components are stripped than `remote_url` has, which git rejects with
+        "cannot strip one component off url".
+    """
+    if not is_relative_url(url):
+        return url
+
+    remote = remote_url
+    if remote and _is_dir_sep(remote[-1]):
+        remote = remote[:-1]
+
+    is_relative = _url_is_local_not_ssh(remote) and not _is_absolute_path(remote)
+    if is_relative and not _starts_with_dot_slash(remote) and not _starts_with_dot_dot_slash(remote):
+        remote = "./" + remote
+
+    colonsep = False
+    while url:
+        if _starts_with_dot_dot_slash(url):
+            url = url[3:]
+            separator = _find_last_dir_sep(remote)
+            if separator != -1:
+                remote = remote[:separator]
+            elif ":" in remote:
+                remote = remote[: remote.rfind(":")]
+                colonsep = True
+            elif is_relative or remote == ".":
+                raise ValueError("cannot strip one component off url %r" % remote)
+            else:
+                remote = "."
+        elif _starts_with_dot_slash(url):
+            url = url[2:]
+        else:
+            break
+
+    resolved = remote + (":" if colonsep else "/") + url
+    if url.endswith("/"):
+        resolved = resolved[:-1]
+    if _starts_with_dot_slash(resolved):
+        resolved = resolved[2:]
+    return resolved
+
+
 # } END utilities
 
 # { Classes

test/test_submodule.py

@@ -26,6 +26,7 @@
 )
 from git.objects.submodule.base import Submodule
 from git.objects.submodule.root import RootModule, RootUpdateProgress
+from git.objects.submodule.util import resolve_relative_url
 from git.repo.fun import find_submodule_git_dir, touch
 from git.util import HIDE_WINDOWS_KNOWN_ERRORS, cwd, join_path_native, to_native_path_linux
 
@@ -2416,3 +2417,65 @@ def test_submodule_update_relative_url(self, rwdir):
 
         has_module = any(sm.name == "module" for sm in cloned_repo.submodules)
         assert has_module, "Relative submodule was not updated properly"
+
+    def test_resolve_relative_url_like_git(self):
+        # Expected values are what `git submodule init` records for the same pair.
+        cases = [
+            ("ssh://host/org/super.git", "./sub", "ssh://host/org/super.git/sub"),
+            ("ssh://host/org/super.git", "../sub", "ssh://host/org/sub"),
+            ("ssh://host:22/org/super.git", "../sub", "ssh://host:22/org/sub"),
+            ("git@host:org/super.git", "./sub", "git@host:org/super.git/sub"),
+            ("git@host:org/super.git", "../sub", "git@host:org/sub"),
+            ("git@host:org/super.git", "../../sub", "git@host:sub"),
+            ("https://host/org/super.git", "./sub/", "https://host/org/super.git/sub"),
+            ("https://host/org/super.git/", "././sub", "https://host/org/super.git/sub"),
+            ("https://host/org/super.git", ".././sub", "https://host/org/sub"),
+            ("https://host/org/super.git", "../../../sub", "https://sub"),
+            ("file:///srv/git/super.git", "../sub", "file:///srv/git/sub"),
+            ("/srv/git/super", "../sub", "/srv/git/sub"),
+            ("/srv/git/super", "../../../../sub", "sub"),
+            ("//share/super", "../../sub", "//sub"),
+            ("foo/bar", "../sub", "foo/sub"),
+            ("./foo", "../sub", "sub"),
+            ("../foo/bar", "../sub", "../foo/sub"),
+            ("ssh://host/org/super.git", ".sub", ".sub"),
+            ("ssh://host/org/super.git", "https://other/sub", "https://other/sub"),
+        ]
+        for remote_url, url, expected in cases:
+            assert resolve_relative_url(remote_url, url) == expected, (remote_url, url)
+
+        for remote_url, url in [("foo", "../../sub"), ("git@host:super.git", "../../../sub"), ("../foo", "../../sub")]:
+            with self.assertRaisesRegex(ValueError, "cannot strip one component"):
+                resolve_relative_url(remote_url, url)
+
+    @with_rw_directory
+    def test_update_resolves_relative_url_against_remote(self, rwdir):
+        parent = git.Repo.init(osp.join(rwdir, "parent"))
+        parent.create_remote("origin", "ssh://git@host/org/super.git")
+        os.mkdir(osp.join(parent.working_tree_dir, "planted"))
+
+        for url, expected in [
+            ("./planted", "ssh://git@host/org/super.git/planted"),
+            ("../planted", "ssh://git@host/org/planted"),
+        ]:
+            submodule = Submodule(parent, Submodule.NULL_BIN_SHA, name="module", path="module", url=url)
+            with mock.patch.object(Submodule, "_clone_repo", side_effect=AssertionError("stop")) as clone_repo:
+                with pytest.raises(AssertionError, match="stop"):
+                    submodule.update(init=True)
+            assert clone_repo.call_args.args[1] == expected
+
+    @with_rw_directory
+    def test_clone_repo_resolves_relative_url_against_tracking_remote(self, rwdir):
+        parent = git.Repo.init(osp.join(rwdir, "parent"))
+        parent.index.commit("initial")
+        parent.create_remote("origin", "git@host:org/super.git")
+        parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/main"))
+
+        for url, expected in [
+            ("./planted", "git@host:org/super.git/planted"),
+            ("../planted", "git@host:org/planted"),
+        ]:
+            with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from:
+                with pytest.raises(AssertionError, match="stop"):
+                    Submodule._clone_repo(parent, url, "module", "module")
+            assert clone_from.call_args.args[0] == expected