Merge pull request #2335 from gitpython-developers/validate-submodule-path

fix(submodule): bind writes to the validated checkout

Author: Byron

SECURITY.md

@@ -9,6 +9,37 @@ Only the latest version of GitPython can receive security updates. If a vulnerab
 | 3.x.x   | :white_check_mark: |
 | < 3.0   | :x:                |
 
+## Threat Model
+
+GitPython inherits its threat model directly from Git. The trust boundaries in
+[Git's security documentation](https://github.com/git/git/blob/master/Documentation/git.adoc#security)
+apply to both the CLI and Gix backends:
+
+- **Repository content may be untrusted.** Cloning an untrusted repository and
+  inspecting its history are supported operations. A clone creates local
+  repository metadata without copying the source repository's configuration or
+  hooks. Tracked files, including `.gitmodules`, remain untrusted input.
+- **Local repository metadata must be trusted.** Running commands in an
+  attacker-supplied `.git` directory, bare repository, or surrounding worktree is
+  unsafe: configuration and hooks can execute commands. This includes repositories
+  received in archives or copied directories. Owning the extracted files or listing
+  a repository in `safe.directory` does not make its contents trustworthy.
+- **Repository paths are not a sandbox.** Git supports metadata and working trees
+  in [separate locations](https://github.com/git/git/blob/master/Documentation/git-clone.adoc),
+  including metadata reached through gitfiles or symlinks.
+  GitPython does not promise that all filesystem access stays within a checkout
+  or `.git` directory. Paths supplied by untrusted repository content must still
+  receive Git's intended protections.
+
+For an untrusted repository directory, Git recommends obtaining a clean copy with
+`git clone --no-local` before running other commands. Serving the source through
+`git-upload-pack` still has an attack surface; Git recommends using an
+unprivileged user for additional isolation. See
+[Git's `git-upload-pack` security guidance](https://github.com/git/git/blob/master/Documentation/git-upload-pack.adoc#security).
+
+Violations of these trust boundaries or bypasses of Git's intended protections
+remain reportable, including cases where Git itself is affected.
+
 ## Reporting a Vulnerability
 
 Please report private portions of a vulnerability to <https://github.com/gitpython-developers/GitPython/security/advisories/new>. Doing so helps to receive updates and collaborate on the matter, without disclosing it publicly right away.

doc/gix-backend.md

@@ -968,6 +968,18 @@ identify the historical fixture where needed.
 | GIX-33 | Bug (open); missing exact-name mode | With Git 2.54.0 and GixPython 0.3.0, `exact-refs.sh sha1 files symbolic` makes `refs/tags/REBASE_HEAD` a symbolic ref to another tag while the root `REBASE_HEAD` is absent. `Repository.try_find_reference("REBASE_HEAD")` returns that tag; native `_symbolic_ref` formerly returned its target, whereas `git symbolic-ref --quiet --no-recurse -- REBASE_HEAD` exits 1. GitPython's root-reference object read consequently returned a commit through Gix and raised `ValueError` through CLI. SHA-256 reproduces the same mismatch. | Expose an exact-name lookup mode, including root references, whose absence and error behavior matches Git. `_symbolic_ref()` compares the native reference's name with the requested bytes and selects CLI for a lookalike. `test_missing_root_reference_does_not_follow_symbolic_lookalike_tag` verifies parity and forced CLI fallback. This adapter guard does not close the upstream compatibility bug. |
 | GIX-34 | Bug (open) | With Git 2.54.0 and GixPython 0.3.0, an author, committer or tagger line with fields after its time zone, such as `A <a@example.com> 1112591163 +0200 1700000000 -0500`, decodes through `Commit.author()`/`committer()` and `Tag.tagger()` with the correct seconds but an offset of 0. Git reads only the first date and time zone after the last `>` (`split_ident_line`) and reports `1112591163 +0200` through `git show --format=%ad --date=raw` and `%(taggerdate:raw)`. `git fsck` reports `badTimezone` for such objects, but the default `transfer.fsckObjects=false` accepts them. The `trailing-date` read-only `commit.sh` fixture in `test_deserialization_reads_date_after_email` reproduces both object kinds. | Report the time zone Git reads for identity lines with trailing fields, or reject them consistently. `object_metadata()` selects CLI when the decoded signature does not reproduce the end of its raw header line. This adapter guard does not close the upstream compatibility bug. |
 
+GIX-14 also covers configured worktree symlink resolution. On macOS with
+GixPython 0.3.0, CPython 3.12.14 and Git 2.54.0, set a submodule's
+`core.worktree` to `parent/redirect`, where `redirect` links to `outside`.
+`Repository.workdir()` retains `parent/redirect`; Git's
+`rev-parse --show-toplevel` returns `outside`. A trailing `/worktree` reproduces
+the same difference for an intermediate symlink. Opening the canonical gitfile
+or reopening the canonical native Git directory does not resolve it.
+`test_submodule_writes_use_checked_worktree` exercises both layouts: submodule
+handles now pass the validated checkout as `GIT_WORK_TREE` to prevent redirected
+writes in either backend. General native discovery still needs Git-compatible
+configured-worktree resolution; this guard does not close GIX-14.
+
 The PR #2306 regression exposed an adapter ownership-check bypass in both
 backends: `Repo`'s automatic `GIT_DIR` made Git 2.54.0 treat hook execution as
 explicitly selecting the repository, so it ran a hook where an ordinary

git/objects/submodule/base.py

@@ -1727,7 +1727,8 @@ def module(self) -> "Repo":
         module_checkout_abspath = self.abspath
         self._module_abspath(self.repo, self.path, self.name)
         try:
-            repo = git.Repo(module_checkout_abspath)
+            # Keep core.worktree from redirecting writes past the validated checkout.
+            repo = git.Repo(module_checkout_abspath, _env={"GIT_WORK_TREE": os.fspath(module_checkout_abspath)})
             if repo != self.repo:
                 return repo
             # END handle repo uninitialized

test/test_submodule.py

@@ -949,6 +949,36 @@ def test_submodule_rejects_checkout_and_gitmodules_symlinks(configured_submodule
     assert (root / "moved").is_dir()
 
 
+@pytest.mark.parametrize("implementation", ["git", "gitpython"])
+@pytest.mark.parametrize("operation", ["update", "remove"])
+@pytest.mark.parametrize("kind", ["leaf", "intermediate"])
+def test_submodule_writes_use_checked_worktree(configured_submodule, implementation, operation, kind):
+    """A configured worktree must not redirect writes past the checked checkout."""
+    sm = configured_submodule
+    root = Path(sm.repo.working_tree_dir)
+    outside = root.parent / "outside"
+    if kind == "intermediate":
+        outside /= "worktree"
+    before = {path.name: path.read_bytes() for path in outside.iterdir()}
+    if implementation == "git":
+        if operation == "update":
+            sm.repo.git.submodule("update", "--force", "--no-fetch", "--", sm.path)
+        else:
+            sm.repo.git.submodule("deinit", "--force", "--", sm.path)
+    elif operation == "update":
+        sm.update(force=True, no_fetch=True)
+    else:
+        sm.remove(configuration=False)
+
+    assert outside.is_dir()
+    assert {path.name: path.read_bytes() for path in outside.iterdir()} == before
+    assert (root / "redirect").is_symlink()
+    if operation == "update":
+        assert (root / "module/file").read_text() == "updated"
+    else:
+        assert not (root / "module/file").exists()
+
+
 def test_add_closes_checkout_processes(movable_submodule, monkeypatch):
     """Adding a submodule must not leave a child process holding its checkout open."""
     # Native reads have no persistent process; keep exercising the CLI fallback.

tests/fixtures/submodule-symlinks.sh

@@ -10,6 +10,25 @@ relative_target=${args[6]}
 modules="$root/parent/.git/modules"
 target="$root/outside"
 case "$scenario" in
+    test_submodule_writes_use_checked_worktree)
+        if test "${args[5]}" = update; then
+            printf updated > parent/module/file
+            git -C parent/module add file
+            git -C parent/module commit -qm 'Advance submodule'
+            git -C parent add module
+            git -C parent commit -qm 'Record updated submodule'
+            git -C parent/module reset -q --hard HEAD^
+        fi
+        mkdir "$target"
+        ln -s "$target" parent/redirect
+        worktree="$root/parent/redirect"
+        if test "$kind" = intermediate; then
+            mkdir "$target/worktree"
+            worktree="$worktree/worktree"
+        fi
+        printf content > "$worktree/file"
+        git -C parent/module config core.worktree "$worktree"
+        ;;
     test_move_leaf_symlink_compatibility)
         case "$kind" in
             empty|nonempty) mkdir "$target" ;;