fix: sync PR head ref on the actual reopen path - #38733
Open
lunny wants to merge 1 commit into
Open
Conversation
The GitHub-flow head-ref consistency check in NewComment was gated on branchOtherUnmergedPR != nil, which is exactly when the reopen is refused and ReopenIssue is never called. So a genuine reopen never refreshed refs/pull/<index>/head and could reopen a PR against a stale ref, while a refused reopen force-pushed into the base repo for a PR that stays closed. Also fix the head-branch existence check, which queried the head repo with pull.BaseBranch, and guard the nil HeadRepo left behind by LoadHeadRepo when the fork was deleted. Assisted-by: Codet:claude-opus-4-6
GiteaBot
added
the
lgtm/need 2
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
2 participants
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.
The GitHub-flow head-ref consistency check in
NewCommentis gated onbranchOtherUnmergedPR != nil. That value is non-nil exactly when another open unmerged PR already exists for the same head/base branches, which is when the reopen is refused andReopenIssueis never called. The gate was introduced by 7b17234; before that the block ran on every reopen attempt.So the check runs in both wrong directions:
refs/pull/<index>/head, so a PR whose head branch moved while it was closed is reopened against a stale ref.This moves the block onto the path where the PR is actually reopened, and syncs the ref before
StartPullRequestCheckImmediatelyso the patch is regenerated against the correct ref.Two related fixes in the same block:
pull.BaseBranchinstead ofpull.HeadBranch. A fork without a branch named like the base branch got a bogus "cannot reopen" refusal, while a deleted head branch slipped through into a 500.LoadHeadRepodeliberately swallowsErrRepoNotExist, leavingHeadReponil when the fork was deleted. Since the block now runs on the common reopen path, that is guarded and treated like a missing head branch.The refusal path sets a flash and suppresses the reopen instead of returning without a body, which the
form-fetch-actioncaller cannot handle.The force-push now reuses
pull_service.PushToBaseRepo, still behind the commit-ID comparison so nothing is pushed when the ref is already in sync.Reported as #38718. The security claim in that report does not hold:
NewCommentis only reachable via the web/commentsroute, which enables neitherAllowBasicnorAllowOAuth2, so a public-only token cannot authenticate it and the missingTokenIsPublicOnlycheck there is not exploitable. The reopen regression above is the real defect.TestPullReopenSyncsHeadRefcovers it and fails without the router change.Generated by Codet