diff --git a/models/actions/run_list.go b/models/actions/run_list.go index 2628c4712f..64a59b5a75 100644 --- a/models/actions/run_list.go +++ b/models/actions/run_list.go @@ -70,7 +70,6 @@ type FindRunOptions struct { Ref string // the commit/tag/… that caused this workflow TriggerUserID int64 TriggerEvent webhook_module.HookEventType - Approved bool // not util.OptionalBool, it works only when it's true Status []Status ConcurrencyGroup string CommitSHA string @@ -87,9 +86,6 @@ func (opts FindRunOptions) ToConds() builder.Cond { if opts.TriggerUserID > 0 { cond = cond.And(builder.Eq{"`action_run`.trigger_user_id": opts.TriggerUserID}) } - if opts.Approved { - cond = cond.And(builder.Gt{"`action_run`.approved_by": 0}) - } if len(opts.Status) > 0 { cond = cond.And(builder.In("`action_run`.status", opts.Status)) } diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index 64905744e1..a867324a89 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -399,6 +399,24 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo } func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *repo_model.Repository, user *user_model.User) (bool, error) { + canWrite := func(ctx context.Context, repo *repo_model.Repository, user *user_model.User) (bool, error) { + perm, err := access_model.GetDoerRepoPermission(ctx, repo, user) + if err != nil { + return false, err + } + return perm.CanWrite(unit_model.TypeActions), nil + } + return ifNeedApprovalWith(ctx, run, repo, user, canWrite, issues_model.HasMergedPullRequestInRepo) +} + +func ifNeedApprovalWith( + ctx context.Context, + run *actions_model.ActionRun, + repo *repo_model.Repository, + user *user_model.User, + canWriteActions func(context.Context, *repo_model.Repository, *user_model.User) (bool, error), + hasMergedPR func(context.Context, int64, int64) (bool, error), +) (bool, error) { // 1. don't need approval if it's not a fork PR // 2. don't need approval if the event is `pull_request_target` since the workflow will run in the context of base branch // see https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks @@ -413,27 +431,24 @@ func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *rep } // don't need approval if the user can write - if perm, err := access_model.GetDoerRepoPermission(ctx, repo, user); err != nil { + if ok, err := canWriteActions(ctx, repo, user); err != nil { return false, fmt.Errorf("GetDoerRepoPermission: %w", err) - } else if perm.CanWrite(unit_model.TypeActions) { + } else if ok { log.Trace("do not need approval because user %d can write", user.ID) return false, nil } - // don't need approval if the user has been approved before - if count, err := db.Count[actions_model.ActionRun](ctx, actions_model.FindRunOptions{ - RepoID: repo.ID, - TriggerUserID: user.ID, - Approved: true, - }); err != nil { - return false, fmt.Errorf("CountRuns: %w", err) - } else if count > 0 { - log.Trace("do not need approval because user %d has been approved before", user.ID) + // trust the user only after a merged PR — matching GitHub Actions. Approving one + // fork PR's run must not implicitly trust later fork PRs that replace the workflow. + if merged, err := hasMergedPR(ctx, repo.ID, user.ID); err != nil { + return false, fmt.Errorf("HasMergedPullRequestInRepo: %w", err) + } else if merged { + log.Trace("do not need approval because user %d has a merged pull request in repo %d", user.ID, repo.ID) return false, nil } // otherwise, need approval - log.Trace("need approval because it's the first time user %d triggered actions", user.ID) + log.Trace("need approval because user %d has no merged pull request in repo %d", user.ID, repo.ID) return true, nil } diff --git a/services/actions/notifier_helper_test.go b/services/actions/notifier_helper_test.go new file mode 100644 index 0000000000..bb9fcc0cdb --- /dev/null +++ b/services/actions/notifier_helper_test.go @@ -0,0 +1,102 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "context" + "errors" + "testing" + + actions_model "code.gitea.io/gitea/models/actions" + repo_model "code.gitea.io/gitea/models/repo" + user_model "code.gitea.io/gitea/models/user" + actions_module "code.gitea.io/gitea/modules/actions" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIfNeedApproval(t *testing.T) { + alwaysWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + return true, nil + } + neverWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + return false, nil + } + hasMerged := func(_ context.Context, _, _ int64) (bool, error) { return true, nil } + noMerged := func(_ context.Context, _, _ int64) (bool, error) { return false, nil } + errPerm := errors.New("perm error") + errMerge := errors.New("merge error") + + forkRun := &actions_model.ActionRun{IsForkPullRequest: true, TriggerEvent: actions_module.GithubEventPullRequest} + nonForkRun := &actions_model.ActionRun{IsForkPullRequest: false, TriggerEvent: actions_module.GithubEventPullRequest} + prTargetRun := &actions_model.ActionRun{IsForkPullRequest: true, TriggerEvent: actions_module.GithubEventPullRequestTarget} + + repo := &repo_model.Repository{ID: 1} + normalUser := &user_model.User{ID: 10} + restrictedUser := &user_model.User{ID: 11, IsRestricted: true} + + t.Run("not a fork PR never needs approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), nonForkRun, repo, normalUser, alwaysWrite, hasMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("pull_request_target never needs approval even when fork", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), prTargetRun, repo, normalUser, alwaysWrite, hasMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("restricted user always needs approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, restrictedUser, alwaysWrite, hasMerged) + require.NoError(t, err) + assert.True(t, need) + }) + + t.Run("fork PR with write permission does not need approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, alwaysWrite, noMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("fork PR with merged PR but no write permission does not need approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, neverWrite, hasMerged) + require.NoError(t, err) + assert.False(t, need) + }) + + t.Run("fork PR with no write and no merged PR needs approval", func(t *testing.T) { + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, neverWrite, noMerged) + require.NoError(t, err) + assert.True(t, need) + }) + + t.Run("canWriteActions error is propagated", func(t *testing.T) { + failWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + return false, errPerm + } + _, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, failWrite, noMerged) + require.ErrorIs(t, err, errPerm) + }) + + t.Run("hasMergedPR error is propagated", func(t *testing.T) { + failMerge := func(_ context.Context, _, _ int64) (bool, error) { return false, errMerge } + _, err := ifNeedApprovalWith(t.Context(), forkRun, repo, normalUser, neverWrite, failMerge) + require.ErrorIs(t, err, errMerge) + }) + + t.Run("restricted user skips permission check entirely", func(t *testing.T) { + // The perm and merge functions must not be called for a restricted user. + called := false + trackWrite := func(_ context.Context, _ *repo_model.Repository, _ *user_model.User) (bool, error) { + called = true + return true, nil + } + need, err := ifNeedApprovalWith(t.Context(), forkRun, repo, restrictedUser, trackWrite, noMerged) + require.NoError(t, err) + assert.True(t, need) + assert.False(t, called, "permission check must not run for restricted user") + }) +} diff --git a/tests/integration/actions_approve_test.go b/tests/integration/actions_approve_test.go index 3f2c02f77a..41b7eac462 100644 --- a/tests/integration/actions_approve_test.go +++ b/tests/integration/actions_approve_test.go @@ -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") + }) +} diff --git a/tests/integration/actions_concurrency_test.go b/tests/integration/actions_concurrency_test.go index 07deecd566..2145d2e01b 100644 --- a/tests/integration/actions_concurrency_test.go +++ b/tests/integration/actions_concurrency_test.go @@ -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)