From edf98a2dc30956c8e04b778bb7f1ce55c14ba963 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 24 Feb 2023 15:58:49 +0800 Subject: [PATCH] Require approval to run actions for fork pull request (#22803) Currently, Gitea will run actions automatically which are triggered by fork pull request. It's a security risk, people can create a PR and modify the workflow yamls to execute a malicious script. So we should require approval for first-time contributors, which is the default strategy of a public repo on GitHub, see [Approving workflow runs from public forks](https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks). Current strategy: - don't need approval if it's not a fork PR; - always need approval if the user is restricted; - don't need approval if the user can write; - don't need approval if the user has been approved before; - otherwise, need approval. https://user-images.githubusercontent.com/9418365/217207121-badf50a8-826c-4425-bef1-d82d1979bc81.mov GitHub has an option for that, you can see that at `///settings/actions`, and we can support that later. image --------- Co-authored-by: Lunny Xiao --- models/actions/run.go | 12 +++--- models/actions/run_list.go | 8 ++++ models/actions/status.go | 4 ++ models/migrations/migrations.go | 4 ++ models/migrations/v1_20/v244.go | 22 +++++++++++ options/locale/locale_en-US.ini | 2 + routers/web/repo/actions/view.go | 49 +++++++++++++++++++++--- routers/web/web.go | 1 + services/actions/notifier_helper.go | 48 ++++++++++++++++++++++- web_src/js/components/RepoActionView.vue | 20 +++++++++- 10 files changed, 154 insertions(+), 16 deletions(-) create mode 100644 models/migrations/v1_20/v244.go diff --git a/models/actions/run.go b/models/actions/run.go index 14d191c814..a8d991471e 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -32,11 +32,13 @@ type ActionRun struct { OwnerID int64 `xorm:"index"` WorkflowID string `xorm:"index"` // the name of workflow file Index int64 `xorm:"index unique(repo_index)"` // a unique number for each run of a repository - TriggerUserID int64 - TriggerUser *user_model.User `xorm:"-"` + TriggerUserID int64 `xorm:"index"` + TriggerUser *user_model.User `xorm:"-"` Ref string CommitSHA string IsForkPullRequest bool + NeedApproval bool // may need approval if it's a fork pull request + ApprovedBy int64 `xorm:"index"` // who approved Event webhook_module.HookEventType EventPayload string `xorm:"LONGTEXT"` Status Status `xorm:"index"` @@ -164,10 +166,6 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork } run.Index = index - if run.Status.IsUnknown() { - run.Status = StatusWaiting - } - if err := db.Insert(ctx, run); err != nil { return err } @@ -191,7 +189,7 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork job.EraseNeeds() payload, _ := v.Marshal() status := StatusWaiting - if len(needs) > 0 { + if len(needs) > 0 || run.NeedApproval { status = StatusBlocked } runJobs = append(runJobs, &ActionRunJob{ diff --git a/models/actions/run_list.go b/models/actions/run_list.go index f9d8417227..bc69c65840 100644 --- a/models/actions/run_list.go +++ b/models/actions/run_list.go @@ -68,6 +68,8 @@ type FindRunOptions struct { OwnerID int64 IsClosed util.OptionalBool WorkflowFileName string + TriggerUserID int64 + Approved bool // not util.OptionalBool, it works only when it's true } func (opts FindRunOptions) toConds() builder.Cond { @@ -89,6 +91,12 @@ func (opts FindRunOptions) toConds() builder.Cond { if opts.WorkflowFileName != "" { cond = cond.And(builder.Eq{"workflow_id": opts.WorkflowFileName}) } + if opts.TriggerUserID > 0 { + cond = cond.And(builder.Eq{"trigger_user_id": opts.TriggerUserID}) + } + if opts.Approved { + cond = cond.And(builder.Gt{"approved_by": 0}) + } return cond } diff --git a/models/actions/status.go b/models/actions/status.go index 059cf9bc09..c97578f2ac 100644 --- a/models/actions/status.go +++ b/models/actions/status.go @@ -82,6 +82,10 @@ func (s Status) IsRunning() bool { return s == StatusRunning } +func (s Status) IsBlocked() bool { + return s == StatusBlocked +} + // In returns whether s is one of the given statuses func (s Status) In(statuses ...Status) bool { for _, v := range statuses { diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 989a1d6ae1..585457e474 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/models/migrations/v1_17" "code.gitea.io/gitea/models/migrations/v1_18" "code.gitea.io/gitea/models/migrations/v1_19" + "code.gitea.io/gitea/models/migrations/v1_20" "code.gitea.io/gitea/models/migrations/v1_6" "code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_8" @@ -463,6 +464,9 @@ var migrations = []Migration{ NewMigration("Add exclusive label", v1_19.AddExclusiveLabel), // Gitea 1.19.0 ends at v244 + + // v244 -> v245 + NewMigration("Add NeedApproval to actions tables", v1_20.AddNeedApprovalToActionRun), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_20/v244.go b/models/migrations/v1_20/v244.go new file mode 100644 index 0000000000..977566ad7d --- /dev/null +++ b/models/migrations/v1_20/v244.go @@ -0,0 +1,22 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_20 //nolint + +import ( + "xorm.io/xorm" +) + +func AddNeedApprovalToActionRun(x *xorm.Engine) error { + /* + New index: TriggerUserID + New fields: NeedApproval, ApprovedBy + */ + type ActionRun struct { + TriggerUserID int64 `xorm:"index"` + NeedApproval bool // may need approval if it's a fork pull request + ApprovedBy int64 `xorm:"index"` // who approved + } + + return x.Sync(new(ActionRun)) +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index df66ce2339..fbd3068053 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -3346,3 +3346,5 @@ runs.open_tab = %d Open runs.closed_tab = %d Closed runs.commit = Commit runs.pushed_by = Pushed by + +need_approval_desc = Need approval to run workflows for fork pull request. diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 5370310e8d..dd2750f905 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -49,11 +49,12 @@ type ViewRequest struct { type ViewResponse struct { State struct { Run struct { - Link string `json:"link"` - Title string `json:"title"` - CanCancel bool `json:"canCancel"` - Done bool `json:"done"` - Jobs []*ViewJob `json:"jobs"` + Link string `json:"link"` + Title string `json:"title"` + CanCancel bool `json:"canCancel"` + CanApprove bool `json:"canApprove"` // the run needs an approval and the doer has permission to approve + Done bool `json:"done"` + Jobs []*ViewJob `json:"jobs"` } `json:"run"` CurrentJob struct { Title string `json:"title"` @@ -107,6 +108,7 @@ func ViewPost(ctx *context_module.Context) { resp.State.Run.Title = run.Title resp.State.Run.Link = run.Link() resp.State.Run.CanCancel = !run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions) + resp.State.Run.CanApprove = run.NeedApproval && ctx.Repo.CanWrite(unit.TypeActions) resp.State.Run.Done = run.Status.IsDone() resp.State.Run.Jobs = make([]*ViewJob, 0, len(jobs)) // marshal to '[]' instead fo 'null' in json for _, v := range jobs { @@ -135,6 +137,9 @@ func ViewPost(ctx *context_module.Context) { resp.State.CurrentJob.Title = current.Name resp.State.CurrentJob.Detail = current.Status.LocaleString(ctx.Locale) + if run.NeedApproval { + resp.State.CurrentJob.Detail = ctx.Locale.Tr("actions.need_approval_desc") + } resp.State.CurrentJob.Steps = make([]*ViewJobStep, 0) // marshal to '[]' instead fo 'null' in json resp.Logs.StepsLog = make([]*ViewStepLog, 0) // marshal to '[]' instead fo 'null' in json if task != nil { @@ -261,6 +266,40 @@ func Cancel(ctx *context_module.Context) { ctx.JSON(http.StatusOK, struct{}{}) } +func Approve(ctx *context_module.Context) { + runIndex := ctx.ParamsInt64("run") + + current, jobs := getRunJobs(ctx, runIndex, -1) + if ctx.Written() { + return + } + run := current.Run + doer := ctx.Doer + + if err := db.WithTx(ctx, func(ctx context.Context) error { + run.NeedApproval = false + run.ApprovedBy = doer.ID + if err := actions_model.UpdateRun(ctx, run, "need_approval", "approved_by"); err != nil { + return err + } + for _, job := range jobs { + if len(job.Needs) == 0 && job.Status.IsBlocked() { + job.Status = actions_model.StatusWaiting + _, err := actions_model.UpdateRunJob(ctx, job, nil, "status") + if err != nil { + return err + } + } + } + return nil + }); err != nil { + ctx.Error(http.StatusInternalServerError, err.Error()) + return + } + + ctx.JSON(http.StatusOK, struct{}{}) +} + // getRunJobs gets the jobs of runIndex, and returns jobs[jobIndex], jobs. // Any error will be written to the ctx. // It never returns a nil job of an empty jobs, if the jobIndex is out of range, it will be treated as 0. diff --git a/routers/web/web.go b/routers/web/web.go index 88e27ad678..ff312992dd 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1286,6 +1286,7 @@ func RegisterRoutes(m *web.Route) { m.Post("/rerun", reqRepoActionsWriter, actions.Rerun) }) m.Post("/cancel", reqRepoActionsWriter, actions.Cancel) + m.Post("/approve", reqRepoActionsWriter, actions.Approve) }) }, reqRepoActionsReader, actions.MustEnableActions) diff --git a/services/actions/notifier_helper.go b/services/actions/notifier_helper.go index df67d2fa11..ef63b8cf94 100644 --- a/services/actions/notifier_helper.go +++ b/services/actions/notifier_helper.go @@ -153,7 +153,7 @@ func notify(ctx context.Context, input *notifyInput) error { } for id, content := range workflows { - run := actions_model.ActionRun{ + run := &actions_model.ActionRun{ Title: strings.SplitN(commit.CommitMessage, "\n", 2)[0], RepoID: input.Repo.ID, OwnerID: input.Repo.OwnerID, @@ -166,12 +166,19 @@ func notify(ctx context.Context, input *notifyInput) error { EventPayload: string(p), Status: actions_model.StatusWaiting, } + if need, err := ifNeedApproval(ctx, run, input.Repo, input.Doer); err != nil { + log.Error("check if need approval for repo %d with user %d: %v", input.Repo.ID, input.Doer.ID, err) + continue + } else { + run.NeedApproval = need + } + jobs, err := jobparser.Parse(content) if err != nil { log.Error("jobparser.Parse: %v", err) continue } - if err := actions_model.InsertRun(ctx, &run, jobs); err != nil { + if err := actions_model.InsertRun(ctx, run, jobs); err != nil { log.Error("InsertRun: %v", err) continue } @@ -234,3 +241,40 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo }). Notify(ctx) } + +func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *repo_model.Repository, user *user_model.User) (bool, error) { + // don't need approval if it's not a fork PR + if !run.IsForkPullRequest { + return false, nil + } + + // always need approval if the user is restricted + if user.IsRestricted { + log.Trace("need approval because user %d is restricted", user.ID) + return true, nil + } + + // don't need approval if the user can write + if perm, err := access_model.GetUserRepoPermission(ctx, repo, user); err != nil { + return false, fmt.Errorf("GetUserRepoPermission: %w", err) + } else if perm.CanWrite(unit_model.TypeActions) { + 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 := actions_model.CountRuns(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) + return false, nil + } + + // otherwise, need approval + log.Trace("need approval because it's the first time user %d triggered actions", user.ID) + return true, nil +} diff --git a/web_src/js/components/RepoActionView.vue b/web_src/js/components/RepoActionView.vue index e0ec488933..762067f523 100644 --- a/web_src/js/components/RepoActionView.vue +++ b/web_src/js/components/RepoActionView.vue @@ -3,7 +3,10 @@
{{ run.title }} - +
@@ -97,6 +100,7 @@ const sfc = { link: '', title: '', canCancel: false, + canApprove: false, done: false, jobs: [ // { @@ -173,6 +177,10 @@ const sfc = { cancelRun() { this.fetchPost(`${this.run.link}/cancel`); }, + // approve a run + approveRun() { + this.fetchPost(`${this.run.link}/approve`); + }, createLogLine(line) { const div = document.createElement('div'); @@ -303,7 +311,15 @@ export function initRepositoryActionView() { cursor: pointer; transition:transform 0.2s; }; - .run_cancel:hover{ + .run_approve { + border: none; + color: var(--color-green); + background-color: transparent; + outline: none; + cursor: pointer; + transition:transform 0.2s; + }; + .run_cancel:hover, .run_approve:hover { transform:scale(130%); }; }