FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(submodule): validate destinations before mutation · gitpython-developers/GitPython@97eadb8 · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Commit 97eadb8

Browse files
Byron
and
codex
committed
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>
1 parent 2d7f566 commit 97eadb8

3 files changed

Lines changed: 318 additions & 11 deletions

File tree

‎doc/source/changes.rst‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,19 @@
22
Changelog
33
=========
44

5+
3.2.1
6+
=====
7+
8+
Security fixes for
9+
10+
* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-83vg-56qc-22m7
11+
12+
If you can, also try and provide feedback on the upcoming v4 branch
13+
https://github.com/gitpython-developers/GitPython/pull/2177 - patches welcome.
14+
15+
See the following for all changes.
16+
https://github.com/gitpython-developers/GitPython/releases/tag/3.2.1
17+
518
3.2.0
619
=====
720

‎git/objects/submodule/base.py‎

Lines changed: 80 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import ntpath
99
import os
1010
import os.path as osp
11+
import re
1112
import shlex
1213
import stat
1314
import sys
@@ -47,6 +48,7 @@
4748
IterableList,
4849
RemoteProgress,
4950
_to_relative_path,
51+
_validate_repo_path,
5052
join_path_native,
5153
rmtree,
5254
to_native_path_linux,
@@ -305,20 +307,55 @@ def _config_parser_constrained(self, read_only: bool) -> SectionConstraint:
305307
def _validated_name(cls, name: str) -> str:
306308
if (
307309
not name
310+
or "\0" in name
308311
or name.startswith(("/", "\\"))
309312
or ntpath.splitdrive(name)[0]
310313
or ".." in name.replace("\\", "/").split("/")
311314
):
312315
raise ValueError("Invalid submodule name %r" % name)
316+
cls._validate_windows_path(name)
313317
return name
314318

319+
@staticmethod
320+
def _validate_windows_path(path: PathLike) -> None:
321+
"""Apply Git for Windows' filename checks before creating directories."""
322+
if sys.platform == "win32":
323+
for component in ntpath.splitdrive(os.fspath(path))[1].replace("\\", "/").split("/"):
324+
if component in (".", ".."):
325+
continue
326+
stem = component.split(".", 1)[0].rstrip(" ").upper()
327+
if (
328+
component.endswith((" ", "."))
329+
or any(ord(char) < 32 or char in '<>:"|?*' for char in component)
330+
or re.fullmatch(r"CON(?:IN\$|OUT\$)?|PRN|AUX|NUL|COM[1-9]|LPT[1-9]", stem)
331+
):
332+
raise ValueError("Invalid submodule path on Windows: %r" % path)
333+
315334
@classmethod
316-
def _module_abspath(cls, parent_repo: "Repo", path: PathLike, name: str) -> PathLike:
335+
def _module_abspath(
336+
cls, parent_repo: "Repo", path: PathLike, name: str, *, moving_from: Union[PathLike, None] = None
337+
) -> PathLike:
338+
"""Reject nested Git directories, allowing the source of a pending rename."""
339+
from git.repo.fun import is_git_dir
340+
317341
name = cls._validated_name(name)
318342
if cls._need_gitfile_submodules(parent_repo.git):
343+
directory = osp.join(parent_repo.git_dir, "modules")
344+
for component in to_native_path_linux(name).split("/")[:-1]:
345+
directory = osp.join(directory, component)
346+
if is_git_dir(directory) and (
347+
moving_from is None or Path(directory).resolve() != Path(moving_from).resolve()
348+
):
349+
raise ValueError(
350+
"Submodule metadata for %r is inside another Git directory: %r" % (name, directory)
351+
)
319352
return osp.join(parent_repo.git_dir, "modules", name)
320353
if parent_repo.working_tree_dir:
321-
return cls._checked_abspath(parent_repo.working_tree_dir, cls._to_relative_path(parent_repo, path))
354+
return cls._checked_abspath(
355+
parent_repo.working_tree_dir,
356+
cls._to_relative_path(parent_repo, path),
357+
git_dirs=(parent_repo.git_dir, parent_repo.common_dir),
358+
)
322359
raise NotADirectoryError()
323360

324361
@classmethod
@@ -360,7 +397,9 @@ def _clone_repo(
360397
path = cls._to_relative_path(repo, path)
361398
if repo.working_tree_dir is None:
362399
raise NotADirectoryError("Submodules require a working tree")
363-
module_checkout_path = cls._checked_abspath(repo.working_tree_dir, path)
400+
module_checkout_path = cls._checked_abspath(
401+
repo.working_tree_dir, path, git_dirs=(repo.git_dir, repo.common_dir)
402+
)
364403
module_abspath = cls._module_abspath(repo, path, name)
365404
if cls._need_gitfile_submodules(repo.git):
366405
if not allow_unsafe_options:
@@ -402,6 +441,16 @@ def _clone_repo(
402441
**kwargs,
403442
)
404443
if cls._need_gitfile_submodules(repo.git):
444+
# A concurrent clone may have turned a leading directory into a repository.
445+
try:
446+
cls._module_abspath(repo, path, name)
447+
except ValueError:
448+
clone.close()
449+
try:
450+
os.remove(osp.join(clone.git_dir, "HEAD"))
451+
except FileNotFoundError:
452+
pass
453+
raise
405454
cls._write_git_file_and_module_config(module_checkout_path, module_abspath)
406455

407456
return clone
@@ -411,8 +460,9 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike:
411460
""":return: A path guaranteed to be relative to the given parent repository
412461
413462
:raise ValueError:
414-
If path is not contained in the parent repository's working tree.
463+
If path is outside the working tree or is unsafe as a submodule checkout.
415464
"""
465+
cls._validate_windows_path(path)
416466
if parent_repo.working_tree_dir:
417467
path = _to_relative_path(parent_repo.working_tree_dir, path)
418468
else:
@@ -422,6 +472,7 @@ def _to_relative_path(cls, parent_repo: "Repo", path: PathLike) -> PathLike:
422472
if not path or path == ".":
423473
raise ValueError("Submodule checkout path must not be the repository root")
424474

475+
_validate_repo_path(path)
425476
return path
426477

427478
@property
@@ -433,22 +484,35 @@ def abspath(self) -> PathLike:
433484

434485
def _checkout_abspath(self, relative_path: PathLike, allow_final_symlink: bool = False) -> PathLike:
435486
"""Check a checkout path already normalized by :meth:`_to_relative_path`."""
436-
return self._checked_abspath(self.repo.working_tree_dir, relative_path, allow_final_symlink)
487+
return self._checked_abspath(
488+
self.repo.working_tree_dir,
489+
relative_path,
490+
allow_final_symlink,
491+
git_dirs=(self.repo.git_dir, self.repo.common_dir),
492+
)
437493

438494
@classmethod
439495
def _checked_abspath(
440-
cls, root: Union[PathLike, None], relative_path: PathLike, allow_final_symlink: bool = False
496+
cls,
497+
root: Union[PathLike, None],
498+
relative_path: PathLike,
499+
allow_final_symlink: bool = False,
500+
*,
501+
git_dirs: Sequence[PathLike] = (),
441502
) -> str:
442-
"""Reject symlinks below a trusted root before accessing submodule paths."""
503+
"""Reject symlinks and checkout aliases of Git directories below a trusted root."""
443504
if root is None:
444505
raise NotADirectoryError("Submodules require a working tree")
445506
path = os.fspath(root)
507+
metadata_dirs = set(git_dirs)
446508
components = to_native_path_linux(relative_path).split("/")
447509
for index, component in enumerate(components):
448510
path = os.fspath(join_path_native(path, component))
449-
if allow_final_symlink and index == len(components) - 1:
450-
break
511+
if metadata_dirs and osp.exists(path) and any(osp.samefile(path, directory) for directory in metadata_dirs):
512+
raise ValueError("Submodule checkout path aliases Git metadata: %r" % relative_path)
451513
if osp.islink(path):
514+
if allow_final_symlink and index == len(components) - 1:
515+
break
452516
raise ValueError("Submodule path %r contains a symbolic link" % relative_path)
453517
return path
454518

@@ -1122,6 +1186,11 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
11221186
self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
11231187
# Validate the source before removing the destination.
11241188
cur_path = self.abspath
1189+
module_abspath = self._module_abspath(self.repo, self.path, self.name)
1190+
if self.path == self.name:
1191+
self._module_abspath(
1192+
self.repo, module_checkout_path, os.fspath(module_checkout_path), moving_from=module_abspath
1193+
)
11251194
module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)
11261195
if osp.isfile(module_checkout_abspath):
11271196
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 =
11601229
renamed_module = True
11611230

11621231
if osp.isfile(osp.join(module_checkout_abspath, ".git")):
1163-
module_abspath = self._module_abspath(self.repo, self.path, self.name)
11641232
self._write_git_file_and_module_config(module_checkout_abspath, module_abspath)
11651233
# END handle git file rewrite
11661234
# END move physical module
@@ -1522,8 +1590,8 @@ def rename(self, new_name: str) -> "Submodule":
15221590

15231591
self._validated_name(self.name)
15241592
self._validated_name(new_name)
1525-
destination_module_abspath = self._module_abspath(self.repo, self.path, new_name)
15261593
mod = self.module()
1594+
destination_module_abspath = self._module_abspath(self.repo, self.path, new_name, moving_from=mod.git_dir)
15271595
self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)
15281596

15291597
# .git/config
@@ -1573,6 +1641,7 @@ def module(self) -> "Repo":
15731641
"""
15741642
self._validated_name(self.name)
15751643
module_checkout_abspath = self.abspath
1644+
self._module_abspath(self.repo, self.path, self.name)
15761645
try:
15771646
repo = git.Repo(module_checkout_abspath)
15781647
if repo != self.repo:

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL