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
`/<owner>/<repo>/settings/actions`, and we can support that later.

<img width="835" alt="image"
src="https://user-images.githubusercontent.com/9418365/217199990-2967e68b-e693-4e59-8186-ab33a1314a16.png">

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
This commit is contained in:
Jason Song 2023-02-24 15:58:49 +08:00 committed by GitHub
parent a6175b01d9
commit edf98a2dc3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 154 additions and 16 deletions

View file

@ -32,11 +32,13 @@ type ActionRun struct {
OwnerID int64 `xorm:"index"` OwnerID int64 `xorm:"index"`
WorkflowID string `xorm:"index"` // the name of workflow file 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 Index int64 `xorm:"index unique(repo_index)"` // a unique number for each run of a repository
TriggerUserID int64 TriggerUserID int64 `xorm:"index"`
TriggerUser *user_model.User `xorm:"-"` TriggerUser *user_model.User `xorm:"-"`
Ref string Ref string
CommitSHA string CommitSHA string
IsForkPullRequest bool IsForkPullRequest bool
NeedApproval bool // may need approval if it's a fork pull request
ApprovedBy int64 `xorm:"index"` // who approved
Event webhook_module.HookEventType Event webhook_module.HookEventType
EventPayload string `xorm:"LONGTEXT"` EventPayload string `xorm:"LONGTEXT"`
Status Status `xorm:"index"` Status Status `xorm:"index"`
@ -164,10 +166,6 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork
} }
run.Index = index run.Index = index
if run.Status.IsUnknown() {
run.Status = StatusWaiting
}
if err := db.Insert(ctx, run); err != nil { if err := db.Insert(ctx, run); err != nil {
return err return err
} }
@ -191,7 +189,7 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork
job.EraseNeeds() job.EraseNeeds()
payload, _ := v.Marshal() payload, _ := v.Marshal()
status := StatusWaiting status := StatusWaiting
if len(needs) > 0 { if len(needs) > 0 || run.NeedApproval {
status = StatusBlocked status = StatusBlocked
} }
runJobs = append(runJobs, &ActionRunJob{ runJobs = append(runJobs, &ActionRunJob{

View file

@ -68,6 +68,8 @@ type FindRunOptions struct {
OwnerID int64 OwnerID int64
IsClosed util.OptionalBool IsClosed util.OptionalBool
WorkflowFileName string WorkflowFileName string
TriggerUserID int64
Approved bool // not util.OptionalBool, it works only when it's true
} }
func (opts FindRunOptions) toConds() builder.Cond { func (opts FindRunOptions) toConds() builder.Cond {
@ -89,6 +91,12 @@ func (opts FindRunOptions) toConds() builder.Cond {
if opts.WorkflowFileName != "" { if opts.WorkflowFileName != "" {
cond = cond.And(builder.Eq{"workflow_id": 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 return cond
} }

View file

@ -82,6 +82,10 @@ func (s Status) IsRunning() bool {
return s == StatusRunning return s == StatusRunning
} }
func (s Status) IsBlocked() bool {
return s == StatusBlocked
}
// In returns whether s is one of the given statuses // In returns whether s is one of the given statuses
func (s Status) In(statuses ...Status) bool { func (s Status) In(statuses ...Status) bool {
for _, v := range statuses { for _, v := range statuses {

View file

@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/models/migrations/v1_17" "code.gitea.io/gitea/models/migrations/v1_17"
"code.gitea.io/gitea/models/migrations/v1_18" "code.gitea.io/gitea/models/migrations/v1_18"
"code.gitea.io/gitea/models/migrations/v1_19" "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_6"
"code.gitea.io/gitea/models/migrations/v1_7" "code.gitea.io/gitea/models/migrations/v1_7"
"code.gitea.io/gitea/models/migrations/v1_8" "code.gitea.io/gitea/models/migrations/v1_8"
@ -463,6 +464,9 @@ var migrations = []Migration{
NewMigration("Add exclusive label", v1_19.AddExclusiveLabel), NewMigration("Add exclusive label", v1_19.AddExclusiveLabel),
// Gitea 1.19.0 ends at v244 // Gitea 1.19.0 ends at v244
// v244 -> v245
NewMigration("Add NeedApproval to actions tables", v1_20.AddNeedApprovalToActionRun),
} }
// GetCurrentDBVersion returns the current db version // GetCurrentDBVersion returns the current db version

View file

@ -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))
}

View file

@ -3346,3 +3346,5 @@ runs.open_tab = %d Open
runs.closed_tab = %d Closed runs.closed_tab = %d Closed
runs.commit = Commit runs.commit = Commit
runs.pushed_by = Pushed by runs.pushed_by = Pushed by
need_approval_desc = Need approval to run workflows for fork pull request.

View file

@ -49,11 +49,12 @@ type ViewRequest struct {
type ViewResponse struct { type ViewResponse struct {
State struct { State struct {
Run struct { Run struct {
Link string `json:"link"` Link string `json:"link"`
Title string `json:"title"` Title string `json:"title"`
CanCancel bool `json:"canCancel"` CanCancel bool `json:"canCancel"`
Done bool `json:"done"` CanApprove bool `json:"canApprove"` // the run needs an approval and the doer has permission to approve
Jobs []*ViewJob `json:"jobs"` Done bool `json:"done"`
Jobs []*ViewJob `json:"jobs"`
} `json:"run"` } `json:"run"`
CurrentJob struct { CurrentJob struct {
Title string `json:"title"` Title string `json:"title"`
@ -107,6 +108,7 @@ func ViewPost(ctx *context_module.Context) {
resp.State.Run.Title = run.Title resp.State.Run.Title = run.Title
resp.State.Run.Link = run.Link() resp.State.Run.Link = run.Link()
resp.State.Run.CanCancel = !run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions) 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.Done = run.Status.IsDone()
resp.State.Run.Jobs = make([]*ViewJob, 0, len(jobs)) // marshal to '[]' instead fo 'null' in json resp.State.Run.Jobs = make([]*ViewJob, 0, len(jobs)) // marshal to '[]' instead fo 'null' in json
for _, v := range jobs { for _, v := range jobs {
@ -135,6 +137,9 @@ func ViewPost(ctx *context_module.Context) {
resp.State.CurrentJob.Title = current.Name resp.State.CurrentJob.Title = current.Name
resp.State.CurrentJob.Detail = current.Status.LocaleString(ctx.Locale) 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.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 resp.Logs.StepsLog = make([]*ViewStepLog, 0) // marshal to '[]' instead fo 'null' in json
if task != nil { if task != nil {
@ -261,6 +266,40 @@ func Cancel(ctx *context_module.Context) {
ctx.JSON(http.StatusOK, struct{}{}) 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. // getRunJobs gets the jobs of runIndex, and returns jobs[jobIndex], jobs.
// Any error will be written to the ctx. // 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. // It never returns a nil job of an empty jobs, if the jobIndex is out of range, it will be treated as 0.

View file

@ -1286,6 +1286,7 @@ func RegisterRoutes(m *web.Route) {
m.Post("/rerun", reqRepoActionsWriter, actions.Rerun) m.Post("/rerun", reqRepoActionsWriter, actions.Rerun)
}) })
m.Post("/cancel", reqRepoActionsWriter, actions.Cancel) m.Post("/cancel", reqRepoActionsWriter, actions.Cancel)
m.Post("/approve", reqRepoActionsWriter, actions.Approve)
}) })
}, reqRepoActionsReader, actions.MustEnableActions) }, reqRepoActionsReader, actions.MustEnableActions)

View file

@ -153,7 +153,7 @@ func notify(ctx context.Context, input *notifyInput) error {
} }
for id, content := range workflows { for id, content := range workflows {
run := actions_model.ActionRun{ run := &actions_model.ActionRun{
Title: strings.SplitN(commit.CommitMessage, "\n", 2)[0], Title: strings.SplitN(commit.CommitMessage, "\n", 2)[0],
RepoID: input.Repo.ID, RepoID: input.Repo.ID,
OwnerID: input.Repo.OwnerID, OwnerID: input.Repo.OwnerID,
@ -166,12 +166,19 @@ func notify(ctx context.Context, input *notifyInput) error {
EventPayload: string(p), EventPayload: string(p),
Status: actions_model.StatusWaiting, 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) jobs, err := jobparser.Parse(content)
if err != nil { if err != nil {
log.Error("jobparser.Parse: %v", err) log.Error("jobparser.Parse: %v", err)
continue 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) log.Error("InsertRun: %v", err)
continue continue
} }
@ -234,3 +241,40 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo
}). }).
Notify(ctx) 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
}

View file

@ -3,7 +3,10 @@
<div class="action-view-header"> <div class="action-view-header">
<div class="action-info-summary"> <div class="action-info-summary">
{{ run.title }} {{ run.title }}
<button class="run_cancel" @click="cancelRun()" v-if="run.canCancel"> <button class="run_approve" @click="approveRun()" v-if="run.canApprove">
<i class="play circle outline icon"/>
</button>
<button class="run_cancel" @click="cancelRun()" v-else-if="run.canCancel">
<i class="stop circle outline icon"/> <i class="stop circle outline icon"/>
</button> </button>
</div> </div>
@ -97,6 +100,7 @@ const sfc = {
link: '', link: '',
title: '', title: '',
canCancel: false, canCancel: false,
canApprove: false,
done: false, done: false,
jobs: [ jobs: [
// { // {
@ -173,6 +177,10 @@ const sfc = {
cancelRun() { cancelRun() {
this.fetchPost(`${this.run.link}/cancel`); this.fetchPost(`${this.run.link}/cancel`);
}, },
// approve a run
approveRun() {
this.fetchPost(`${this.run.link}/approve`);
},
createLogLine(line) { createLogLine(line) {
const div = document.createElement('div'); const div = document.createElement('div');
@ -303,7 +311,15 @@ export function initRepositoryActionView() {
cursor: pointer; cursor: pointer;
transition:transform 0.2s; 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%); transform:scale(130%);
}; };
} }