diff --git a/cli/repo/repo_info.go b/cli/repo/repo_info.go index e92d55be1..648766940 100644 --- a/cli/repo/repo_info.go +++ b/cli/repo/repo_info.go @@ -65,6 +65,7 @@ Visibility: {{ .Visibility }} Private: {{ .IsSCMPrivate }} Trusted: {{ .IsTrusted }} Gated: {{ .IsGated }} +Require approval for: {{ .RequireApproval }} Clone url: {{ .Clone }} Allow pull-requests: {{ .AllowPullRequests }} ` diff --git a/cli/repo/repo_update.go b/cli/repo/repo_update.go index 049bf0242..c5fe2a232 100644 --- a/cli/repo/repo_update.go +++ b/cli/repo/repo_update.go @@ -39,6 +39,10 @@ var repoUpdateCmd = &cli.Command{ Name: "gated", Usage: "repository is gated", }, + &cli.StringFlag{ + Name: "require-approval", + Usage: "repository requires approval for", + }, &cli.DurationFlag{ Name: "timeout", Usage: "repository timeout", @@ -79,6 +83,7 @@ func repoUpdate(ctx context.Context, c *cli.Command) error { timeout = c.Duration("timeout") trusted = c.Bool("trusted") gated = c.Bool("gated") + requireApproval = c.String("require-approval") pipelineCounter = int(c.Int("pipeline-counter")) unsafe = c.Bool("unsafe") ) @@ -87,8 +92,29 @@ func repoUpdate(ctx context.Context, c *cli.Command) error { if c.IsSet("trusted") { patch.IsTrusted = &trusted } + // TODO: remove isGated in next major release if c.IsSet("gated") { - patch.IsGated = &gated + if gated { + patch.RequireApproval = &woodpecker.RequireApprovalAllEvents + } else { + patch.RequireApproval = &woodpecker.RequireApprovalNone + } + } + if c.IsSet("require-approval") { + if mode := woodpecker.ApprovalMode(requireApproval); mode.Valid() { + patch.RequireApproval = &mode + } else { + return fmt.Errorf("update approval mode failed: '%s' is no valid mode", mode) + } + + // TODO: remove isGated in next major release + if requireApproval == string(woodpecker.RequireApprovalAllEvents) { + trueBool := true + patch.IsGated = &trueBool + } else if requireApproval == string(woodpecker.RequireApprovalNone) { + falseBool := false + patch.IsGated = &falseBool + } } if c.IsSet("timeout") { v := int64(timeout / time.Minute) diff --git a/cmd/server/docs/docs.go b/cmd/server/docs/docs.go index 3113408cc..aa0399483 100644 --- a/cmd/server/docs/docs.go +++ b/cmd/server/docs/docs.go @@ -4671,6 +4671,9 @@ const docTemplate = `{ "forge_url": { "type": "string" }, + "from_fork": { + "type": "boolean" + }, "id": { "type": "integer" }, @@ -4861,15 +4864,15 @@ const docTemplate = `{ "private": { "type": "boolean" }, + "require_approval": { + "$ref": "#/definitions/model.ApprovalMode" + }, "scm": { "$ref": "#/definitions/SCMKind" }, "timeout": { "type": "integer" }, - "trusted": { - "type": "boolean" - }, "visibility": { "$ref": "#/definitions/RepoVisibility" } @@ -4899,12 +4902,12 @@ const docTemplate = `{ "netrc_only_trusted": { "type": "boolean" }, + "require_approval": { + "type": "string" + }, "timeout": { "type": "integer" }, - "trusted": { - "type": "boolean" - }, "visibility": { "type": "string" } @@ -5163,6 +5166,27 @@ const docTemplate = `{ "EventManual" ] }, + "model.ApprovalMode": { + "type": "string", + "enum": [ + "none", + "forks", + "pull_requests", + "all_events" + ], + "x-enum-comments": { + "RequireApprovalAllEvents": "require approval for all external events", + "RequireApprovalForks": "require approval for PRs from forks (default)", + "RequireApprovalNone": "require approval for no events", + "RequireApprovalPullRequests": "require approval for all PRs" + }, + "x-enum-varnames": [ + "RequireApprovalNone", + "RequireApprovalForks", + "RequireApprovalPullRequests", + "RequireApprovalAllEvents" + ] + }, "model.ForgeType": { "type": "string", "enum": [ diff --git a/docs/docs/20-usage/75-project-settings.md b/docs/docs/20-usage/75-project-settings.md index 24bdbe605..94a49e40f 100644 --- a/docs/docs/20-usage/75-project-settings.md +++ b/docs/docs/20-usage/75-project-settings.md @@ -25,10 +25,9 @@ Only activate this option if you trust all users who have push access to your re Otherwise, these users will be able to steal secrets that are only available for `deploy` events. ::: -## Protected +## Require approval for -Every pipeline initiated by an webhook event needs to be approved by a project members with push permissions before being executed. -The protected option can be used as an additional review process before running potentially harmful pipelines. Especially if pipelines can be executed by third-parties through pull-requests. +To prevent malicious pipelines from extracting secrets or running harmful commands or to prevent accidental pipeline runs, you can require approval for an additional review process. Depending on the enabled option, a pipeline will be put on hold after creation and will only continue after approval. The default restrictive setting is `Approvals for forked repositories`. ## Trusted diff --git a/docs/docs/20-usage/project-settings.png b/docs/docs/20-usage/project-settings.png index fc29daac8..f3ce025f2 100644 Binary files a/docs/docs/20-usage/project-settings.png and b/docs/docs/20-usage/project-settings.png differ diff --git a/server/api/repo.go b/server/api/repo.go index b5cefa699..ad34ed357 100644 --- a/server/api/repo.go +++ b/server/api/repo.go @@ -90,6 +90,7 @@ func PostRepo(c *gin.Context) { repo.Update(from) } else { repo = from + repo.RequireApproval = model.RequireApprovalForks repo.AllowPull = true repo.AllowDeploy = false repo.NetrcOnlyTrusted = true @@ -236,8 +237,20 @@ func PatchRepo(c *gin.Context) { if in.AllowDeploy != nil { repo.AllowDeploy = *in.AllowDeploy } - if in.IsGated != nil { - repo.IsGated = *in.IsGated + + if in.RequireApproval != nil { + if mode := model.ApprovalMode(*in.RequireApproval); mode.Valid() { + repo.RequireApproval = mode + } else { + c.String(http.StatusBadRequest, "Invalid require-approval setting") + return + } + } else if in.IsGated != nil { // TODO: remove isGated in next major release + if *in.IsGated { + repo.RequireApproval = model.RequireApprovalAllEvents + } else { + repo.RequireApproval = model.RequireApprovalForks + } } if in.IsTrusted != nil { repo.IsTrusted = *in.IsTrusted diff --git a/server/forge/bitbucket/convert.go b/server/forge/bitbucket/convert.go index a3b1376b7..431c24086 100644 --- a/server/forge/bitbucket/convert.go +++ b/server/forge/bitbucket/convert.go @@ -183,6 +183,7 @@ func convertPullHook(from *internal.PullRequestHook) *model.Pipeline { Author: from.Actor.Login, Sender: from.Actor.Login, Timestamp: from.PullRequest.Updated.UTC().Unix(), + FromFork: from.PullRequest.Source.Repo.UUID != from.PullRequest.Dest.Repo.UUID, } if from.PullRequest.State == stateClosed { diff --git a/server/forge/bitbucketdatacenter/convert.go b/server/forge/bitbucketdatacenter/convert.go index bef0c30d5..7a74b9cfb 100644 --- a/server/forge/bitbucketdatacenter/convert.go +++ b/server/forge/bitbucketdatacenter/convert.go @@ -123,6 +123,7 @@ func convertPullRequestEvent(ev *bb.PullRequestEvent, baseURL string) *model.Pip Ref: fmt.Sprintf("refs/pull-requests/%d/from", ev.PullRequest.ID), ForgeURL: fmt.Sprintf("%s/projects/%s/repos/%s/commits/%s", baseURL, ev.PullRequest.Source.Repository.Project.Key, ev.PullRequest.Source.Repository.Slug, ev.PullRequest.Source.Latest), Refspec: fmt.Sprintf("%s:%s", ev.PullRequest.Source.DisplayID, ev.PullRequest.Target.DisplayID), + FromFork: ev.PullRequest.Source.Repository.ID != ev.PullRequest.Target.Repository.ID, } if ev.EventKey == bb.EventKeyPullRequestMerged || ev.EventKey == bb.EventKeyPullRequestDeclined || ev.EventKey == bb.EventKeyPullRequestDeleted { diff --git a/server/forge/forgejo/helper.go b/server/forge/forgejo/helper.go index 0ab2b24e0..b6e4a2785 100644 --- a/server/forge/forgejo/helper.go +++ b/server/forge/forgejo/helper.go @@ -171,6 +171,7 @@ func pipelineFromPullRequest(hook *pullRequestHook) *model.Pipeline { hook.PullRequest.Base.Ref, ), PullRequestLabels: convertLabels(hook.PullRequest.Labels), + FromFork: hook.PullRequest.Head.RepoID != hook.PullRequest.Base.RepoID, } return pipeline diff --git a/server/forge/gitea/helper.go b/server/forge/gitea/helper.go index 69b2a885b..56ec0953e 100644 --- a/server/forge/gitea/helper.go +++ b/server/forge/gitea/helper.go @@ -172,6 +172,7 @@ func pipelineFromPullRequest(hook *pullRequestHook) *model.Pipeline { hook.PullRequest.Base.Ref, ), PullRequestLabels: convertLabels(hook.PullRequest.Labels), + FromFork: hook.PullRequest.Head.RepoID != hook.PullRequest.Base.RepoID, } return pipeline diff --git a/server/forge/github/parse.go b/server/forge/github/parse.go index 95702f376..649a6df49 100644 --- a/server/forge/github/parse.go +++ b/server/forge/github/parse.go @@ -157,6 +157,8 @@ func parsePullHook(hook *github.PullRequestEvent, merge bool) (*github.PullReque event = model.EventPullClosed } + fromFork := hook.GetPullRequest().GetHead().GetRepo().GetID() != hook.GetPullRequest().GetBase().GetRepo().GetID() + pipeline := &model.Pipeline{ Event: event, Commit: hook.GetPullRequest().GetHead().GetSHA(), @@ -173,6 +175,7 @@ func parsePullHook(hook *github.PullRequestEvent, merge bool) (*github.PullReque hook.GetPullRequest().GetBase().GetRef(), ), PullRequestLabels: convertLabels(hook.GetPullRequest().Labels), + FromFork: fromFork, } if merge { pipeline.Ref = fmt.Sprintf(mergeRefs, hook.GetPullRequest().GetNumber()) diff --git a/server/forge/gitlab/convert.go b/server/forge/gitlab/convert.go index 51d16bac3..dab74fbc8 100644 --- a/server/forge/gitlab/convert.go +++ b/server/forge/gitlab/convert.go @@ -138,6 +138,7 @@ func convertMergeRequestHook(hook *gitlab.MergeEvent, req *http.Request) (int, * pipeline.Title = obj.Title pipeline.ForgeURL = obj.URL pipeline.PullRequestLabels = convertLabels(hook.Labels) + pipeline.FromFork = target.PathWithNamespace != source.PathWithNamespace return obj.IID, repo, pipeline, nil } diff --git a/server/model/pipeline.go b/server/model/pipeline.go index 5106308de..69abe5a94 100644 --- a/server/model/pipeline.go +++ b/server/model/pipeline.go @@ -52,6 +52,7 @@ type Pipeline struct { AdditionalVariables map[string]string `json:"variables,omitempty" xorm:"json 'additional_variables'"` PullRequestLabels []string `json:"pr_labels,omitempty" xorm:"json 'pr_labels'"` IsPrerelease bool `json:"is_prerelease,omitempty" xorm:"is_prerelease"` + FromFork bool `json:"from_fork,omitempty" xorm:"from_fork"` } // @name Pipeline // TableName return database table name for xorm. diff --git a/server/model/repo.go b/server/model/repo.go index 8cbdb86db..0732c1258 100644 --- a/server/model/repo.go +++ b/server/model/repo.go @@ -20,6 +20,27 @@ import ( "strings" ) +type ApprovalMode string + +const ( + RequireApprovalNone ApprovalMode = "none" // require approval for no events + RequireApprovalForks ApprovalMode = "forks" // require approval for PRs from forks (default) + RequireApprovalPullRequests ApprovalMode = "pull_requests" // require approval for all PRs + RequireApprovalAllEvents ApprovalMode = "all_events" // require approval for all external events +) + +func (mode ApprovalMode) Valid() bool { + switch mode { + case RequireApprovalNone, + RequireApprovalForks, + RequireApprovalPullRequests, + RequireApprovalAllEvents: + return true + default: + return false + } +} + // Repo represents a repository. type Repo struct { ID int64 `json:"id,omitempty" xorm:"pk autoincr 'id'"` @@ -41,7 +62,7 @@ type Repo struct { Timeout int64 `json:"timeout,omitempty" xorm:"timeout"` Visibility RepoVisibility `json:"visibility" xorm:"varchar(10) 'visibility'"` IsSCMPrivate bool `json:"private" xorm:"private"` - IsTrusted bool `json:"trusted" xorm:"trusted"` + RequireApproval ApprovalMode `json:"require_approval" xorm:"varchar(50) require_approval"` IsGated bool `json:"gated" xorm:"gated"` IsActive bool `json:"active" xorm:"active"` AllowPull bool `json:"allow_pr" xorm:"allow_pr"` @@ -109,7 +130,7 @@ func (r *Repo) Update(from *Repo) { // RepoPatch represents a repository patch object. type RepoPatch struct { Config *string `json:"config_file,omitempty"` - IsTrusted *bool `json:"trusted,omitempty"` + RequireApproval *string `json:"require_approval,omitempty"` IsGated *bool `json:"gated,omitempty"` Timeout *int64 `json:"timeout,omitempty"` Visibility *string `json:"visibility,omitempty"` diff --git a/server/pipeline/approve.go b/server/pipeline/approve.go index d91840c7d..ff4336c4f 100644 --- a/server/pipeline/approve.go +++ b/server/pipeline/approve.go @@ -27,8 +27,7 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/server/store" ) -// Approve update the status to pending for a blocked pipeline because of a gated repo -// and start them afterward. +// Approve update the status to pending for a blocked pipeline so it can be executed. func Approve(ctx context.Context, store store.Store, currentPipeline *model.Pipeline, user *model.User, repo *model.Repo) (*model.Pipeline, error) { if currentPipeline.Status != model.StatusBlocked { return nil, ErrBadRequest{Msg: fmt.Sprintf("cannot approve a pipeline with status %s", currentPipeline.Status)} diff --git a/server/pipeline/create.go b/server/pipeline/create.go index 0d75eb727..9a60299ff 100644 --- a/server/pipeline/create.go +++ b/server/pipeline/create.go @@ -68,7 +68,7 @@ func Create(ctx context.Context, _store store.Store, repo *model.Repo, pipeline // update some pipeline fields pipeline.RepoID = repo.ID pipeline.Status = model.StatusCreated - setGatedState(repo, pipeline) + setApprovalState(repo, pipeline) err = _store.CreatePipeline(pipeline) if err != nil { msg := fmt.Errorf("failed to save pipeline for %s", repo.FullName) diff --git a/server/pipeline/decline.go b/server/pipeline/decline.go index a4f7e48a3..2d098fdc5 100644 --- a/server/pipeline/decline.go +++ b/server/pipeline/decline.go @@ -26,7 +26,7 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/server/store" ) -// Decline updates the status to declined for blocked pipelines because of a gated repo. +// Decline updates the status to declined for blocked pipelines. func Decline(ctx context.Context, store store.Store, pipeline *model.Pipeline, user *model.User, repo *model.Repo) (*model.Pipeline, error) { forge, err := server.Config.Services.Manager.ForgeFromRepo(repo) if err != nil { diff --git a/server/pipeline/gated.go b/server/pipeline/gated.go index ef424e720..38fe2a1e2 100644 --- a/server/pipeline/gated.go +++ b/server/pipeline/gated.go @@ -16,11 +16,40 @@ package pipeline import "go.woodpecker-ci.org/woodpecker/v2/server/model" -func setGatedState(repo *model.Repo, pipeline *model.Pipeline) { - // TODO(336): extend gated feature with an allow/block List - if repo.IsGated && - // events created by woodpecker itself should run right away - pipeline.Event != model.EventCron && pipeline.Event != model.EventManual { - pipeline.Status = model.StatusBlocked +func setApprovalState(repo *model.Repo, pipeline *model.Pipeline) { + if !needsApproval(repo, pipeline) { + return } + + // set pipeline status to blocked and require approval + pipeline.Status = model.StatusBlocked +} + +func needsApproval(repo *model.Repo, pipeline *model.Pipeline) bool { + // skip events created by woodpecker itself + if pipeline.Event == model.EventCron || pipeline.Event == model.EventManual { + return false + } + + // repository allows all events without approval + if repo.RequireApproval == model.RequireApprovalNone { + return false + } + + // repository requires approval for pull requests from forks + if pipeline.Event == model.EventPull && pipeline.FromFork { + return true + } + + // repository requires approval for pull requests + if pipeline.Event == model.EventPull && repo.RequireApproval == model.RequireApprovalPullRequests { + return true + } + + // repository requires approval for all events + if repo.RequireApproval == model.RequireApprovalAllEvents { + return true + } + + return false } diff --git a/server/pipeline/gated_test.go b/server/pipeline/gated_test.go new file mode 100644 index 000000000..2b95313ec --- /dev/null +++ b/server/pipeline/gated_test.go @@ -0,0 +1,78 @@ +package pipeline + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "go.woodpecker-ci.org/woodpecker/v2/server/model" +) + +func TestSetGatedState(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + repo *model.Repo + pipeline *model.Pipeline + expectBlocked bool + }{ + { + name: "by-pass for cron", + repo: &model.Repo{ + RequireApproval: model.RequireApprovalAllEvents, + }, + pipeline: &model.Pipeline{ + Event: model.EventCron, + }, + expectBlocked: false, + }, + { + name: "by-pass for manual pipeline", + repo: &model.Repo{ + RequireApproval: model.RequireApprovalAllEvents, + }, + pipeline: &model.Pipeline{ + Event: model.EventManual, + }, + expectBlocked: false, + }, + { + name: "require approval for fork PRs", + repo: &model.Repo{ + RequireApproval: model.RequireApprovalForks, + }, + pipeline: &model.Pipeline{ + Event: model.EventPull, + FromFork: true, + }, + expectBlocked: true, + }, + { + name: "require approval for PRs", + repo: &model.Repo{ + RequireApproval: model.RequireApprovalPullRequests, + }, + pipeline: &model.Pipeline{ + Event: model.EventPull, + FromFork: false, + }, + expectBlocked: true, + }, + { + name: "require approval for everything", + repo: &model.Repo{ + RequireApproval: model.RequireApprovalAllEvents, + }, + pipeline: &model.Pipeline{ + Event: model.EventPush, + }, + expectBlocked: true, + }, + } + + for _, tc := range testCases { + setApprovalState(tc.repo, tc.pipeline) + assert.Equal(t, tc.expectBlocked, tc.pipeline.Status == model.StatusBlocked) + } +} diff --git a/server/store/datastore/migration/017_gated_to_require_approval.go b/server/store/datastore/migration/017_gated_to_require_approval.go new file mode 100644 index 000000000..2b669d6c6 --- /dev/null +++ b/server/store/datastore/migration/017_gated_to_require_approval.go @@ -0,0 +1,72 @@ +// Copyright 2024 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package migration + +import ( + "fmt" + + "src.techknowlogick.com/xormigrate" + "xorm.io/builder" + "xorm.io/xorm" +) + +var gatedToRequireApproval = xormigrate.Migration{ + ID: "gated-to-require-approval", + MigrateSession: func(sess *xorm.Session) (err error) { + const ( + RequireApprovalNone string = "none" + RequireApprovalForks string = "forks" + RequireApprovalPullRequests string = "pull_requests" + RequireApprovalAllEvents string = "all_events" + ) + + type repos struct { + ID int64 `xorm:"pk autoincr 'id'"` + IsGated bool `xorm:"gated"` + RequireApproval string `xorm:"require_approval"` + Visibility string `xorm:"varchar(10) 'visibility'"` + } + + if err := sess.Sync(new(repos)); err != nil { + return fmt.Errorf("sync new models failed: %w", err) + } + + // migrate gated repos + if _, err := sess.Exec( + builder.Update(builder.Eq{"require_approval": RequireApprovalAllEvents}). + From("repos"). + Where(builder.Eq{"gated": true})); err != nil { + return err + } + + // migrate public repos to new default require approval + if _, err := sess.Exec( + builder.Update(builder.Eq{"require_approval": RequireApprovalForks}). + From("repos"). + Where(builder.Eq{"gated": false, "visibility": "public"})); err != nil { + return err + } + + // migrate private repos to new default require approval + if _, err := sess.Exec( + builder.Update(builder.Eq{"require_approval": RequireApprovalNone}). + From("repos"). + Where(builder.Eq{"gated": false}.And(builder.Neq{"visibility": "public"}))); err != nil { + return err + } + + return dropTableColumns(sess, "repos", "gated") + }, +} diff --git a/server/store/datastore/migration/migration.go b/server/store/datastore/migration/migration.go index 6996482be..e6aa6e36a 100644 --- a/server/store/datastore/migration/migration.go +++ b/server/store/datastore/migration/migration.go @@ -64,6 +64,7 @@ var migrationTasks = []*xormigrate.Migration{ &unifyColumnsTables, &alterTableRegistriesFixRequiredFields, &correctPotentialCorruptOrgsUsersRelation, + &gatedToRequireApproval, } var allBeans = []any{ diff --git a/web/src/assets/locales/en.json b/web/src/assets/locales/en.json index 0b3cb8b27..27eb45fe2 100644 --- a/web/src/assets/locales/en.json +++ b/web/src/assets/locales/en.json @@ -87,10 +87,6 @@ "allow": "Allow deployments", "desc": "Allow deployments from successful pipelines. Only use if you trust all users with push access." }, - "protected": { - "protected": "Protected", - "desc": "Every pipeline needs to be approved before being executed." - }, "netrc_only_trusted": { "netrc_only_trusted": "Only inject netrc credentials into trusted containers", "desc": "Only inject netrc credentials into trusted containers (recommended)." @@ -469,5 +465,14 @@ "internal_error": "Some internal error occurred", "registration_closed": "The registration is closed", "access_denied": "You are not allowed to access this instance", - "invalid_state": "The OAuth state is invalid" + "invalid_state": "The OAuth state is invalid", + "require_approval": { + "require_approval_for": "Require approval for", + "none": "No approval required", + "none_desc": "This setting can be dangerous and should only be used on private forges where all users are trusted.", + "forks": "Pull request from forked repositories", + "pull_requests": "All pull requests", + "all_events": "All events from forge", + "desc": "Prevent malicious pipelines from exposing secrets or running harmful tasks by approving them before execution." + } } diff --git a/web/src/components/form/RadioField.vue b/web/src/components/form/RadioField.vue index 0662af0de..689765246 100644 --- a/web/src/components/form/RadioField.vue +++ b/web/src/components/form/RadioField.vue @@ -5,7 +5,7 @@ type="radio" class="radio relative flex-shrink-0 border bg-wp-control-neutral-100 border-wp-control-neutral-200 cursor-pointer rounded-full w-5 h-5 checked:bg-wp-control-ok-200 checked:border-wp-control-ok-200 focus-visible:border-wp-control-neutral-300 checked:focus-visible:border-wp-control-ok-300" :value="option.value" - :checked="innerValue.includes(option.value)" + :checked="innerValue?.includes(option.value)" @click="innerValue = option.value" />
diff --git a/web/src/components/repo/settings/GeneralTab.vue b/web/src/components/repo/settings/GeneralTab.vue index 83f484f3f..6684f5925 100644 --- a/web/src/components/repo/settings/GeneralTab.vue +++ b/web/src/components/repo/settings/GeneralTab.vue @@ -1,26 +1,6 @@