Skip to content

Commit 75a9276

Browse files
committed
fix(index): check repository ownership before commit hooks
Port PR #2306's regression against direct `git commit` and `git hook` invocations. Before this fix, `Repo`'s automatic `GIT_DIR` binding made Git treat the repository as explicitly selected: `IndexFile.commit()` ran its pre-commit hook even when ordinary Git rejected dubious ownership. The regression fails on both backends without the fix. Retain the original discovery location and run `git rev-parse --git-dir` before executing any commit hook, removing only the automatically bound environment values. Remembering discovery supports external `core.worktree` locations while matching `safe.directory` on ordinary, bare, linked and separate-directory repositories. Explicit environment or command-line repository selections retain Git's behavior, including relative paths and consumption of one-shot options after failures. Validate NUL arguments before starting either process. The fixture tests verify rejection without hook side effects, Git's safe-directory exemptions, and explicit repository-selection behavior. Update the native-backend coverage test and GIX-23 documentation to require two CLI processes per valid hook request, including missing hooks. No independent ownership policy or native hook implementation is introduced. Validation: before the final option-preservation refinements, the full index, repository and backend modules passed (CLI: 236 tests, 106 skips; Gix: 338 tests, 4 skips; each with 19 passing subtests). Final focused hook coverage passes with CLI (12 tests, 2 skips) and Gix (14 tests), each with 9 passing subtests. A separate review independently reran the ownership and repository-selection regressions on both backends. `ruff check`, targeted `mypy`, fixture `bash -n` and `git diff --check` pass. Windows and Linux were not run locally.
1 parent beadcb8 commit 75a9276

6 files changed

Lines changed: 160 additions & 8 deletions

File tree

‎doc/gix-backend.md‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,7 @@ identify the historical fixture where needed.
915915
| GIX-20 | Bug; missing API | `Target.Symbolic(name)` validates full names but rejects standalone names accepted by `git check-ref-format --allow-onelevel`. Four of 430 audited calls differed: `refs` twice, `hellothere`, and `valid_one_level_refname`. Further probes include digits, punctuation and Unicode (`1`, `A1`, `HEAD_1`, `A-B`, `A.B`, `Ä`). | Expose Git-compatible partial/one-level name validation, or correct the native restriction where appropriate. This is a binding/behavior shortcoming to revisit upstream; standalone names rejected natively retain CLI validation. |
916916
| GIX-21 | Bug; missing API | `Repository.committer()` preserves a process identity of ` Name. ` / ` email@example.invalid ` while `git var GIT_COMMITTER_IDENT` returns `Name.` / `email@example.invalid`. Native snapshot overrides for name, email and date work, but modifying the retained snapshot would leak per-command identity across threads. GixPython 0.1.0 has no general independent repository clone: Python copying fails and internal `RepoHandle::clone()` shares its `Arc` state. | Expose Git-compatible identity cleanup and a cheap independent native clone with isolated config, including per-command removal semantics. Reopening isolates state but repeats setup; `with_object_memory()` changes object-write behavior and is not a general clone. Keep `_checked_signature` validation and CLI fallback for cleanup/overrides. Never clean identity fields in Python or mutate shared snapshots/process environment to handle a command. |
917917
| GIX-22 | Missing API | The native Git directory is sufficient for the two fixed metadata leaves used here: `modules` and `COMMIT_EDITMSG`. Git's `path.c` applies no special relocation to either, including in linked worktrees. The adapter appends only these names to the canonical native Git directory. | No new binding is needed for these ordinary paths. Other `--git-path` names and symlinked metadata leaves retain CLI resolution; a general resolver must handle config/environment overrides, common/private storage and canonical targets. `Repository.modules_path()` means `.gitmodules`, not the `modules` storage directory. |
918-
| GIX-23 | Missing API | No hook lookup or execution API is bound in GixPython 0.1.0. The removed absence fast path read config through Gix but used Python `os.stat()` to declare success. | Bind native hook lookup with configured/default path, linked-worktree, missing/nonexecutable-hook and diagnostic semantics, plus execution where needed. Until then all managed hook calls use Git, including `--ignore-missing` no-ops. A Python filesystem check is not a Gix implementation. |
918+
| GIX-23 | Missing API | No hook lookup or execution API is bound in GixPython 0.1.0. The removed absence fast path read config through Gix but used Python `os.stat()` to declare success. | Bind native hook lookup with configured/default path, linked-worktree, missing/nonexecutable-hook and diagnostic semantics, plus execution where needed. Until then all managed hook calls use Git, including `--ignore-missing` no-ops, preceded by a Git discovery/ownership check that removes only `Repo`'s automatic environment bindings. Each valid hook request therefore makes two CLI calls. Explicit caller overrides retain Git's behavior. A Python filesystem check is not a Gix implementation. |
919919
| GIX-24 | Bug | In both object formats, `new_commit_as()` accepts a blob as the tree or a parent where `git commit-tree` rejects it. Tree-editor `upsert()`/`write()` accepts object IDs whose actual kinds disagree with blob/tree/gitlink modes; `git mktree --missing` rejects all three tested mismatches. `edit_references_as()` accepts a blob target under `refs/heads/`, rejected by `git update-ref`. | Provide checked commit/tree construction and reference edits, at least in Git-strict mode. Match Git's kind and direct-branch target validation before writing and under the required ref locks, including its distinct handling of symbolic aliases. Existing `_commit_tree`, `_write_tree` and `_update_ref` guards query Gix headers and select CLI on invalid inputs; they do not replace Gix writes with Python. |
920920
| GIX-25 | Bug; missing API | After `git symbolic-ref refs/heads/dangling refs/heads/missing`, `Repository.references().all()` enumerates the dangling name, while `git for-each-ref --format=%(refname)` omits it. Valid symbolic aliases must remain present. Both SHA-1 and SHA-256 probes reproduce this difference. | Expose Git-compatible enumeration with the same dangling-reference and diagnostic behavior, retaining the raw iterator for callers that need it. `_for_each_ref` forces Gix to resolve symbolic targets and falls back to Git on failure; keep that fallback until a compatible Gix API/mode is verified. |
921921
| GIX-26 | Bug; missing API | `dangerously_push_entry(IndexStat(), oid, 0, 0o120000, b".gitmodules")`, followed by `sort_entries()`, `verify_entries()` and `write()`, succeeds and retains the unsafe symlink. Protected Git `update-index -z --index-info` ignores it with exit 0 and `Ignoring path .gitmodules`; protected `read-tree` rejects it. The native fresh `index_from_tree` also rejects it. | A Git-strict checked index batch editor that validates names/modes, preserves metadata and reports rejected records. `verify_entries()` verifies ordering, not pathname safety. All private-index edits use Git with `core.protectHFS=true` and `core.protectNTFS=true`, followed by requested-entry readback; no Python dictionary reconstruction or native dangerous insertion remains. |
@@ -925,6 +925,17 @@ identify the historical fixture where needed.
925925
| GIX-30 | Bug (open) | GixPython 0.2.0 `Commit.decode()` reports `encoding=None` when `encoding ISO-8859-1` follows `x-extra value`; Git 2.54.0 selects `ISO-8859-1` and renders Latin-1 identity/message bytes correctly. Gix retains `(b"encoding", b"ISO-8859-1")` in `extra_headers`, but its encoding field disagrees with Git. The `late-latin1` read-only `commit.sh` fixture in `test_deserialization_declared_encoding` reproduces this and asserts the fallback. | Recognize encoding headers in Git-compatible order. `object_metadata()` conservatively retains CLI when an encoding appears in `extra_headers`; it does not select the header in Python. Keep this bug open until an upstream fix or compatible mode is verified. |
926926
| GIX-31 | Bug (open) | GixPython 0.2.0 `Commit.decode()` rejects commit headers containing a CR-only or tab-only line with `object parsing failed, at gix-object/src/commit/decode.rs:20`. Git 2.54.0 accepts both objects with `fsck --strict`, continues through their header block and renders the later Latin-1 declaration and message correctly. A space-only continuation line decodes natively but retains the late encoding declaration in `extra_headers` (GIX-30). `test_deserialization_headers_end_at_empty_line` reproduces all three using the `commit.sh whitespace-headers` fixtures and checks Git's projection. | Accept Git-compatible header lines and end headers only at an empty line. The existing `object_metadata()` native decoder diagnostics fallback retains Git for CR/tab inputs; the existing GIX-30 fallback covers the space input. Neither fallback closes these bugs. |
927927

928+
The PR #2306 regression exposed an adapter ownership-check bypass in both
929+
backends: `Repo`'s automatic `GIT_DIR` made Git 2.54.0 treat hook execution as
930+
explicitly selecting the repository, so it ran a hook where an ordinary
931+
`git commit` rejected dubious ownership. `run_commit_hook()` now asks Git to
932+
rediscover the repository from its original location before executing hooks.
933+
`test_pre_commit_hook_not_run_in_repository_git_refuses` uses Git's
934+
`GIT_TEST_ASSUME_DIFFERENT_OWNER` switch and verifies rejection, `safe.directory`
935+
exemptions and explicit command-environment behavior across ordinary, bare,
936+
linked, separate-directory and configured-worktree layouts. This fixes the
937+
adapter bug without claiming a native Gix trust check; GIX-23 remains open.
938+
928939
The October 22 Windows Gix CI run on Git 2.56.0.windows.2, CPython 3.12.10,
929940
and GixPython 0.2.0 exposed an adapter failure in
930941
`test_ignored_items_reported_verbatim`: Python's path metadata lookup raises

‎git/index/fun.py‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
from typing import TYPE_CHECKING, Tuple, Union, cast
77

88
from git.cmd import Git
9-
from git.exc import GitCommandError, HookExecutionError
9+
from git.exc import GitCommandError, HookExecutionError, UnsafeOptionError
1010
from git.types import PathLike
1111
from .typ import BaseIndexEntry
1212

@@ -27,10 +27,37 @@ def hook_path(name: str, git_dir: PathLike) -> str:
2727

2828

2929
def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
30-
"""Run a Git commit hook, ignoring missing hooks."""
30+
"""Run a Git commit hook, ignoring missing hooks.
31+
32+
:raise GitCommandError: If Git rejects the repository before executing its hook.
33+
"""
3134
Git._check_operand(name, "hook name")
3235
if name not in ("pre-commit", "commit-msg", "post-commit"):
3336
raise ValueError("Only commit hooks are supported")
37+
if any("\0" in arg for arg in Git._unpack_args(args)):
38+
raise UnsafeOptionError("Git arguments cannot contain NUL bytes")
39+
command = index.repo.git
40+
# Repo's automatic GIT_DIR bypasses Git's discovery-time ownership check.
41+
one_shot_options, command._git_options = command._git_options, ()
42+
options = [*command._persistent_git_options, *one_shot_options]
43+
discovery_env = {}
44+
if (
45+
"GIT_DIR" in command._repository_environment
46+
and command.environment().get("GIT_DIR") == command._repository_environment["GIT_DIR"]
47+
and not any(option == "-C" or option == "--git-dir" or option.startswith("--git-dir=") for option in options)
48+
):
49+
options[:0] = ["-C", index.repo._discovery_path]
50+
discovery_env = {
51+
key: None
52+
for key, value in command._repository_environment.items()
53+
if command.environment().get(key) == value
54+
}
55+
command.execute(
56+
[command.GIT_PYTHON_GIT_EXECUTABLE, *options, "rev-parse", "--git-dir"],
57+
env=discovery_env,
58+
_repository_redirected="-C" in options,
59+
)
60+
command._git_options = one_shot_options
3461
try:
3562
index.repo.git._call_process_safe(
3663
"hook",

‎git/repo/base.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,7 @@ def __init__(
375375
checkout = checkout_probe._call_process_safe("rev_parse", "--show-toplevel")
376376

377377
self.git_dir = git_dir
378+
self._discovery_path = git_dir if candidate == curpath else curpath
378379
probe.update_environment(GIT_DIR=git_dir)
379380
try:
380381
if native is not NotImplemented:

‎test/test_gix_backend.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -342,18 +342,18 @@ def test_hooks_retain_cli_even_when_missing(repo):
342342
run_commit_hook(name, current.index)
343343
with pytest.raises(UnsafeOptionError, match="NUL"):
344344
run_commit_hook("commit-msg", repo.index, "message\0path")
345-
assert cli.call_count == 6
346-
assert _backend.statistics().get(("Git.execute", "CLI process"), 0) == count + 6
345+
assert cli.call_count == 12
346+
assert _backend.statistics().get(("Git.execute", "CLI process"), 0) == count + 12
347347
hook = Path(repo.common_dir, "hooks", "pre-commit")
348348
hook.parent.mkdir(exist_ok=True)
349349
hook.write_text("present, even without executable permission")
350350
with patch.object(Git, "execute", return_value="") as cli:
351351
run_commit_hook("pre-commit", linked.index)
352-
cli.assert_called_once()
352+
assert cli.call_count == 2
353353
repo.git.config("core.hooksPath", "custom-hooks")
354354
with patch.object(Git, "execute", return_value="") as cli:
355355
run_commit_hook("post-commit", repo.index)
356-
cli.assert_called_once()
356+
assert cli.call_count == 2
357357

358358

359359
@pytest.mark.parametrize("config_variant", ["normal", "duplicate", "include"])

‎test/test_index.py‎

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
import ddt
1717
import pytest
1818

19-
from git import NULL_TREE, BlobFilter, Diff, Git, IndexFile, Object, Tree
19+
from git import NULL_TREE, BlobFilter, Diff, Git, IndexFile, Object, Repo, Tree
2020
from git.diff import NULL_TREE_SHA
2121
from git.exc import (
2222
CheckoutError,
@@ -1189,6 +1189,86 @@ def test_pre_commit_hook_outside_worktree(self):
11891189
output = Path(repo.working_dir, "outside-hook-output.txt").read_text(encoding="utf-8")
11901190
self.assertEqual(output, "ran outside hook\n")
11911191

1192+
def test_pre_commit_hook_not_run_in_repository_git_refuses(self):
1193+
for layout in ("regular", "bare", "linked", "separate", "configured"):
1194+
# Linked worktree registrations contain absolute paths.
1195+
with self.subTest(layout=layout), fixture_repo(
1196+
"hook-ownership.sh",
1197+
layout,
1198+
write=True,
1199+
execute=layout == "linked",
1200+
subdir="metadata" if layout == "configured" else "repo",
1201+
) as repo:
1202+
output = Path(repo.working_dir, "output.txt")
1203+
discovery = repo.git_dir if layout == "configured" else repo.working_dir
1204+
baseline_output = Path(discovery, "output.txt")
1205+
baseline = Git(discovery)
1206+
# Reset any configured exemptions, as in Git's t0033-safe-directory.sh.
1207+
with mock.patch.dict(
1208+
os.environ,
1209+
{
1210+
"GIT_TEST_ASSUME_DIFFERENT_OWNER": "1",
1211+
"GIT_CONFIG_COUNT": "1",
1212+
"GIT_CONFIG_KEY_0": "safe.directory",
1213+
"GIT_CONFIG_VALUE_0": "",
1214+
},
1215+
):
1216+
if layout == "regular":
1217+
with self.assertRaisesRegex(GitCommandError, "dubious ownership"):
1218+
baseline.commit("--allow-empty", "--no-gpg-sign", "-m", "Git refuses this repository")
1219+
self.assertFalse(output.exists())
1220+
with self.assertRaisesRegex(GitCommandError, "dubious ownership"):
1221+
repo.index.commit("Git refuses this repository")
1222+
self.assertFalse(output.exists())
1223+
with self.assertRaises(GitCommandError):
1224+
baseline.hook("run", "pre-commit")
1225+
self.assertFalse(baseline_output.exists())
1226+
with self.assertRaisesRegex(GitCommandError, "dubious ownership"):
1227+
run_commit_hook("pre-commit", repo.index)
1228+
self.assertFalse(output.exists())
1229+
1230+
for environment in (
1231+
{"GIT_CONFIG_VALUE_0": str(discovery)},
1232+
{"GIT_DIR": str(repo.git_dir), "GIT_WORK_TREE": str(repo.working_dir)},
1233+
):
1234+
with baseline.custom_environment(**environment), repo.git.custom_environment(**environment):
1235+
baseline.hook("run", "pre-commit")
1236+
expected = baseline_output.read_text(encoding="utf-8")
1237+
baseline_output.unlink()
1238+
run_commit_hook("pre-commit", repo.index)
1239+
self.assertEqual(output.read_text(encoding="utf-8"), expected)
1240+
output.unlink()
1241+
1242+
def test_commit_hook_preserves_explicit_repository_selection(self):
1243+
for selection in ("one-shot", "persistent", "environment", "git-dir-option"):
1244+
with self.subTest(selection=selection), fixture_repo("hook-ownership.sh", "regular", write=True) as repo:
1245+
with Repo(repo.git_dir) as metadata:
1246+
index = metadata.index
1247+
baseline = Git(metadata.working_dir)
1248+
metadata.git.version_info
1249+
environment = {"GIT_DIR": ".git"} if selection == "environment" else {}
1250+
with baseline.custom_environment(**environment), metadata.git.custom_environment(**environment):
1251+
for command in (baseline, metadata.git):
1252+
if selection == "one-shot":
1253+
command(C=".git")
1254+
elif selection == "persistent":
1255+
command.set_persistent_git_options(C=".git")
1256+
elif selection == "git-dir-option":
1257+
command(git_dir=".git")
1258+
output = Path(repo.git_dir if selection in ("one-shot", "persistent") else repo.working_dir)
1259+
output /= "output.txt"
1260+
baseline.hook("run", "pre-commit")
1261+
expected = output.read_text(encoding="utf-8")
1262+
output.unlink()
1263+
run_commit_hook("pre-commit", index)
1264+
self.assertEqual(output.read_text(encoding="utf-8"), expected)
1265+
1266+
metadata.git.set_persistent_git_options()
1267+
metadata.git(C="missing")
1268+
with self.assertRaises(GitCommandError):
1269+
run_commit_hook("pre-commit", index)
1270+
self.assertEqual(metadata.git.rev_parse("--git-dir"), str(metadata.git_dir))
1271+
11921272
def test_pre_commit_hook_fail(self):
11931273
with fixture_repo("index.sh", "hook-fail", write=True) as repo:
11941274
with self.assertRaises(HookExecutionError) as caught:

‎tests/fixtures/hook-ownership.sh‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
#!/usr/bin/env bash
2+
set -eu
3+
4+
case "$1" in
5+
bare)
6+
git init --bare --initial-branch=master repo
7+
hooks=repo/hooks
8+
;;
9+
linked)
10+
git init --initial-branch=master main
11+
git -C main -c commit.gpgsign=false commit --allow-empty -m initial
12+
git -C main worktree add -b linked ../repo
13+
hooks=main/.git/hooks
14+
;;
15+
separate)
16+
git init --initial-branch=master --separate-git-dir=metadata repo
17+
printf 'gitdir: ../metadata\n' >repo/.git
18+
hooks=metadata/hooks
19+
;;
20+
configured)
21+
git init --initial-branch=master repo
22+
git -C repo config core.worktree ../repo
23+
mv repo/.git metadata
24+
hooks=metadata/hooks
25+
;;
26+
regular)
27+
git init --initial-branch=master repo
28+
hooks=repo/.git/hooks
29+
;;
30+
*) exit 1 ;;
31+
esac
32+
printf "#!/usr/bin/env sh\necho 'ran hook' >output.txt\n" >"$hooks/pre-commit"
33+
chmod +x "$hooks/pre-commit"

0 commit comments

Comments
 (0)