fix(submodule): validate destinations before mutation

<!-- Byron -->

Pretty much a rubber-stamp. It won't be out there long as the replacement
with CLI + Gix is already on the way.

<!-- agent -->
Submodule checkout destinations could pass the containment check and be
rejected by the index only after cloning had changed the filesystem. This
addresses `GHSA-83vg-56qc-22m7` at the shared destination boundaries, including
initialization and moves as well as creation.

Reuse `_validate_repo_path` before checkout mutations to enforce portable
NTFS/HFS metadata-alias checks and invalid-path rejection. Validate Windows
filenames and submodule-name NULs before creating directories. Compare path
components with `Repo.git_dir` and `Repo.common_dir` by filesystem identity,
so separately named metadata directories and their aliases are protected too.

Reject metadata destinations nested inside another submodule's Git directory
before cloning, reuse, or renaming. Repeat the check after cloning and disable
a clone that became nested. Preflight implicit metadata renames during moves,
while preserving supported metadata symlinks and relocation of a submodule's
own metadata directory.

Git reference: `d38352cd43ab9745686d697872408bc3249a153f`, particularly
`read-cache.c::verify_path_internal`, NTFS/HFS recognition,
`compat/mingw.c::is_valid_win32_path`, and
`submodule.c::validate_submodule_git_dir`. Related Git tests are in
`t/t7450-bad-git-dotfiles.sh` and `t/t7406-submodule-update.sh`.

Regression tests first demonstrated writes before rejection and acceptance
of nested and separately named metadata destinations. Tests use harmless
file content and compare portable aliases with native Git index validation.
Coverage includes all 16 HFS ignored characters, Windows filename rules,
relative and absolute paths, metadata reuse, and nesting during cloning.

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

Author: Byron

doc/source/changes.rst

@@ -2,6 +2,19 @@
 Changelog
 =========
 
+3.2.1
+=====
+
+Security fixes for
+
+* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-83vg-56qc-22m7
+
+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.2.1
+
 3.2.0
 =====
 

git/objects/submodule/base.py

@@ -8,6 +8,7 @@
 import ntpath
 import os
 import os.path as osp
+import re
 import shlex
 import stat
 import sys
@@ -47,6 +48,7 @@
     IterableList,
     RemoteProgress,
     _to_relative_path,
+    _validate_repo_path,
     join_path_native,
     rmtree,
     to_native_path_linux,
@@ -305,20 +307,55 @@ def _config_parser_constrained(self, read_only: bool) -> SectionConstraint:
     def _validated_name(cls, name: str) -> str:
         if (
             not name
+            or "\0" in name
             or name.startswith(("/", "\\"))
             or ntpath.splitdrive(name)[0]
             or ".." in name.replace("\\", "/").split("/")
         ):
             raise ValueError("Invalid submodule name %r" % name)
+        cls._validate_windows_path(name)
         return name
 
+    @staticmethod
+    def _validate_windows_path(path: PathLike) -> None:
+        """Apply Git for Windows' filename checks before creating directories."""
+        if sys.platform == "win32":
+            for component in ntpath.splitdrive(os.fspath(path))[1].replace("\\", "/").split("/"):
+                if component in (".", ".."):
+                    continue
+                stem = component.split(".", 1)[0].rstrip(" ").upper()
+                if (
+                    component.endswith((" ", "."))
+                    or any(ord(char) < 32 or char in '<>:"|?*' for char in component)
+                    or re.fullmatch(r"CON(?:IN\$|OUT\$)?|PRN|AUX|NUL|COM[1-9]|LPT[1-9]", stem)
+                ):
+                    raise ValueError("Invalid submodule path on Windows: %r" % path)
+
     @classmethod
-    def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> PathLike:
+    def _module_abspath(
+        cls, parent_repo: "Repo", path: PathLike, name: str, *, moving_from: Union[PathLike, None] = None
+    ) -> PathLike:
+        """Reject nested Git directories, allowing the source of a pending rename."""
+        from git.repo.fun import is_git_dir
+
         name = cls._validated_name(name)
         if cls._need_gitfile_submodules(parent_repo.git):
+            directory = osp.join(parent_repo.git_dir, "modules")
+            for component in to_native_path_linux(name).split("/")[:-1]:
+                directory = osp.join(directory, component)
+                if is_git_dir(directory) and (
+                    moving_from is None or Path(directory).resolve() != Path(moving_from).resolve()
+                ):
+                    raise ValueError(
+                        "Submodule metadata for %r is inside another Git directory: %r" % (name, directory)
+                    )
             return osp.join(parent_repo.git_dir, "modules", name)
         if parent_repo.working_tree_dir:
-            return cls._checked_abspath(parent_repo.working_tree_dir, cls._to_relative_path(parent_repo, path))
+            return cls._checked_abspath(
+                parent_repo.working_tree_dir,
+                cls._to_relative_path(parent_repo, path),
+                git_dirs=(parent_repo.git_dir, parent_repo.common_dir),
+            )
         raise NotADirectoryError()
 
     @classmethod
@@ -360,7 +397,9 @@ def _clone_repo(
         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_checkout_path = cls._checked_abspath(
+            repo.working_tree_dir, path, git_dirs=(repo.git_dir, repo.common_dir)
+        )
         module_abspath = cls._module_abspath(repo, path, name)
         if cls._need_gitfile_submodules(repo.git):
             if not allow_unsafe_options:
@@ -402,6 +441,16 @@ def _clone_repo(
             **kwargs,
         )
         if cls._need_gitfile_submodules(repo.git):
+            # A concurrent clone may have turned a leading directory into a repository.
+            try:
+                cls._module_abspath(repo, path, name)
+            except ValueError:
+                clone.close()
+                try:
+                    os.remove(osp.join(clone.git_dir, "HEAD"))
+                except FileNotFoundError:
+                    pass
+                raise
             cls._write_git_file_and_module_config(module_checkout_path, module_abspath)
 
         return clone
@@ -411,8 +460,9 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike:
         """:return: A path guaranteed to be relative to the given parent repository
 
         :raise ValueError:
-            If path is not contained in the parent repository's working tree.
+            If path is outside the working tree or is unsafe as a submodule checkout.
         """
+        cls._validate_windows_path(path)
         if parent_repo.working_tree_dir:
             path = _to_relative_path(parent_repo.working_tree_dir, path)
         else:
@@ -422,6 +472,7 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike:
         if not path or path == ".":
             raise ValueError("Submodule checkout path must not be the repository root")
 
+        _validate_repo_path(path)
         return path
 
     @property
@@ -433,22 +484,35 @@ 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`."""
-        return self._checked_abspath(self.repo.working_tree_dir, relative_path, allow_final_symlink)
+        return self._checked_abspath(
+            self.repo.working_tree_dir,
+            relative_path,
+            allow_final_symlink,
+            git_dirs=(self.repo.git_dir, self.repo.common_dir),
+        )
 
     @classmethod
     def _checked_abspath(
-        cls, root: Union[PathLike, None], relative_path: PathLike, allow_final_symlink: bool = False
+        cls,
+        root: Union[PathLike, None],
+        relative_path: PathLike,
+        allow_final_symlink: bool = False,
+        *,
+        git_dirs: Sequence[PathLike] = (),
     ) -> str:
-        """Reject symlinks below a trusted root before accessing submodule paths."""
+        """Reject symlinks and checkout aliases of Git directories below a trusted root."""
         if root is None:
             raise NotADirectoryError("Submodules require a working tree")
         path = os.fspath(root)
+        metadata_dirs = set(git_dirs)
         components = to_native_path_linux(relative_path).split("/")
         for index, component in enumerate(components):
             path = os.fspath(join_path_native(path, component))
-            if allow_final_symlink and index == len(components) - 1:
-                break
+            if metadata_dirs and osp.exists(path) and any(osp.samefile(path, directory) for directory in metadata_dirs):
+                raise ValueError("Submodule checkout path aliases Git metadata: %r" % relative_path)
             if osp.islink(path):
+                if allow_final_symlink and index == len(components) - 1:
+                    break
                 raise ValueError("Submodule path %r contains a symbolic link" % relative_path)
         return path
 
@@ -1122,6 +1186,11 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
             self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
         # Validate the source before removing the destination.
         cur_path = self.abspath
+        module_abspath = self._module_abspath(self.repo, self.path, self.name)
+        if self.path == self.name:
+            self._module_abspath(
+                self.repo, module_checkout_path, os.fspath(module_checkout_path), moving_from=module_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)
@@ -1160,7 +1229,6 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
             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
@@ -1522,8 +1590,8 @@ 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()
+        destination_module_abspath = self._module_abspath(self.repo, self.path, new_name, moving_from=mod.git_dir)
         self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
 
         # .git/config
@@ -1573,6 +1641,7 @@ def module(self) -> "Repo":
         """
         self._validated_name(self.name)
         module_checkout_abspath = self.abspath
+        self._module_abspath(self.repo, self.path, self.name)
         try:
             repo = git.Repo(module_checkout_abspath)
             if repo != self.repo: