fix: Reject submodule move destinations through intermediate symlinks

Also, sloppy review of the tests which are assumpted to not make things worse.
<!-- agent -->
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.

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

Author: Byron

doc/source/changes.rst

@@ -2,6 +2,19 @@
 Changelog
 =========
 
+3.1.63
+======
+
+Security fixes for
+
+* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-gq48-pqfc-9p58
+
+If you can, also try and provide feedback on the upcoming v4 branch
+https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome.
+
+See the following for all changes.
+https://github.com/gitpython-developers/GitPython/releases/tag/3.1.63
+
 3.1.62
 ======
 

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,131 @@ def _patch_git_config(name, value):
         yield
 
 
+@pytest.fixture
+def movable_submodule(tmp_path):
+    """Create a committed local submodule whose logical name stays fixed when moved."""
+    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")
+        # Release clone handles before Windows moves the checkout.
+        submodule.module().close()
+        yield submodule
+
+
+def _move_snapshot(submodule):
+    """Capture index, configuration, and path state to detect side effects of rejected moves."""
+    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
+):
+    """Reject intermediate symlinks before changing repository state or their targets.
+
+    Cover relative and absolute destinations in every move mode, including links
+    within the repository and dangling links, which must also be rejected.
+    """
+    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):
+    """Allow ordinary relative and absolute moves, and make a repeated move a no-op."""
+    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):
+    """Preserve leaf-symlink replacement without modifying the external target.
+
+    Moving onto a link to an empty directory replaces the link; nonempty, file,
+    and dangling targets fail without changing repository state. Direct checkout
+    path access must still reject every leaf symlink.
+    """
+    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"""