angular/dev-infra — ng-dev pr checkout / pr rebase: git hooks supplied by the pull request are executed on checkout #4005

Project: angular/dev-infra (ng-dev/pr), consumed by caretakers and reviewers of angular/angular
Commit tested: a948cdfd (main)
Affected files: ng-dev/pr/common/checkout-pr.ts, ng-dev/pr/rebase/index.ts, ng-dev/pr/discover-new-conflicts/index.ts
Type: Git hooks supplied by a pull request are executed locally when the pull request is checked out

Description

ng-dev pr checkout and ng-dev pr rebase fetch a contributor's fork and check it out into the
user's working tree:

// ng-dev/pr/common/checkout-pr.ts:113-114
git.run(['fetch', '-q', headRefUrl, '--', headRefName]);
git.run(['checkout', '--detach', 'FETCH_HEAD']);

// ng-dev/pr/rebase/index.ts:77-78
git.run(['fetch', '-q', headRefUrl, '--deepen=500', '--', headRefName]);
git.run(['checkout', '-q', '--detach', 'FETCH_HEAD']);

In any workspace where pnpm install has run, package.json has "prepare": "husky", so husky
sets core.hooksPath to .husky/_. That directory lives inside the working tree and husky writes
.husky/_/.gitignore containing *, so nothing under it is tracked upstream. A pull request can
therefore add its own .husky/_/h; checking that branch out replaces husky's generated wrapper, and
git then runs it — post-checkout on both commands, plus pre-push on the rebase path.

mergePullRequest sets HUSKY=0 for this reason:

// ng-dev/pr/merge/merge-pull-request.ts:39-41
// Set the environment variable to skip all git commit hooks triggered by husky. We are unable to
// rely on `--no-verify` as some hooks still run, notably the `prepare-commit-msg` hook.
process.env['HUSKY'] = '0';

That covers the merge command only, and it depends on the wrapper being husky's own — husky's
generated h honours $HUSKY, a wrapper supplied by the pull request does not. pr checkout and
pr rebase set nothing at all:

$ grep -rn "HUSKY" ng-dev/
ng-dev/release/publish/actions.ts:438:    // like `prepare-commit-message` which still run. We have set the `HUSKY=0` environment
ng-dev/release/publish/index.ts:93:    process.env['HUSKY'] = '0';
ng-dev/pr/merge/merge-pull-request.ts:41:  process.env['HUSKY'] = '0';

ng-dev pr discover-new-conflicts performs the same checkout at
ng-dev/pr/discover-new-conflicts/index.ts:75 and :97, the second inside a loop over pending pull
requests. I have not executed that command — its GraphQL query exceeds GitHub's node limit in my
test repository — so it is cited from source only.

The remaining FETCH_HEAD checkout, ng-dev/release/publish/actions.ts:397, fetches an upstream
branch via getRepoGitUrl() rather than a fork, and is unaffected.

Reproduction

A test repository wired the same way as angular/angular ("prepare": "husky", husky 9.1.7),
where main tracks nothing under .husky/_. A pull request changes one ordinary file and adds
.husky/_/h:

mkdir -p .husky/_
printf '#!/usr/bin/env sh\necho "hook=$(basename "$0") user=$(whoami) cwd=$(pwd)" >> /tmp/marker\n' > .husky/_/h
echo "hello world" > src.txt
git add -f .husky/_/h src.txt
git commit -m "fix: update greeting text"
git push

Fresh clone, with husky installed exactly as pnpm install leaves it:

$ npm i husky@9.1.7 && ./node_modules/.bin/husky
$ git config core.hooksPath
.husky/_
$ git status --porcelain | wc -l
0
$ git ls-files .husky/_ | wc -l
0
$ ls /tmp/marker
ls: /tmp/marker: No such file or directory

Then check the pull request out:

$ ng-dev pr checkout 1

Actual result

The author of this pull request does not allow maintainers to modify the pull
request. Since you will not be able to push changes to the original pull request
you will instead need to perform a "takeover." In a takeover, the original pull
request will be checked out, the commits are modified to close the original on
merge of the newly created branch.
? Would you like to create a takeover pull request? (Y/n)

$ cat /tmp/marker
hook=post-checkout user=<me> cwd=<my checkout>

The wrapper supplied by the pull request has replaced .husky/_/h and run. Note the marker is
written while the command is still sitting at the prompt, so it happens before any decision is made
about the pull request. ng-dev pr rebase 1 behaves the same way and additionally fires pre-push.

Expected result

No hook supplied by the checked-out pull request runs. /tmp/marker does not exist.

Suggested fix

runGraceful already injects a config override on every git invocation for a related reason, so the
hooks path fits on the same line. Config passed on the command line cannot be replaced by
checked-out content, which HUSKY=0 can be:

--- a/ng-dev/utils/git/git-client.ts
+++ b/ng-dev/utils/git/git-client.ts
@@ -9,6 +9,7 @@
 import {DryRunError, isDryRun} from '../dry-run.js';
 import {GithubConfig, assertValidGithubConfig, getConfig} from '../config.js';
 import {SpawnSyncOptions, SpawnSyncReturns, spawnSync} from 'child_process';
+import {devNull} from 'os';
 import {Log} from '../logging.js';
 
 import {GithubClient} from './github.js';
@@ -92,7 +93,10 @@
 
     // Clear the credential helper that is used, preventing the temporary token from being saved as a
     // valid token for future use.
-    args = ['-c', 'credential.helper=', ...args];
+    // Point the hooks path at a location which can never hold hooks. Commands such as `pr checkout`
+    // and `pr rebase` check out pull request content into the working tree, and hooks provided by
+    // that content must not be executed.
+    args = ['-c', 'credential.helper=', '-c', `core.hooksPath=${devNull}`, ...args];
     // To improve the debugging experience in case something fails, we print all executed Git
     // commands at the DEBUG level to better understand the git actions occurring.
     // Note that we sanitize the command before printing it to the console. We do not want to

One existing spec asserts on the injected argument count and needs updating with it:

--- a/ng-dev/utils/test/git-client.spec.ts
+++ b/ng-dev/utils/test/git-client.spec.ts
@@ -85,8 +85,8 @@
 
     // Write mock git script
     const scriptContent = `#!/bin/sh
-# shift 2 to get rid of -c credential.helper=
-shift 2
+# shift 4 to get rid of -c credential.helper= -c core.hooksPath=
+shift 4
 case "$1" in
   mock-stdout)
     echo "stdout https://user:pass@github.com"

Verified fix

With the above applied to a948cdfd, the same reproduction on a restored clone:

$ ng-dev pr checkout 1
The author of this pull request does not allow maintainers to modify the pull
request. […]
? Would you like to create a takeover pull request? (Y/n)

$ ls /tmp/marker
ls: /tmp/marker: No such file or directory

The command behaves identically and the hook no longer runs.

$ jasmine ng-dev/utils/test/git-client.spec.ts
14 specs, 0 failures

$ jasmine ng-dev/pr/merge/strategies/strategy.spec.ts ng-dev/pr/merge/integration.spec.ts
41 specs, 0 failures

$ tsc --noEmit -p tsconfig.json | grep -c "error TS"
1901   # with the patch
1901   # without the patch

The pre-existing errors are spec files missing jasmine types and bazel/rules/rules_angular
sources that require Bazel-generated inputs; the count is identical before and after.

I'm happy to send this as a pull request.

Status

The three call sites above are present on main at a948cdfd.

3c28e060 ("fix(ng-dev/pr): prevent option injection in git fetch calls") hardened the git fetch
on checkout-pr.ts:113 by adding the -- separator; the git checkout on the following line was
not part of that change.

No existing issue or pull request covers this. Searched issues for husky, hooksPath,
pr checkout hooks and git hooks, and open pull requests touching ng-dev/pr/ or git-client —
the only matches are unrelated (#678 dependency dashboard, #769 a CI check, #3975 enforced statuses).

Environment

angular/dev-infra@a948cdfd; Node v26.7.0, tsx 4.23.13, husky 9.1.7, git 2.54.0, macOS arm64.
ng-dev run from source with tsx. No Angular-owned repository was involved; all testing used a
private test repository.