fix(ng-dev): verify validated head SHA before merging a pull request

ng-dev/pr/merge/failures.ts

@@ -51,3 +51,14 @@ export class PullRequestValidationError extends FatalMergeToolError {
     super('Tool exited as at least one pull request validation error was discovered.');
   }
 }
+
+export class MismatchedPullRequestHeadShaFatalError extends FatalMergeToolError {
+  constructor(expectedSha: string, actualSha: string) {
+    super(
+      `Pull request head commit changed after it was validated. The pull request was ` +
+        `validated at ${expectedSha}, but its head is now ${actualSha}. Merging would land ` +
+        `commits that were never reviewed or checked. Please re-run the merge so the new head ` +
+        `is validated.`,
+    );
+  }
+}

ng-dev/pr/merge/strategies/api-merge.ts

@@ -58,14 +58,20 @@ export class GithubApiMergeStrategy extends AutosquashMergeStrategy {
    * @throws {FatalMergeToolError} A fatal error if the merge could not be performed.
    */
   override async merge(pullRequest: PullRequest): Promise<void> {
-    const {githubTargetBranch, prNumber, needsCommitMessageFixup, targetBranches} = pullRequest;
+    const {githubTargetBranch, prNumber, needsCommitMessageFixup, targetBranches, headSha} =
+      pullRequest;
     const cherryPickTargetBranches = targetBranches.filter((b) => b !== githubTargetBranch);
     const commits = await this.getPullRequestCommits(pullRequest);
     const {squashCount, fixupCount, normalCommitsCount} = await this.getCommitsInfo(pullRequest);
     const method = this.getMergeActionFromPullRequest(pullRequest);
     const mergeOptions: OctokitMergeParams = {
       pull_number: prNumber,
       merge_method: method === 'auto' ? 'rebase' : method,
+      // Pin the merge to the head commit that was validated. `pulls.merge` otherwise merges
+      // whatever the pull request head currently is, so a commit pushed after validation but
+      // before this call would be merged without review or CI. With `sha` set, GitHub rejects
+      // the merge (HTTP 409) if the head has moved since it was validated.
+      sha: headSha,
       ...this.git.remoteParams,
     };
 
@@ -138,6 +144,16 @@ export class GithubApiMergeStrategy extends AutosquashMergeStrategy {
       if (isGithubApiError(e) && (e.status === 403 || e.status === 404)) {
         throw new FatalMergeToolError('Insufficient Github API permissions to merge pull request.');
       }
+      // Github returns `409` when the `sha` we pinned the merge to no longer matches the pull
+      // request's actual head. That means the pull request's head moved after it was validated
+      // (approvals, CI status) but before this merge call went out.
+      if (isGithubApiError(e) && e.status === 409) {
+        throw new FatalMergeToolError(
+          `Pull request head commit changed after it was validated (expected ${headSha}). ` +
+            `Merging now would land commits that were never reviewed or checked. Please re-run ` +
+            `the merge so the new head is validated.`,
+        );
+      }
       throw e;
     }
 

ng-dev/pr/merge/strategies/strategy.spec.ts

@@ -0,0 +1,63 @@
+/**
+ * @license
+ * Copyright Google LLC
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-client.js';
+import {MismatchedPullRequestHeadShaFatalError} from '../failures.js';
+import {PullRequest} from '../pull-request.js';
+import {MergeStrategy, TEMP_PR_HEAD_BRANCH} from './strategy.js';
+
+/** Minimal concrete strategy so the abstract base's `prepare` can be exercised. */
+class TestMergeStrategy extends MergeStrategy {
+  override async merge(): Promise<void> {}
+}
+
+/** Builds a `PullRequest` with only the fields `prepare` relies on. */
+function createPullRequest(headSha: string): PullRequest {
+  return {
+    prNumber: 123,
+    headSha,
+    targetBranches: ['main'],
+  } as unknown as PullRequest;
+}
+
+describe('MergeStrategy#prepare', () => {
+  let gitClient: jasmine.SpyObj<AuthenticatedGitClient>;
+
+  beforeEach(() => {
+    gitClient = jasmine.createSpyObj<AuthenticatedGitClient>('git', ['run', 'getRepoGitUrl']);
+    gitClient.getRepoGitUrl.and.returnValue('https://github.com/angular/angular.git');
+  });
+
+  /** Makes `git rev-parse merge_pr_head` resolve to `fetchedHeadSha`. */
+  function stubFetchedHead(fetchedHeadSha: string): void {
+    gitClient.run.and.callFake((args: string[]) => {
+      if (args[0] === 'rev-parse' && args[1] === TEMP_PR_HEAD_BRANCH) {
+        return {stdout: `${fetchedHeadSha}\n`} as ReturnType<AuthenticatedGitClient['run']>;
+      }
+      return {stdout: ''} as ReturnType<AuthenticatedGitClient['run']>;
+    });
+  }
+
+  it('rejects when the fetched PR head no longer matches the validated head SHA', async () => {
+    // The PR was validated at `validated-sha`, but `pull/<n>/head` now points at a commit
+    // the author pushed after validation. Merging it would land unreviewed code.
+    stubFetchedHead('pushed-after-validation-sha');
+
+    await expectAsync(
+      new TestMergeStrategy(gitClient).prepare(createPullRequest('validated-sha')),
+    ).toBeRejectedWithError(MismatchedPullRequestHeadShaFatalError);
+  });
+
+  it('resolves when the fetched PR head matches the validated head SHA', async () => {
+    stubFetchedHead('validated-sha');
+
+    await expectAsync(
+      new TestMergeStrategy(gitClient).prepare(createPullRequest('validated-sha')),
+    ).toBeResolved();
+  });
+});

ng-dev/pr/merge/strategies/strategy.ts

@@ -10,6 +10,7 @@ import {AuthenticatedGitClient} from '../../../utils/git/authenticated-git-clien
 import {
   FatalMergeToolError,
   MergeConflictsFatalError,
+  MismatchedPullRequestHeadShaFatalError,
   MismatchedTargetBranchFatalError,
   UnsatisfiedBaseShaFatalError,
 } from '../failures.js';
@@ -38,6 +39,17 @@ export abstract class MergeStrategy {
       pullRequest.targetBranches,
       `pull/${pullRequest.prNumber}/head:${TEMP_PR_HEAD_BRANCH}`,
     );
+
+    // The pull request was validated (approvals, CI status, target labels) against
+    // `pullRequest.headSha`, the head commit the GitHub API reported when the pull request
+    // was loaded. `pull/<number>/head` is a mutable ref though, so a pull request author can
+    // push a new commit between that validation and this fetch. Merging whatever the ref now
+    // points at would land commits that were never reviewed or checked, so we require the
+    // fetched head to be exactly the validated commit and fail closed on any mismatch.
+    const fetchedHeadSha = this.git.run(['rev-parse', TEMP_PR_HEAD_BRANCH]).stdout.trim();
+    if (fetchedHeadSha !== pullRequest.headSha) {
+      throw new MismatchedPullRequestHeadShaFatalError(pullRequest.headSha, fetchedHeadSha);
+    }
   }
 
   /**