Repository navigation
fix(submodule): validate destinations before mutation - #2264
Byron merged 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1 · 2
· 1
LPT[0-9] will incorrectly reject LPT0, which is not a Win32 reserved device name (reserved are… · New
os.path.samefile(path, directory) can raise OSError/FileNotFoundError/PermissionError… · New
The new changelog entry mixes prose and bare URLs without consistent reStructuredText structure,… · New
This PR hardens submodule handling by validating checkout and metadata destinations before any filesystem mutation, aligning behavior more closely with Git’s path/index validation to prevent unsafe paths and nested metadata issues.
Changes:
- Add comprehensive regression tests ensuring unsafe paths are rejected without side effects (no clone attempts / no filesystem writes).
- Enforce additional path validation in submodule name/path handling, including Windows-specific restrictions and Git metadata alias detection.
- Document the security fix in the changelog for the 3.2.1 release.
| File | Description |
|---|---|
| test/test_submodule.py | Adds regression coverage for rejecting unsafe checkout/metadata destinations before mutation across multiple operations. |
| git/objects/submodule/base.py | Adds Windows path validation, .git-alias checks via filesystem identity, and nested-metadata rejection logic in clone/move/rename/module flows. |
| doc/source/changes.rst | Adds a 3.2.1 changelog entry referencing the security advisory and release notes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Windows LPT0 validation and release version metadata are inconsistent with the added tests and changelog.
Review effort: Balanced
Findings: 1 · 1
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Metadata-alias and post-clone exception paths remain unsafe, and the release version is inconsistent.
Review effort: Balanced
Findings: 2 · 2
Sorry, something went wrong.
<!-- 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>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Windows validation still permits LPT0, contrary to the referenced Git implementation.
Review effort: Balanced
Findings: None
Sorry, something went wrong.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by Codex.
Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Submodule destinations could fail index validation after cloning had already changed the filesystem. Validate checkout paths before mutation across creation, initialization, direct cloning, and moves, and validate metadata destinations before creation, reuse, and renaming.
Advisory summary
GHSA-83vg-56qc-22m7: high severity; pip package Gitpython; affected range <= 3.2.0. No patched version or CVE is currently assigned. The advisory is a draft; this description omits its exploit details.
Git reference
Inspected Git at d38352cd43ab9745686d697872408bc3249a153f, including read-cache.c::verify_path_internal, NTFS/HFS path recognition, compat/mingw.c::is_valid_win32_path, and submodule.c::validate_submodule_git_dir. Git's t7450-bad-git-dotfiles.sh covers nested metadata; t7406-submodule-update.sh covers checkout symlinks. Regression tests also compare metadata aliases with native Git index validation using both filesystem protections.
Validation
Regression tests first reproduced writes before rejection and acceptance of nested metadata. Coverage includes all 16 HFS ignored characters, NTFS aliases, Windows reserved names and invalid characters, separate Git directories, relative and absolute destinations, reuse after deinitialization, moves and renames, and metadata nesting introduced during cloning. The fixtures use harmless file content.
The required Codex commit review was attempted once for faf32c2b, but the CLI rejected its configured gpt-6.1-sol model with this ChatGPT account before reviewing code. Automated commit review is therefore unavailable for this commit.
The follow-up commit 69d4702d passed its single Codex review using the supported gpt-5.5 model, with no findings.