fix: Validate submodule checkout and metadata paths before mutation

A bit of a sloppy review, rubber-stamping the tests based on the assumption
that they are validating it's conforming to Git, probably also while increasing
coverage.

<!-- agent -->
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.

Assisted-by: GPT 6.0
Co-authored-by: GPT 6.0 <codex@openai.com>

Author: Byron

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:
@@ -322,7 +322,7 @@ def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> Path
         if cls._need_gitfile_submodules(parent_repo.git):
             return osp.join(parent_repo.git_dir, "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,11 @@ 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
         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 +380,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 +425,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 +467,18 @@ def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_ab
         :param module_abspath:
             Absolute path to the bare repository.
         """
+        # Git resolves metadata symlinks before interpreting core.worktree.
+        module_abspath = osp.realpath(module_abspath)
+        working_tree_dir = osp.realpath(working_tree_dir)
         git_file = osp.join(working_tree_dir, ".git")
+        module_config = osp.join(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 +589,8 @@ def add(
             name,
             url="invalid-temporary",
         )
+        cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)
+        sm._checkout_abspath(path)
         if sm.exists():
             # Reretrieve submodule from tree.
             try:
@@ -1067,6 +1082,10 @@ 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 before removing the destination.
+        cur_path = self.abspath
         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,7 +1118,6 @@ 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)
@@ -1201,6 +1219,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():
@@ -1219,7 +1239,7 @@ def remove(
         ################################
         if module and self.module_exists():
             mod = self.module()
-            git_dir = mod.git_dir
+            git_dir = osp.realpath(mod.git_dir)
             if force:
                 # Take the fast lane and just delete everything in our module path.
                 # TODO: If we run into permission problems, we have a highly
@@ -1460,6 +1480,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 +1499,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)):

test/test_submodule.py

@@ -176,6 +176,171 @@ 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):
+    """Reject checkout symlinks before add or clone creates metadata or touches the target.
+
+    Cover leaf and intermediate links, including dangling targets, with both
+    embedded and separate Git directories.
+    """
+    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("link_kind", ["gitmodules", "checkout"])
+@pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"])
+def test_submodule_rejects_checkout_and_gitmodules_symlinks(movable_submodule, tmp_path, link_kind, operation):
+    """Reject operations on symlinked checkouts or .gitmodules without side effects.
+
+    Update, move, rename, and forced removal must preserve the external target,
+    repository configuration, index, checkout, and any existing move destination.
+    """
+    sm = movable_submodule
+    sm.rename("nested/module")
+    root = Path(sm.repo.working_tree_dir)
+    path = root / (".gitmodules" if link_kind == "gitmodules" else "module")
+    target = tmp_path / "outside"
+    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()
+
+
+def test_submodule_allows_symlink_above_worktree(movable_submodule, tmp_path):
+    """Allow adding and moving submodules when the parent is opened through a symlink."""
+    sm = movable_submodule
+    alias = tmp_path / "alias"
+    alias.symlink_to(sm.repo.working_tree_dir, target_is_directory=True)
+    with git.Repo(alias) as parent:
+        added = Submodule.add(parent, "new", "new", sm.url)
+        added.move("moved")
+        with added.module() as module:
+            assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(added.abspath).resolve()
+        assert Path(added.abspath, "file").read_text() == "content"
+
+
+@pytest.mark.parametrize("operation", ["add", "reconnect", "rename"])
+def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_path, operation):
+    """Allow linked metadata destinations while keeping the checkout correctly connected.
+
+    Adding, reconnecting after deinit, and renaming may store metadata outside the
+    parent repository through a symlink under .git/modules, preserving that link.
+    """
+    sm = movable_submodule
+    root = Path(sm.repo.working_tree_dir)
+    outside = tmp_path / "outside"
+    outside.mkdir()
+    link = Path(sm.repo.git_dir) / "modules/link"
+    link.symlink_to(outside, target_is_directory=True)
+    if operation == "rename":
+        sm.rename("link/new")
+    else:
+        sm = Submodule.add(sm.repo, "link/new", "new", sm.url)
+        if operation == "reconnect":
+            sm.repo.index.commit("Add linked metadata submodule")
+            sm.repo.git.submodule("deinit", "--force", "new")
+            sm.update(init=True)
+    assert link.is_symlink()
+    assert (outside / "new/HEAD").is_file()
+    with sm.module() as module:
+        assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve()
+    assert Path(sm.abspath, "file").read_text() == "content"
+    assert (root / ".gitmodules").is_file()
+
+
+@pytest.mark.parametrize("kind", ["modules", "intermediate", "leaf", "gitfile", "config", "alias"])
+@pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"])
+def test_submodule_allows_existing_metadata_symlinks(movable_submodule, tmp_path, kind, operation):
+    """Keep submodule operations compatible with existing symlinks in Git metadata.
+
+    Cover linked metadata directories, gitfiles, configs, and internal aliases.
+    Update, move, and rename must retain a usable checkout; forced removal must
+    still remove it.
+    """
+    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",
+        "alias": modules / "alias",
+    }
+    link = paths[kind]
+    target = tmp_path / "outside"
+    if kind == "alias":
+        target = modules / "nested"
+        (root / "module/.git").write_text("gitdir: ../.git/modules/alias/module")
+    else:
+        link.rename(target)
+    link.symlink_to(target, target_is_directory=target.is_dir())
+    # Relocating metadata changes the base of a relative core.worktree setting.
+    sm.repo.git.config("--file", str(modules / "nested/module/config"), "core.worktree", str(root / "module"))
+    assert sm.module_exists()
+    if operation == "remove":
+        sm.remove(force=True)
+        assert not (root / "module").exists()
+        return
+    if operation == "update":
+        sm.update()
+    elif operation == "move":
+        sm.move("moved")
+    else:
+        sm.rename("renamed")
+    with sm.module() as module:
+        assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve()
+    assert Path(sm.abspath, "file").read_text() == "content"
+
+
 class TestRootProgress(RootUpdateProgress):
     """Just prints messages, for now without checking the correctness of the states"""