Repository navigation
fix(index): let git check the repository before running a commit hook - #2306
Open
Keerthana-64 wants to merge 1 commit into
Open
Keerthana-64 wants to merge 1 commit into
Keerthana-64 wants to merge 1 commit into
Conversation
`run_commit_hook` finds a hook in `.git/hooks` or `core.hooksPath`, read through GitPython's own config parser, and executes it directly. No git command runs before that, so git's check that the repository is safe to use (the ownership and `safe.directory` check added for CVE-2022-24765) has not happened when code from the repository is executed. `IndexFile.commit` runs the `pre-commit` and `commit-msg` hooks before it calls git at all. In a repository owned by another user, where `git commit` stops with "detected dubious ownership" without running any hook, `repo.index.commit()` ran the repository's `pre-commit` hook and only then failed in `git hash-object`. That is the situation git's check exists for: a `.git` directory planted by another user in a shared or parent directory (`Repo(path, search_parent_directories=True)` finds it the same way git would). Before executing a hook that exists, `run_commit_hook` now runs `git rev-parse --git-dir` in the repository, so git applies its own policy, including `safe.directory` and the exemptions git makes. When git refuses, the `GitCommandError` it raises is the one the commit already ended with, only now before the hook runs. When git accepts the repository, hooks run as before, and nothing changes when there is no hook. Asking git, rather than reimplementing the ownership rules, keeps the result identical to `git commit` for every git version, including versions older than 2.35.2 that make no check. Adds `test_pre_commit_hook_not_run_in_repository_git_refuses` to `test/test_index.py`, which uses git's `GIT_TEST_ASSUME_DIFFERENT_OWNER` to simulate another owner; it fails before this change and passes after. `test_run_commit_hook` and `test_run_commit_hook_outside_worktree_on_windows` mock `Git.execute` (the latter because it fakes the Windows `PATH`), so they now allow this one call while still rejecting any git command during hook lookup. `ruff check`, `ruff format --check` and `mypy` pass. In a full clone prepared with `init-tests-after-clone.sh`, the pytest suite passes except `test_diff_with_staged_file`, `test_index_mutation`, `test_blame_content_with_carriage_returns`, `test_empty_repo`, `test_rebasing` and the two `test_actor_get_uid_laziness_*` tests, which fail the same way without this change on this machine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repro: in a repository git refuses as "dubious ownership" (simulated with
GIT_TEST_ASSUME_DIFFERENT_OWNER=1),repo.index.commit("x")runs.git/hooks/pre-commitand only then fails ingit hash-object, whilegit commitrefuses without running any hook.Cause:
run_commit_hookfinds and executes the hook without calling git, andIndexFile.commitrunspre-commitandcommit-msgbefore its first git command, so git'ssafe.directorycheck (CVE-2022-24765) comes after code from the repository has already run.Fix: run
git rev-parse --git-dirbefore executing an existing hook so git applies its own ownership policy. Repositories git accepts behave as before, and the new test intest/test_index.pyfails before the change and passes after.