Reject submodule move destinations through intermediate symlinks

Submodule.move() checked lexical containment but did not validate intermediate destination components before filesystem and repository updates. GHSA-gq48-pqfc-9p58 identifies the resulting checkout-path boundary violation. The new regression failed before the fix because move() returned successfully.

Share the existing abspath component walk with move() and validate the normalized destination before any mutation, including configuration-only and module-only calls. Preserve the no-op early return and existing final-component symlink handling; abspath still rejects every symlink component. This addresses pre-existing links, not concurrent directory replacement races.

The Git reference checkout at 1630431f326e15fcde608827b5ff38422528eb59 uses has_symlink_leading_path() in builtin/mv.c and tests rejection without index changes in t/t7001-mv.sh. The fix follows that intermediate-component rule while retaining GitPython leaf-link compatibility.

Validation: the 30 new parameterized cases pass, covering relative and absolute destinations and link targets, internal and dangling links, all move flag combinations, unchanged repository state after rejection, ordinary and no-op moves, and leaf-link compatibility. The complete test/test_submodule.py suite passes: 75 passed, 3 skipped, 1 xfailed. Test-process commit.gpgsign=false avoids sandbox GPG failures. Ruff lint and format checks, mypy (45 source files), and git diff --check pass.

Author: codex

git/objects/submodule/base.py

@@ -419,11 +419,20 @@ def abspath(self) -> PathLike:
         root = self.repo.working_tree_dir
         if root is None:
             return super().abspath
-        path = root
-        for component in os.fspath(self._to_relative_path(self.repo, self.path)).split("/"):
+        return self._checkout_abspath(self._to_relative_path(self.repo, self.path))
+
+    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:
+            raise NotADirectoryError("Submodules require a working tree")
+        components = os.fspath(relative_path).split("/")
+        for index, component in enumerate(components):
             path = 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" % self.path)
+                raise ValueError("Submodule checkout path %r contains a symbolic link" % relative_path)
         return path
 
     @classmethod
@@ -1039,7 +1048,8 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
             self
 
         :raise ValueError:
-            If the module path existed and was not empty, or was a file.
+            If the module path existed and was not empty, was a file, or had a
+            symbolic link in an intermediate component.
 
         :note:
             Currently the method is not atomic, and it could leave the repository in an
@@ -1057,7 +1067,7 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
             return self
         # END handle no change
 
-        module_checkout_abspath = join_path_native(str(self.repo.working_tree_dir), 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)
         # END handle target files

test/test_submodule.py

@@ -51,6 +51,115 @@ def _patch_git_config(name, value):
         yield
 
 
+@pytest.fixture
+def movable_submodule(tmp_path):
+    with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent:
+        (tmp_path / "source" / "file").write_text("content", encoding="utf-8")
+        source.index.add(["file"])
+        source.index.commit("Create source")
+        with _patch_git_config("protocol.file.allow", "always"):
+            submodule = parent.create_submodule("logical-name", "module", source.working_tree_dir)
+        parent.index.commit("Create submodule")
+        yield submodule
+
+
+def _move_snapshot(submodule):
+    parent = submodule.repo
+    with submodule.module() as module:
+        config = Path(module.git_dir, "config").read_bytes()
+    return (
+        Path(parent.index.path).read_bytes(),
+        Path(parent.working_tree_dir, ".gitmodules").read_bytes(),
+        Path(submodule.abspath, ".git").read_bytes(),
+        config,
+        submodule.path,
+    )
+
+
+@pytest.mark.parametrize("target_kind", ["relative", "absolute", "internal", "dangling"])
+@pytest.mark.parametrize("configuration,module", [(True, True), (False, True), (True, False)])
+@pytest.mark.parametrize("absolute_path", [False, True])
+def test_move_rejects_intermediate_symlink(
+    movable_submodule, tmp_path, target_kind, configuration, module, absolute_path
+):
+    submodule = movable_submodule
+    parent = submodule.repo
+    root = Path(parent.working_tree_dir)
+    target = root / "target" if target_kind == "internal" else tmp_path / "outside"
+    if target_kind != "dangling":
+        target.mkdir()
+    (root / "nested").mkdir()
+    link = root / "nested" / "link"
+    link.symlink_to(
+        target if target_kind == "absolute" else os.path.relpath(target, link.parent), target_is_directory=True
+    )
+    parent.index.add(["nested/link"])
+    parent.index.commit("Record layout")
+    tree = parent.git.write_tree()
+    before = _move_snapshot(submodule)
+    destination = root / "nested/link/new/moved" if absolute_path else "nested/link/new/moved"
+
+    with pytest.raises(ValueError, match="contains a symbolic link"):
+        submodule.move(destination, configuration=configuration, module=module)
+
+    assert _move_snapshot(submodule) == before
+    assert parent.git.write_tree() == tree
+    assert Path(submodule.abspath, "file").read_text(encoding="utf-8") == "content"
+    if target_kind == "dangling":
+        assert not target.exists()
+    else:
+        assert list(target.iterdir()) == []
+
+
+@pytest.mark.parametrize("absolute_path", [False, True])
+def test_move_normal_destination(movable_submodule, absolute_path):
+    submodule = movable_submodule
+    root = Path(submodule.repo.working_tree_dir)
+    destination = root / "nested/moved" if absolute_path else "nested/moved"
+    assert submodule.move(destination) is submodule
+    assert Path(submodule.abspath, "file").read_text(encoding="utf-8") == "content"
+    assert not (root / "module").exists()
+    assert submodule.path == "nested/moved"
+    submodule.repo.git.write_tree()
+    before = _move_snapshot(submodule)
+    assert submodule.move(destination) is submodule
+    assert _move_snapshot(submodule) == before
+
+
+@pytest.mark.parametrize("kind", ["empty", "nonempty", "file", "dangling"])
+def test_move_leaf_symlink_compatibility(movable_submodule, tmp_path, kind):
+    submodule = movable_submodule
+    root = Path(submodule.repo.working_tree_dir)
+    target = tmp_path / "outside"
+    if kind in ("empty", "nonempty"):
+        target.mkdir()
+    if kind == "nonempty":
+        (target / "keep").write_text("keep", encoding="utf-8")
+    if kind == "file":
+        target.write_text("keep", encoding="utf-8")
+    destination = root / "destination"
+    destination.symlink_to(target, target_is_directory=kind != "file")
+    with pytest.raises(ValueError, match="contains a symbolic link"):
+        Submodule(submodule.repo, Submodule.NULL_BIN_SHA, name="unused", path="destination").abspath
+    before = _move_snapshot(submodule)
+    if kind == "empty":
+        assert submodule.move("destination") is submodule
+        assert not destination.is_symlink()
+        assert (destination / "file").is_file()
+        assert list(target.iterdir()) == []
+    else:
+        with pytest.raises(OSError if kind == "dangling" else ValueError):
+            submodule.move("destination")
+        assert _move_snapshot(submodule) == before
+        assert destination.is_symlink()
+        if kind == "nonempty":
+            assert (target / "keep").read_text(encoding="utf-8") == "keep"
+        elif kind == "file":
+            assert target.read_text(encoding="utf-8") == "keep"
+        else:
+            assert not target.exists()
+
+
 class TestRootProgress(RootUpdateProgress):
     """Just prints messages, for now without checking the correctness of the states"""