fix(ng-dev): verify validated head SHA before merging a pull request - #3911
fix(ng-dev): verify validated head SHA before merging a pull request#3911herdiyana256 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation to ensure that a pull request's head commit has not changed between validation and merging, preventing the landing of unreviewed commits. It adds a new MismatchedPullRequestHeadShaFatalError and checks the fetched head SHA against the expected SHA in MergeStrategy#prepare, as well as pinning the API merge to the validated head SHA. Feedback suggests updating the error handling in GithubApiMergeStrategy to gracefully handle HTTP 409 errors from GitHub when the head SHA mismatches during the merge call.
The merge tooling validates a pull request (approvals, CI status, target labels) against the head commit the GitHub API reports when the PR is loaded and stores it as `pullRequest.headSha`, but the value was never enforced. Both merge strategies then operate on the live PR head instead: `MergeStrategy.prepare` fetches the mutable `pull/<number>/head` ref into a local branch and the merge proceeds from there, and `GithubApiMergeStrategy` calls `pulls.merge` without a `sha`. A commit pushed to the pull request after validation but before the fetch or API call is therefore merged without ever being reviewed or checked. `prepare` now resolves the fetched head and fails closed unless it equals the validated `headSha`, and the API merge strategy passes `headSha` to `pulls.merge` so the server rejects the merge if the head has moved.
a3b54f5 to
0f690ac
Compare
|
Gentle bump on this one. It has been open a week with no reviewer assigned. This is a security fix in the same @josephperrott would you be the right person to look, given the recent |
…oved Pinning the API merge to the validated head SHA means GitHub now rejects the merge with a 409 if the head moved after validation, but that error was falling through to the generic `throw e` and surfacing as a raw Octokit exception instead of a clear, actionable message. Adds a branch alongside the existing 403/404 handling that catches the 409 and raises a FatalMergeToolError explaining what happened (the head changed after validation) and what to do (re-run the merge).
|
Pushed 7c7c8dd addressing the feedback on the 409. Added a branch next to the existing 403/404 handling in
|
1 participant
ng-dev pr mergevalidates a pull request (approvals, CI status, target labels) against the head commit the GitHub API reports when the PR is loaded, and stores it aspullRequest.headSha. That value was never checked again, so both merge strategies operate on the live pull request head instead of the commit that was validated:MergeStrategy.preparefetches the mutablepull/<number>/headref intomerge_pr_head, and the autosquash strategy rebases and cherry-picks from there.GithubApiMergeStrategycallspulls.mergewithout asha, so GitHub merges whatever the head currently is.Between the validation read and the fetch (or the API call) the pull request author can push a new commit. The merge then lands that commit even though it was never reviewed or run through CI, a time-of-check/time-of-use gap.
preparenow resolves the fetched head and fails closed unless it equals the validatedheadSha, and the API merge strategy passesheadShatopulls.mergeso the server rejects the merge with a 409 if the head has moved since validation. A unit test covers both the mismatch (rejected) and match (allowed) cases.