Skip to content

Commit edf98a2

Browse files
wolfogrelunny
wolfogre
and
lunny
authored
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 <[email protected]>
1 parent a6175b0 commit edf98a2

File tree

10 files changed

+154
-16
lines changed

10 files changed

+154
-16
lines changed

‎models/actions/run.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,13 @@ type ActionRun struct {
3232
OwnerID int64 `xorm:"index"`
3333
WorkflowID string `xorm:"index"` // the name of workflow file
3434
Index int64 `xorm:"index unique(repo_index)"` // a unique number for each run of a repository
35-
TriggerUserID int64
36-
TriggerUser *user_model.User `xorm:"-"`
35+
TriggerUserID int64 `xorm:"index"`
36+
TriggerUser *user_model.User `xorm:"-"`
3737
Ref string
3838
CommitSHA string
3939
IsForkPullRequest bool
40+
NeedApproval bool // may need approval if it's a fork pull request
41+
ApprovedBy int64 `xorm:"index"` // who approved
4042
Event webhook_module.HookEventType
4143
EventPayload string `xorm:"LONGTEXT"`
4244
Status Status `xorm:"index"`
@@ -164,10 +166,6 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork
164166
}
165167
run.Index = index
166168

167-
if run.Status.IsUnknown() {
168-
run.Status = StatusWaiting
169-
}
170-
171169
if err := db.Insert(ctx, run); err != nil {
172170
return err
173171
}
@@ -191,7 +189,7 @@ func InsertRun(ctx context.Context, run *ActionRun, jobs []*jobparser.SingleWork
191189
job.EraseNeeds()
192190
payload, _ := v.Marshal()
193191
status := StatusWaiting
194-
if len(needs) > 0 {
192+
if len(needs) > 0 || run.NeedApproval {
195193
status = StatusBlocked
196194
}
197195
runJobs = append(runJobs, &ActionRunJob{

‎models/actions/run_list.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ type FindRunOptions struct {
6868
OwnerID int64
6969
IsClosed util.OptionalBool
7070
WorkflowFileName string
71+
TriggerUserID int64
72+
Approved bool // not util.OptionalBool, it works only when it's true
7173
}
7274

7375
func (opts FindRunOptions) toConds() builder.Cond {
@@ -89,6 +91,12 @@ func (opts FindRunOptions) toConds() builder.Cond {
8991
if opts.WorkflowFileName != "" {
9092
cond = cond.And(builder.Eq{"workflow_id": opts.WorkflowFileName})
9193
}
94+
if opts.TriggerUserID > 0 {
95+
cond = cond.And(builder.Eq{"trigger_user_id": opts.TriggerUserID})
96+
}
97+
if opts.Approved {
98+
cond = cond.And(builder.Gt{"approved_by": 0})
99+
}
92100
return cond
93101
}
94102

‎models/actions/status.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,10 @@ func (s Status) IsRunning() bool {
8282
return s == StatusRunning
8383
}
8484

85+
func (s Status) IsBlocked() bool {
86+
return s == StatusBlocked
87+
}
88+
8589
// In returns whether s is one of the given statuses
8690
func (s Status) In(statuses ...Status) bool {
8791
for _, v := range statuses {

‎models/migrations/migrations.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"code.gitea.io/gitea/models/migrations/v1_17"
2020
"code.gitea.io/gitea/models/migrations/v1_18"
2121
"code.gitea.io/gitea/models/migrations/v1_19"
22+
"code.gitea.io/gitea/models/migrations/v1_20"
2223
"code.gitea.io/gitea/models/migrations/v1_6"
2324
"code.gitea.io/gitea/models/migrations/v1_7"
2425
"code.gitea.io/gitea/models/migrations/v1_8"
@@ -463,6 +464,9 @@ var migrations = []Migration{
463464
NewMigration("Add exclusive label", v1_19.AddExclusiveLabel),
464465

465466
// Gitea 1.19.0 ends at v244
467+
468+
// v244 -> v245
469+
NewMigration("Add NeedApproval to actions tables", v1_20.AddNeedApprovalToActionRun),
466470
}
467471

468472
// GetCurrentDBVersion returns the current db version

‎models/migrations/v1_20/v244.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright 2023 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package v1_20 //nolint
5+
6+
import (
7+
"xorm.io/xorm"
8+
)
9+
10+
func AddNeedApprovalToActionRun(x *xorm.Engine) error {
11+
/*
12+
New index: TriggerUserID
13+
New fields: NeedApproval, ApprovedBy
14+
*/
15+
type ActionRun struct {
16+
TriggerUserID int64 `xorm:"index"`
17+
NeedApproval bool // may need approval if it's a fork pull request
18+
ApprovedBy int64 `xorm:"index"` // who approved
19+
}
20+
21+
return x.Sync(new(ActionRun))
22+
}

‎options/locale/locale_en-US.ini

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3346,3 +3346,5 @@ runs.open_tab = %d Open
33463346
runs.closed_tab = %d Closed
33473347
runs.commit = Commit
33483348
runs.pushed_by = Pushed by
3349+
3350+
need_approval_desc = Need approval to run workflows for fork pull request.

‎routers/web/repo/actions/view.go

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,12 @@ type ViewRequest struct {
4949
type ViewResponse struct {
5050
State struct {
5151
Run struct {
52-
Link string `json:"link"`
53-
Title string `json:"title"`
54-
CanCancel bool `json:"canCancel"`
55-
Done bool `json:"done"`
56-
Jobs []*ViewJob `json:"jobs"`
52+
Link string `json:"link"`
53+
Title string `json:"title"`
54+
CanCancel bool `json:"canCancel"`
55+
CanApprove bool `json:"canApprove"` // the run needs an approval and the doer has permission to approve
56+
Done bool `json:"done"`
57+
Jobs []*ViewJob `json:"jobs"`
5758
} `json:"run"`
5859
CurrentJob struct {
5960
Title string `json:"title"`
@@ -107,6 +108,7 @@ func ViewPost(ctx *context_module.Context) {
107108
resp.State.Run.Title = run.Title
108109
resp.State.Run.Link = run.Link()
109110
resp.State.Run.CanCancel = !run.Status.IsDone() && ctx.Repo.CanWrite(unit.TypeActions)
111+
resp.State.Run.CanApprove = run.NeedApproval && ctx.Repo.CanWrite(unit.TypeActions)
110112
resp.State.Run.Done = run.Status.IsDone()
111113
resp.State.Run.Jobs = make([]*ViewJob, 0, len(jobs)) // marshal to '[]' instead fo 'null' in json
112114
for _, v := range jobs {
@@ -135,6 +137,9 @@ func ViewPost(ctx *context_module.Context) {
135137

136138
resp.State.CurrentJob.Title = current.Name
137139
resp.State.CurrentJob.Detail = current.Status.LocaleString(ctx.Locale)
140+
if run.NeedApproval {
141+
resp.State.CurrentJob.Detail = ctx.Locale.Tr("actions.need_approval_desc")
142+
}
138143
resp.State.CurrentJob.Steps = make([]*ViewJobStep, 0) // marshal to '[]' instead fo 'null' in json
139144
resp.Logs.StepsLog = make([]*ViewStepLog, 0) // marshal to '[]' instead fo 'null' in json
140145
if task != nil {
@@ -261,6 +266,40 @@ func Cancel(ctx *context_module.Context) {
261266
ctx.JSON(http.StatusOK, struct{}{})
262267
}
263268

269+
func Approve(ctx *context_module.Context) {
270+
runIndex := ctx.ParamsInt64("run")
271+
272+
current, jobs := getRunJobs(ctx, runIndex, -1)
273+
if ctx.Written() {
274+
return
275+
}
276+
run := current.Run
277+
doer := ctx.Doer
278+
279+
if err := db.WithTx(ctx, func(ctx context.Context) error {
280+
run.NeedApproval = false
281+
run.ApprovedBy = doer.ID
282+
if err := actions_model.UpdateRun(ctx, run, "need_approval", "approved_by"); err != nil {
283+
return err
284+
}
285+
for _, job := range jobs {
286+
if len(job.Needs) == 0 && job.Status.IsBlocked() {
287+
job.Status = actions_model.StatusWaiting
288+
_, err := actions_model.UpdateRunJob(ctx, job, nil, "status")
289+
if err != nil {
290+
return err
291+
}
292+
}
293+
}
294+
return nil
295+
}); err != nil {
296+
ctx.Error(http.StatusInternalServerError, err.Error())
297+
return
298+
}
299+
300+
ctx.JSON(http.StatusOK, struct{}{})
301+
}
302+
264303
// getRunJobs gets the jobs of runIndex, and returns jobs[jobIndex], jobs.
265304
// Any error will be written to the ctx.
266305
// It never returns a nil job of an empty jobs, if the jobIndex is out of range, it will be treated as 0.

‎routers/web/web.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1286,6 +1286,7 @@ func RegisterRoutes(m *web.Route) {
12861286
m.Post("/rerun", reqRepoActionsWriter, actions.Rerun)
12871287
})
12881288
m.Post("/cancel", reqRepoActionsWriter, actions.Cancel)
1289+
m.Post("/approve", reqRepoActionsWriter, actions.Approve)
12891290
})
12901291
}, reqRepoActionsReader, actions.MustEnableActions)
12911292

‎services/actions/notifier_helper.go

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ func notify(ctx context.Context, input *notifyInput) error {
153153
}
154154

155155
for id, content := range workflows {
156-
run := actions_model.ActionRun{
156+
run := &actions_model.ActionRun{
157157
Title: strings.SplitN(commit.CommitMessage, "\n", 2)[0],
158158
RepoID: input.Repo.ID,
159159
OwnerID: input.Repo.OwnerID,
@@ -166,12 +166,19 @@ func notify(ctx context.Context, input *notifyInput) error {
166166
EventPayload: string(p),
167167
Status: actions_model.StatusWaiting,
168168
}
169+
if need, err := ifNeedApproval(ctx, run, input.Repo, input.Doer); err != nil {
170+
log.Error("check if need approval for repo %d with user %d: %v", input.Repo.ID, input.Doer.ID, err)
171+
continue
172+
} else {
173+
run.NeedApproval = need
174+
}
175+
169176
jobs, err := jobparser.Parse(content)
170177
if err != nil {
171178
log.Error("jobparser.Parse: %v", err)
172179
continue
173180
}
174-
if err := actions_model.InsertRun(ctx, &run, jobs); err != nil {
181+
if err := actions_model.InsertRun(ctx, run, jobs); err != nil {
175182
log.Error("InsertRun: %v", err)
176183
continue
177184
}
@@ -234,3 +241,40 @@ func notifyPackage(ctx context.Context, sender *user_model.User, pd *packages_mo
234241
}).
235242
Notify(ctx)
236243
}
244+
245+
func ifNeedApproval(ctx context.Context, run *actions_model.ActionRun, repo *repo_model.Repository, user *user_model.User) (bool, error) {
246+
// don't need approval if it's not a fork PR
247+
if !run.IsForkPullRequest {
248+
return false, nil
249+
}
250+
251+
// always need approval if the user is restricted
252+
if user.IsRestricted {
253+
log.Trace("need approval because user %d is restricted", user.ID)
254+
return true, nil
255+
}
256+
257+
// don't need approval if the user can write
258+
if perm, err := access_model.GetUserRepoPermission(ctx, repo, user); err != nil {
259+
return false, fmt.Errorf("GetUserRepoPermission: %w", err)
260+
} else if perm.CanWrite(unit_model.TypeActions) {
261+
log.Trace("do not need approval because user %d can write", user.ID)
262+
return false, nil
263+
}
264+
265+
// don't need approval if the user has been approved before
266+
if count, err := actions_model.CountRuns(ctx, actions_model.FindRunOptions{
267+
RepoID: repo.ID,
268+
TriggerUserID: user.ID,
269+
Approved: true,
270+
}); err != nil {
271+
return false, fmt.Errorf("CountRuns: %w", err)
272+
} else if count > 0 {
273+
log.Trace("do not need approval because user %d has been approved before", user.ID)
274+
return false, nil
275+
}
276+
277+
// otherwise, need approval
278+
log.Trace("need approval because it's the first time user %d triggered actions", user.ID)
279+
return true, nil
280+
}

‎web_src/js/components/RepoActionView.vue

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33
<div class="action-view-header">
44
<div class="action-info-summary">
55
{{ run.title }}
6-
<button class="run_cancel" @click="cancelRun()" v-if="run.canCancel">
6+
<button class="run_approve" @click="approveRun()" v-if="run.canApprove">
7+
<i class="play circle outline icon"/>
8+
</button>
9+
<button class="run_cancel" @click="cancelRun()" v-else-if="run.canCancel">
710
<i class="stop circle outline icon"/>
811
</button>
912
</div>
@@ -97,6 +100,7 @@ const sfc = {
97100
link: '',
98101
title: '',
99102
canCancel: false,
103+
canApprove: false,
100104
done: false,
101105
jobs: [
102106
// {
@@ -173,6 +177,10 @@ const sfc = {
173177
cancelRun() {
174178
this.fetchPost(`${this.run.link}/cancel`);
175179
},
180+
// approve a run
181+
approveRun() {
182+
this.fetchPost(`${this.run.link}/approve`);
183+
},
176184
177185
createLogLine(line) {
178186
const div = document.createElement('div');
@@ -303,7 +311,15 @@ export function initRepositoryActionView() {
303311
cursor: pointer;
304312
transition:transform 0.2s;
305313
};
306-
.run_cancel:hover{
314+
.run_approve {
315+
border: none;
316+
color: var(--color-green);
317+
background-color: transparent;
318+
outline: none;
319+
cursor: pointer;
320+
transition:transform 0.2s;
321+
};
322+
.run_cancel:hover, .run_approve:hover {
307323
transform:scale(130%);
308324
};
309325
}

0 commit comments

Comments
 (0)