gitpython-developers
diff --git a/‎git/objects/submodule/base.py‎
Lines changed: 35 additions & 14 deletions b/‎git/objects/submodule/base.py‎
Lines changed: 35 additions & 14 deletions
diff --git a/‎test/test_submodule.py‎
Lines changed: 165 additions & 0 deletions b/‎test/test_submodule.py‎
Lines changed: 165 additions & 0 deletions
@@ -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)):
 
@@ -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"""