fix(actions)!: require merged PR to bypass fork PR approval gate (#38010) (#38041)

Backport #38010 by @bircni `ifNeedApproval` in `services/actions/notifier_helper.go` decided whether a fork PR's workflow run had to wait for maintainer approval. The bypass clause counted any prior `approved_by > 0` run for `(repo_id, trigger_user_id)`, so the very first Approve-and-run click on a contributor's fork PR permanently trusted that user for every future fork PR in the same repository — including PRs whose only change is the workflow YAML itself. Approving a workflow *run* is not the same as merging *code*. This change aligns the gate with GitHub Actions' first-time-contributor model: trust is granted only after the user has had a pull request merged in the repo. ## Behavior change - **Before**: one approval = permanent trust for that user in that repo. - **After**: every fork PR is gated until the contributor has at least one merged PR in the repo. Existing already-approved runs and merged PRs continue to work; only the trust criterion for *future* fork PRs changes. Maintainers who rely on the implicit "approve once" trust will see the approval banner reappear until they merge a PR from that contributor. --------- Signed-off-by: bircni <bircni@icloud.com> Co-authored-by: bircni <bircni@icloud.com>
2026-08-05 02:04:00 +02:00 · 2026-06-09 15:53:42 +02:00
parent 3b705738ab
commit e107498f3b
5 changed files with 239 additions and 18 deletions
@@ -140,3 +140,107 @@ jobs:
 		assert.Equal(t, actions_model.StatusWaiting, run2.Status)
 	})
 }
+
+// TestForkPullRequestApprovalNotBypassedByPriorApproval verifies that a single
+// approval on a fork PR does not permanently trust the contributor: a subsequent
+// fork PR from the same user must still be gated (Blocked / NeedApproval=true)
+// until that user has had a pull request merged in the repo.
+func TestForkPullRequestApprovalNotBypassedByPriorApproval(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		user2Session := loginUser(t, user2.Name)
+		user2Token := getTokenForLoggedInUser(t, user2Session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+		user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+		user4Session := loginUser(t, user4.Name)
+		user4Token := getTokenForLoggedInUser(t, user4Session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+
+		apiBaseRepo := createActionsTestRepo(t, user2Token, "fork-approval-regression", false)
+		baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiBaseRepo.ID})
+		user2APICtx := NewAPITestContext(t, baseRepo.OwnerName, baseRepo.Name, auth_model.AccessTokenScopeWriteRepository)
+		defer doAPIDeleteRepository(user2APICtx)(t)
+
+		wfTreePath := ".gitea/workflows/ci.yml"
+		wfContent := `name: CI
+on: pull_request
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo ok
+`
+		createWorkflowFile(t, user2Token, baseRepo.OwnerName, baseRepo.Name, wfTreePath,
+			getWorkflowCreateFileOptions(user2, baseRepo.DefaultBranch, "add ci", wfContent))
+
+		// user4 forks the repo
+		req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/forks", baseRepo.OwnerName, baseRepo.Name),
+			&api.CreateForkOption{Name: new("fork-approval-regression-fork")}).AddTokenAuth(user4Token)
+		resp := MakeRequest(t, req, http.StatusAccepted)
+		apiForkRepo := DecodeJSON(t, resp, &api.Repository{})
+		forkRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiForkRepo.ID})
+		user4APICtx := NewAPITestContext(t, user4.Name, forkRepo.Name, auth_model.AccessTokenScopeWriteRepository)
+		defer doAPIDeleteRepository(user4APICtx)(t)
+
+		// PR #1: a benign change from user4's fork — first-time contributor, gate engages.
+		doAPICreateFile(user4APICtx, "first.txt", &api.CreateFileOptions{
+			FileOptions: api.FileOptions{
+				NewBranchName: "first",
+				Message:       "first",
+				Author:        api.Identity{Name: user4.Name, Email: user4.Email},
+				Committer:     api.Identity{Name: user4.Name, Email: user4.Email},
+				Dates:         api.CommitDateOptions{Author: time.Now(), Committer: time.Now()},
+			},
+			ContentBase64: base64.StdEncoding.EncodeToString([]byte("first")),
+		})(t)
+		pr1, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":first")(t)
+		assert.NoError(t, err)
+
+		run1 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr1.Index)})
+		assert.True(t, run1.NeedApproval, "first fork PR must require approval")
+		assert.Equal(t, actions_model.StatusBlocked, run1.Status)
+
+		// user2 approves run1.
+		req = NewRequest(t, "POST", fmt.Sprintf("%s/actions/approve-all-checks?commit_id=%s", baseRepo.Link(), pr1.Head.Sha))
+		user2Session.MakeRequest(t, req, http.StatusOK)
+		run1 = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: run1.ID})
+		assert.False(t, run1.NeedApproval)
+		assert.Equal(t, user2.ID, run1.ApprovedBy)
+
+		// PR #2: same user, fresh branch. Pre-fix, this run was created with
+		// NeedApproval=false and dispatched immediately — the bypass path.
+		doAPICreateFile(user4APICtx, "second.txt", &api.CreateFileOptions{
+			FileOptions: api.FileOptions{
+				NewBranchName: "second",
+				Message:       "second",
+				Author:        api.Identity{Name: user4.Name, Email: user4.Email},
+				Committer:     api.Identity{Name: user4.Name, Email: user4.Email},
+				Dates:         api.CommitDateOptions{Author: time.Now(), Committer: time.Now()},
+			},
+			ContentBase64: base64.StdEncoding.EncodeToString([]byte("second")),
+		})(t)
+		pr2, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":second")(t)
+		assert.NoError(t, err)
+
+		run2 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr2.Index)})
+		assert.True(t, run2.NeedApproval, "second fork PR must still require approval — prior approval-to-run does not grant trust")
+		assert.Equal(t, actions_model.StatusBlocked, run2.Status)
+		assert.EqualValues(t, 0, run2.ApprovedBy)
+
+		// After merging PR #1, user4 becomes a known contributor and the gate lifts for a new PR.
+		doAPIMergePullRequest(user2APICtx, baseRepo.OwnerName, baseRepo.Name, pr1.Index)(t)
+		doAPICreateFile(user4APICtx, "third.txt", &api.CreateFileOptions{
+			FileOptions: api.FileOptions{
+				NewBranchName: "third",
+				Message:       "third",
+				Author:        api.Identity{Name: user4.Name, Email: user4.Email},
+				Committer:     api.Identity{Name: user4.Name, Email: user4.Email},
+				Dates:         api.CommitDateOptions{Author: time.Now(), Committer: time.Now()},
+			},
+			ContentBase64: base64.StdEncoding.EncodeToString([]byte("third")),
+		})(t)
+		pr3, err := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":third")(t)
+		assert.NoError(t, err)
+
+		run3 := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr3.Index)})
+		assert.False(t, run3.NeedApproval, "fork PR from a user with a prior merged PR should not require approval")
+	})
+}
@@ -484,14 +484,18 @@ jobs:
 			},
 			ContentBase64: base64.StdEncoding.EncodeToString([]byte("user4-fix2")),
 		})(t)
-		doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":do-not-cancel/ccc")(t)
-		// cannot fetch the task because cancel-in-progress is false
+		pr3, _ := doAPICreatePullRequest(user4APICtx, baseRepo.OwnerName, baseRepo.Name, baseRepo.DefaultBranch, user4.Name+":do-not-cancel/ccc")(t)
+		// cannot fetch the task: approval still required (user4 has no merged PR) and cancel-in-progress is false
 		runner.fetchNoTask(t)
 		runner.execTask(t, pr2Task1, &mockTaskOutcome{
 			result: runnerv1.Result_RESULT_SUCCESS,
 		})
 		pr2Run1 = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: pr2Run1.ID})
 		assert.Equal(t, actions_model.StatusSuccess, pr2Run1.Status)
+		// user2 approves the third PR's run (user4 still has no merged PR, approval still required)
+		pr3Run1Pending := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{RepoID: baseRepo.ID, TriggerUserID: user4.ID, Ref: fmt.Sprintf("refs/pull/%d/head", pr3.Index)})
+		req = NewRequest(t, "POST", fmt.Sprintf("/%s/%s/actions/runs/%d/approve", baseRepo.OwnerName, baseRepo.Name, pr3Run1Pending.ID))
+		user2Session.MakeRequest(t, req, http.StatusOK)
 		// fetch the task
 		pr3Task1 := runner.fetchTask(t)
 		_, _, pr3Run1 := getTaskAndJobAndRunByTaskID(t, pr3Task1.Id)