fix: Validate submodule checkout and metadata paths before mutation

Submodule.add() could clone through a checkout symlink after module_exists()
swallowed the validation error. Metadata paths had a similar gap: locally
planted symlinks under .git/modules could redirect cloning, reconnecting,
renaming, updating, or removing a submodule. Some failures were detected only
after changing configuration or moving or removing checkout directories.

Reuse the checkout component check for metadata paths and validate checkout
paths in the shared clone helper, including legacy embedded repositories.
Check .gitfiles, submodule configuration files, and the actual repository
path named by a gitfile, which can differ from .git/modules/<name>. Reject
symlinked .gitmodules files as well. Preflight move and rename sources and
destinations before mutation, including the implicit metadata rename when
a default-named submodule moves. Keep module_exists()'s boolean contract
and the existing supported replacement of a leaf symlink during a move.

Add 56 regression cases covering checkout and metadata links, dangling
links, redirected gitfiles, legacy clone layouts, and rejected operations
preserving external targets, configuration, the index, and an empty move
destination. The initial 36 cases reproduced failures before the fix.
These checks reject existing symlinks; they do not prevent concurrent
filesystem replacement between validation and use.

Validation: the submodule and diff suites passed with 157 passed, 3 skipped,
and 1 expected failure. Test-process Git settings disabled commit signing,
allowed local file transport, and selected the master default branch to
match fixture assumptions. Ruff lint and formatting, mypy for the changed
module, and git diff --check also passed.

Author: codex

git/objects/submodule/base.py

@@ -255,7 +255,7 @@ def _config_parser(
         # END handle parent_commit
         fp_module: Union[str, BytesIO]
         if not repo.bare and parent_matches_head and repo.working_tree_dir:
-            fp_module = osp.join(repo.working_tree_dir, cls.k_modules_file)
+            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)
         else:
             assert parent_commit is not None, "need valid parent_commit in bare repositories"
             try:
@@ -320,9 +320,9 @@ def _validated_name(cls, name: str) -> str:
     def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> PathLike:
         name = cls._validated_name(name)
         if cls._need_gitfile_submodules(parent_repo.git):
-            return osp.join(parent_repo.git_dir, "modules", name)
+            return cls._checked_abspath(parent_repo.git_dir, osp.join("modules", name))
         if parent_repo.working_tree_dir:
-            return osp.join(parent_repo.working_tree_dir, path)
+            return cls._checked_abspath(parent_repo.working_tree_dir, cls._to_relative_path(parent_repo, path))
         raise NotADirectoryError()
 
     @classmethod
@@ -361,8 +361,13 @@ def _clone_repo(
         :param kwargs:
             Additional arguments given to :manpage:`git-clone(1)`.
         """
+        path = cls._to_relative_path(repo, path)
+        if repo.working_tree_dir is None:
+            raise NotADirectoryError("Submodules require a working tree")
+        module_checkout_path = cls._checked_abspath(repo.working_tree_dir, path)
         module_abspath = cls._module_abspath(repo, path, name)
-        module_checkout_path = module_abspath
+        cls._checked_abspath(module_checkout_path, ".git")
+        cls._checked_abspath(module_abspath, "config")
         if cls._need_gitfile_submodules(repo.git):
             if not allow_unsafe_options:
                 Git.check_unsafe_options(Git._option_candidates([], kwargs), repo.unsafe_git_clone_options)
@@ -377,7 +382,6 @@ def _clone_repo(
             module_abspath_dir = osp.dirname(module_abspath)
             if not osp.isdir(module_abspath_dir):
                 os.makedirs(module_abspath_dir)
-            module_checkout_path = osp.join(repo.working_tree_dir, path)  # type: ignore[arg-type]
 
         if url.startswith("../"):
             remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name
@@ -423,16 +427,23 @@ def abspath(self) -> PathLike:
 
     def _checkout_abspath(self, relative_path: PathLike, allow_final_symlink: bool = False) -> PathLike:
         """Check a checkout path already normalized by :meth:`_to_relative_path`."""
-        path = self.repo.working_tree_dir
-        if path is None:
+        return self._checked_abspath(self.repo.working_tree_dir, relative_path, allow_final_symlink)
+
+    @classmethod
+    def _checked_abspath(
+        cls, root: Union[PathLike, None], relative_path: PathLike, allow_final_symlink: bool = False
+    ) -> str:
+        """Reject symlinks below a trusted root before accessing submodule paths."""
+        if root is None:
             raise NotADirectoryError("Submodules require a working tree")
-        components = os.fspath(relative_path).split("/")
+        path = os.fspath(root)
+        components = to_native_path_linux(relative_path).split("/")
         for index, component in enumerate(components):
-            path = join_path_native(path, component)
+            path = os.fspath(join_path_native(path, component))
             if allow_final_symlink and index == len(components) - 1:
                 break
             if osp.islink(path):
-                raise ValueError("Submodule checkout path %r contains a symbolic link" % relative_path)
+                raise ValueError("Submodule path %r contains a symbolic link" % relative_path)
         return path
 
     @classmethod
@@ -458,14 +469,15 @@ def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_ab
         :param module_abspath:
             Absolute path to the bare repository.
         """
-        git_file = osp.join(working_tree_dir, ".git")
+        git_file = cls._checked_abspath(working_tree_dir, ".git")
+        module_config = cls._checked_abspath(module_abspath, "config")
         rela_path = osp.relpath(module_abspath, start=working_tree_dir)
         if sys.platform == "win32" and osp.isfile(git_file):
             os.remove(git_file)
         with open(git_file, "wb") as fp:
             fp.write(("gitdir: %s" % rela_path).encode(defenc))
 
-        with GitConfigParser(osp.join(module_abspath, "config"), read_only=False, merge_includes=False) as writer:
+        with GitConfigParser(module_config, read_only=False, merge_includes=False) as writer:
             writer.set_value(
                 "core",
                 "worktree",
@@ -576,6 +588,9 @@ def add(
             name,
             url="invalid-temporary",
         )
+        cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)
+        sm._checkout_abspath(path)
+        cls._module_abspath(repo, path, name)
         if sm.exists():
             # Reretrieve submodule from tree.
             try:
@@ -1067,6 +1082,17 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
             return self
         # END handle no change
 
+        if configuration:
+            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
+        # Validate the source and both metadata destinations before removing anything.
+        cur_path = self.abspath
+        module_abspath = self._module_abspath(self.repo, self.path, self.name)
+        try:
+            self.module().close()
+        except InvalidGitRepositoryError:
+            pass
+        if self.path == self.name:
+            self._module_abspath(self.repo, module_checkout_path, os.fspath(module_checkout_path))
         module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)
         if osp.isfile(module_checkout_abspath):
             raise ValueError("Cannot move repository onto a file: %s" % module_checkout_abspath)
@@ -1099,14 +1125,12 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
         # END handle module
 
         # Move the module into place if possible.
-        cur_path = self.abspath
         renamed_module = False
         if module and osp.exists(cur_path):
             os.renames(cur_path, module_checkout_abspath)
             renamed_module = True
 
             if osp.isfile(osp.join(module_checkout_abspath, ".git")):
-                module_abspath = self._module_abspath(self.repo, self.path, self.name)
                 self._write_git_file_and_module_config(module_checkout_abspath, module_abspath)
             # END handle git file rewrite
         # END move physical module
@@ -1201,6 +1225,8 @@ def remove(
         # END handle parameters
 
         self._validated_name(self.name)
+        if configuration:
+            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
         # Recursively remove children of this submodule.
         nc = 0
         for csm in self.children():
@@ -1460,6 +1486,9 @@ def rename(self, new_name: str) -> "Submodule":
 
         self._validated_name(self.name)
         self._validated_name(new_name)
+        destination_module_abspath = self._module_abspath(self.repo, self.path, new_name)
+        mod = self.module()
+        self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
 
         # .git/config
         with self.repo.config_writer() as pw:
@@ -1476,9 +1505,7 @@ def rename(self, new_name: str) -> "Submodule":
         self._name = new_name
 
         # .git/modules
-        mod = self.module()
         if mod.has_separate_working_tree():
-            destination_module_abspath = self._module_abspath(self.repo, self.path, new_name)
             source_dir = mod.git_dir
             # Let's be sure the submodule name is not so obviously tied to a directory.
             if str(destination_module_abspath).startswith(str(mod.git_dir)):
@@ -1510,9 +1537,20 @@ def module(self) -> "Repo":
         """
         self._validated_name(self.name)
         module_checkout_abspath = self.abspath
+        module_abspath = self._module_abspath(self.repo, self.path, self.name)
+        self._checked_abspath(module_abspath, "config")
+        self._checked_abspath(module_checkout_abspath, ".git")
         try:
             repo = git.Repo(module_checkout_abspath)
             if repo != self.repo:
+                # The gitfile can name a different repository than .git/modules/<name>.
+                # Validate its actual path too, including old-style embedded repositories.
+                try:
+                    root = osp.commonpath([self.repo.git_dir, repo.git_dir])
+                except ValueError:  # Separate Windows drives have no common path.
+                    root = osp.splitdrive(repo.git_dir)[0] + osp.sep
+                self._checked_abspath(root, osp.relpath(repo.git_dir, root))
+                self._checked_abspath(repo.git_dir, "config")
                 return repo
             # END handle repo uninitialized
         except (InvalidGitRepositoryError, NoSuchPathError) as e:

test/test_submodule.py

@@ -176,6 +176,117 @@ def test_move_leaf_symlink_compatibility(movable_submodule, tmp_path, kind):
             assert not target.exists()
 
 
+@pytest.mark.parametrize("leaf", [False, True])
+@pytest.mark.parametrize("dangling", [False, True])
+@pytest.mark.parametrize("operation", ["add", "clone"])
+@pytest.mark.parametrize("gitfile", [False, True])
+def test_clone_rejects_checkout_symlinks(movable_submodule, tmp_path, leaf, dangling, operation, gitfile):
+    sm = movable_submodule
+    root = Path(sm.repo.working_tree_dir)
+    target = tmp_path / "outside"
+    if not dangling:
+        target.mkdir()
+    (root / "link").symlink_to(target, target_is_directory=True)
+    path = "link" if leaf else "link/new/module"
+    before = _move_snapshot(sm)
+    with mock.patch.object(Submodule, "_need_gitfile_submodules", return_value=gitfile):
+        with pytest.raises(ValueError, match="contains a symbolic link"):
+            if operation == "add":
+                Submodule.add(sm.repo, "new", path, sm.url)
+            else:
+                Submodule._clone_repo(sm.repo, sm.url, path, "new")
+    assert _move_snapshot(sm) == before
+    assert not (Path(sm.repo.git_dir) / "modules/new").exists()
+    assert not target.exists() if dangling else list(target.iterdir()) == []
+
+
+@pytest.mark.parametrize("leaf", [False, True])
+@pytest.mark.parametrize("operation", ["add", "rename", "move", "reconnect"])
+def test_submodule_rejects_metadata_destination_symlinks(movable_submodule, tmp_path, leaf, operation):
+    sm = movable_submodule
+    target = tmp_path / "outside"
+    target.mkdir()
+    modules = Path(sm.repo.git_dir) / "modules"
+    (modules / "link").symlink_to(target, target_is_directory=True)
+    name = "link" if leaf else "link/new/module"
+    if operation == "move":
+        sm.rename(sm.path)  # Moving a default-named module also renames its metadata.
+    before = _move_snapshot(sm)
+    parent_config = Path(sm.repo.git_dir, "config").read_bytes()
+    with pytest.raises(ValueError, match="contains a symbolic link"):
+        if operation == "add":
+            Submodule.add(sm.repo, name, "new", sm.url)
+        elif operation == "rename":
+            sm.rename(name)
+        elif operation == "move":
+            sm.move(name)
+        else:
+            Submodule(sm.repo, sm.binsha, name=name, path="new", url=sm.url).update(init=True)
+    assert _move_snapshot(sm) == before
+    assert Path(sm.repo.git_dir, "config").read_bytes() == parent_config
+    assert list(target.iterdir()) == []
+    assert not Path(sm.repo.working_tree_dir, "new").exists()
+
+
+@pytest.mark.parametrize(
+    "link_kind", ["modules", "intermediate", "leaf", "gitfile", "config", "gitmodules", "alias", "checkout"]
+)
+@pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"])
+def test_submodule_rejects_existing_metadata_symlinks(movable_submodule, tmp_path, link_kind, operation):
+    sm = movable_submodule
+    sm.rename("nested/module")
+    root = Path(sm.repo.working_tree_dir)
+    modules = Path(sm.repo.git_dir) / "modules"
+    paths = {
+        "modules": modules,
+        "intermediate": modules / "nested",
+        "leaf": modules / "nested/module",
+        "gitfile": root / "module/.git",
+        "config": modules / "nested/module/config",
+        "gitmodules": root / ".gitmodules",
+        "alias": modules / "alias",
+        "checkout": root / "module",
+    }
+    path = paths[link_kind]
+    target = tmp_path / "outside"
+    if link_kind == "alias":
+        target = modules / "nested"
+        (root / "module/.git").write_text("gitdir: ../.git/modules/alias/module")
+    else:
+        path.rename(target)
+    path.symlink_to(target, target_is_directory=target.is_dir())
+    before = (
+        {p.relative_to(target): p.read_bytes() for p in target.rglob("*") if p.is_file()}
+        if target.is_dir()
+        else target.read_bytes()
+    )
+    config = Path(sm.repo.git_dir, "config").read_bytes()
+    index = Path(sm.repo.index.path).read_bytes()
+    gitmodules = (root / ".gitmodules").read_bytes()
+    (root / "moved").mkdir()
+    with pytest.raises(ValueError, match="contains a symbolic link"):
+        if operation == "update":
+            sm.update()
+        elif operation == "move":
+            sm.move("moved")
+        elif operation == "rename":
+            sm.rename("renamed")
+        else:
+            sm.remove(force=True)
+    after = (
+        {p.relative_to(target): p.read_bytes() for p in target.rglob("*") if p.is_file()}
+        if target.is_dir()
+        else target.read_bytes()
+    )
+    assert after == before
+    assert Path(sm.repo.git_dir, "config").read_bytes() == config
+    assert Path(sm.repo.index.path).read_bytes() == index
+    assert (root / ".gitmodules").read_bytes() == gitmodules
+    assert (root / "module/file").read_text() == "content"
+    assert path.is_symlink()
+    assert (root / "moved").is_dir()
+
+
 class TestRootProgress(RootUpdateProgress):
     """Just prints messages, for now without checking the correctness of the states"""