fix(pull): re-evaluate review official flag on target branch change (#38319)

The `official` flag of a pull request review is computed against the
target branch's protection rules at submit time and stored on the review
record. `ChangeTargetBranch` updated the base branch but never
re-evaluated it, so an `official` approval obtained against an
unprotected branch could be retargeted onto a protected branch and
satisfy its required approvals, bypassing the branch protection.

This re-evaluates the `official` flag of the latest approve/reject
reviews against the new base branch whenever the target branch changes.

models/issues/review.go

@@ -324,6 +324,59 @@ func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organizatio
 	return slices.Contains(pb.ApprovalsWhitelistTeamIDs, team.ID), nil
 }
 
+// RecalculateReviewsOfficial re-evaluates the "official" flag of the latest approve
+// and reject reviews of an issue against its pull request's current base branch.
+// It must be called whenever the target branch changes, otherwise an approval that
+// was official on the previous (possibly unprotected) branch would keep satisfying
+// the new branch's protection rules.
+func RecalculateReviewsOfficial(ctx context.Context, issue *Issue) error {
+	if err := issue.LoadPullRequest(ctx); err != nil {
+		return err
+	}
+
+	// Clearing and restoring the official flags must happen atomically, otherwise a
+	// failure in between would leave the reviews without any official flag set.
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		// Only the latest approve/reject review of each reviewer counts as official, so
+		// clear the flag on all of them first and restore it only where it still applies.
+		if _, err := db.GetEngine(ctx).
+			Where("issue_id = ?", issue.ID).
+			In("type", ReviewTypeApprove, ReviewTypeReject).
+			Cols("official").
+			Update(&Review{Official: false}); err != nil {
+			return err
+		}
+
+		reviews, err := FindLatestReviews(ctx, FindReviewOptions{
+			Types:   []ReviewType{ReviewTypeApprove, ReviewTypeReject},
+			IssueID: issue.ID,
+		})
+		if err != nil {
+			return err
+		}
+
+		for _, review := range reviews {
+			if err := review.LoadReviewer(ctx); err != nil {
+				return err
+			}
+			if review.Reviewer == nil {
+				continue
+			}
+			official, err := IsOfficialReviewer(ctx, issue, review.Reviewer)
+			if err != nil {
+				return err
+			}
+			if official {
+				if _, err := db.GetEngine(ctx).ID(review.ID).Cols("official").Update(&Review{Official: true}); err != nil {
+					return err
+				}
+			}
+		}
+
+		return nil
+	})
+}
+
 // CreateReview creates a new review based on opts
 func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error) {
 	return db.WithTx2(ctx, func(ctx context.Context) (*Review, error) {

models/issues/review_test.go

@@ -6,6 +6,8 @@ package issues_test
 import (
 	"testing"
 
+	"gitea.dev/models/db"
+	git_model "gitea.dev/models/git"
 	issues_model "gitea.dev/models/issues"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unittest"
@@ -386,3 +388,45 @@ func TestAddReviewRequest(t *testing.T) {
 	assert.NotNil(t, comment.CommentMetaData)
 	assert.Equal(t, issues_model.SpecialDoerNameCodeOwners, comment.CommentMetaData.SpecialDoerName)
 }
+
+func TestRecalculateReviewsOfficial(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	// PR #2 targets repo1's "master" branch. Simulate an approval that became
+	// official while the PR targeted an unprotected branch.
+	issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3})
+	reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+	review, err := issues_model.CreateReview(t.Context(), issues_model.CreateReviewOptions{
+		Type:     issues_model.ReviewTypeApprove,
+		Issue:    issue,
+		Reviewer: reviewer,
+		Official: true,
+	})
+	assert.NoError(t, err)
+
+	// Protect the (now current) target branch with an approvals whitelist that
+	// does not include the reviewer, mirroring a retarget onto a protected branch.
+	rule := &git_model.ProtectedBranch{
+		RepoID:                    issue.RepoID,
+		RuleName:                  "master",
+		EnableApprovalsWhitelist:  true,
+		ApprovalsWhitelistUserIDs: []int64{2},
+		RequiredApprovals:         1,
+	}
+	assert.NoError(t, db.Insert(t.Context(), rule))
+
+	// Re-evaluating must strip the stale official flag, otherwise the approval
+	// would still satisfy the protected branch's required approvals.
+	assert.NoError(t, issues_model.RecalculateReviewsOfficial(t.Context(), issue))
+	review = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: review.ID})
+	assert.False(t, review.Official)
+
+	// Once the reviewer is whitelisted, re-evaluating restores the official flag.
+	rule.ApprovalsWhitelistUserIDs = []int64{2, reviewer.ID}
+	_, err = db.GetEngine(t.Context()).ID(rule.ID).Cols("approvals_whitelist_user_i_ds").Update(rule)
+	assert.NoError(t, err)
+
+	assert.NoError(t, issues_model.RecalculateReviewsOfficial(t.Context(), issue))
+	review = unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: review.ID})
+	assert.True(t, review.Official)
+}

services/pull/pull.go

@@ -326,6 +326,14 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer
 			return fmt.Errorf("syncCommitDivergence: %w", err)
 		}
 
+		// The "official" flag of existing reviews was computed against the previous
+		// target branch's protection rules, so re-evaluate it against the new branch.
+		// Otherwise a stale official approval could bypass the new branch's protection.
+		pr.Issue.PullRequest = pr
+		if err := issues_model.RecalculateReviewsOfficial(ctx, pr.Issue); err != nil {
+			return fmt.Errorf("RecalculateReviewsOfficial: %w", err)
+		}
+
 		// Create comment
 		options := &issues_model.CreateCommentOptions{
 			Type:   issues_model.CommentTypeChangeTargetBranch,