diff --git a/.semgrep/config/logic.yaml b/.semgrep/config/logic.yaml new file mode 100644 index 0000000000..85c43f5531 --- /dev/null +++ b/.semgrep/config/logic.yaml @@ -0,0 +1,11 @@ +rules: + - id: forgejo-logic-suspicious-OwnerID-check + pattern: |- + $X.OwnerID > 0 + languages: + - go + severity: ERROR + message: > + Many resources like comments or runners cannot only be owned by regular users, which have positive IDs, but also + by predefined system users like Ghost or Forgejo Actions that have negative IDs. In those cases, ownership checks + should only exclude 0: `OwnerID != 0`. diff --git a/.semgrep/tests/logic.go b/.semgrep/tests/logic.go new file mode 100644 index 0000000000..32a2777ff6 --- /dev/null +++ b/.semgrep/tests/logic.go @@ -0,0 +1,35 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import "xorm.io/builder" + +type FindRunJobOptions struct { + RepoID int64 + OwnerID int64 +} + +func (opts FindRunJobOptions) Bad() builder.Cond { + cond := builder.NewCond() + if opts.RepoID > 0 { + cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) + } + // ruleid:forgejo-logic-suspicious-OwnerID-check + if opts.OwnerID > 0 { + cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) + } + return cond +} + +func (opts FindRunJobOptions) Good() builder.Cond { + cond := builder.NewCond() + if opts.RepoID > 0 { + cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) + } + // ok:forgejo-logic-suspicious-OwnerID-check + if opts.OwnerID != 0 { + cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) + } + return cond +} diff --git a/models/actions/run_job_list.go b/models/actions/run_job_list.go index 207da30334..40a182082f 100644 --- a/models/actions/run_job_list.go +++ b/models/actions/run_job_list.go @@ -74,7 +74,7 @@ func (opts FindRunJobOptions) ToConds() builder.Cond { if opts.RepoID > 0 { cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) } - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } if opts.CommitSHA != "" { diff --git a/models/actions/run_list.go b/models/actions/run_list.go index b296e2f477..11fa8441a5 100644 --- a/models/actions/run_list.go +++ b/models/actions/run_list.go @@ -85,7 +85,7 @@ func (opts FindRunOptions) ToConds() builder.Cond { if opts.RepoID > 0 { cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) } - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } if opts.WorkflowID != "" { diff --git a/models/actions/runner.go b/models/actions/runner.go index 8061ccf37e..1a7fbb0b45 100644 --- a/models/actions/runner.go +++ b/models/actions/runner.go @@ -152,6 +152,7 @@ func (r *ActionRunner) Editable(ownerID, repoID int64) bool { // LoadAttributes loads the attributes of the runner func (r *ActionRunner) LoadAttributes(ctx context.Context) error { + // nosemgrep: forgejo-logic-suspicious-OwnerID-check (system users are not stored in the database) if r.OwnerID > 0 { var user user_model.User has, err := db.GetEngine(ctx).ID(r.OwnerID).Get(&user) @@ -214,7 +215,7 @@ func (opts FindRunnerOptions) ToConds() builder.Cond { c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0}) } cond = cond.And(c) - } else if opts.OwnerID > 0 { // OwnerID is ignored if RepoID is set + } else if opts.OwnerID != 0 { // OwnerID is ignored if RepoID is set c := builder.NewCond().And(builder.Eq{"owner_id": opts.OwnerID}) if opts.WithVisible { c = c.Or(builder.Eq{"repo_id": 0, "owner_id": 0}) diff --git a/models/actions/runner_list.go b/models/actions/runner_list.go index 6a64c46596..4186ee60be 100644 --- a/models/actions/runner_list.go +++ b/models/actions/runner_list.go @@ -28,6 +28,7 @@ func (runners RunnerList) LoadOwners(ctx context.Context) error { return err } for _, runner := range runners { + // nosemgrep: forgejo-logic-suspicious-OwnerID-check (system users are not stored in the database) if runner.OwnerID > 0 && runner.Owner == nil { runner.Owner = users[runner.OwnerID] } diff --git a/models/actions/schedule.go b/models/actions/schedule.go index 8c410b9d38..bd6755621b 100644 --- a/models/actions/schedule.go +++ b/models/actions/schedule.go @@ -116,7 +116,7 @@ func (opts FindScheduleOptions) ToConds() builder.Cond { if opts.RepoID > 0 { cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) } - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } diff --git a/models/actions/task_list.go b/models/actions/task_list.go index f85e17b41b..4c0e060497 100644 --- a/models/actions/task_list.go +++ b/models/actions/task_list.go @@ -65,7 +65,7 @@ func (opts FindTaskOptions) ToConds() builder.Cond { if opts.RepoID > 0 { cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) } - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } if opts.CommitSHA != "" { diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index cae717011e..620e0ef0ae 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -81,7 +81,7 @@ func (opts FindGPGKeyOptions) ToConds() builder.Cond { cond = cond.And(builder.Eq{"primary_key_id": ""}) } - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } if opts.KeyID != "" { diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index 7f76009e7f..ea05195b5d 100644 --- a/models/asymkey/ssh_key.go +++ b/models/asymkey/ssh_key.go @@ -190,7 +190,7 @@ type FindPublicKeyOptions struct { func (opts FindPublicKeyOptions) ToConds() builder.Cond { cond := builder.NewCond() - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } if opts.Fingerprint != "" { diff --git a/models/project/project.go b/models/project/project.go index 3a36613b49..8f3d09b956 100644 --- a/models/project/project.go +++ b/models/project/project.go @@ -136,6 +136,7 @@ func ProjectLinkForRepo(repo *repo_model.Repository, projectID int64) string { / // Link returns the project's relative URL. func (p *Project) Link(ctx context.Context) string { + // nosemgrep: forgejo-logic-suspicious-OwnerID-check (system users are not stored in the database) if p.OwnerID > 0 { err := p.LoadOwner(ctx) if err != nil { @@ -224,7 +225,7 @@ func (opts SearchOptions) ToConds() builder.Cond { if opts.Type > 0 { cond = cond.And(builder.Eq{"type": opts.Type}) } - if opts.OwnerID > 0 { + if opts.OwnerID != 0 { cond = cond.And(builder.Eq{"owner_id": opts.OwnerID}) } diff --git a/models/repo/repo.go b/models/repo/repo.go index 0c45f483e4..1bd779b5a0 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -893,6 +893,7 @@ type CountRepositoryOptions struct { func CountRepositories(ctx context.Context, opts CountRepositoryOptions) (int64, error) { sess := db.GetEngine(ctx).Where("id > 0") + // nosemgrep: forgejo-logic-suspicious-OwnerID-check (repositories cannot be owned by system users) if opts.OwnerID > 0 { sess.And("owner_id = ?", opts.OwnerID) } diff --git a/routers/web/repo/setting/webhook.go b/routers/web/repo/setting/webhook.go index 9bd4422556..cf28983ba4 100644 --- a/routers/web/repo/setting/webhook.go +++ b/routers/web/repo/setting/webhook.go @@ -341,7 +341,7 @@ func checkWebhook(ctx *context.Context) (*ownerRepoCtx, *webhook.Webhook) { var w *webhook.Webhook if orCtx.RepoID > 0 { w, err = webhook.GetWebhookByRepoID(ctx, orCtx.RepoID, ctx.ParamsInt64(":id")) - } else if orCtx.OwnerID > 0 { + } else if orCtx.OwnerID > 0 { // nosemgrep: forgejo-logic-suspicious-OwnerID-check (system users do not own webhooks) w, err = webhook.GetWebhookByOwnerID(ctx, orCtx.OwnerID, ctx.ParamsInt64(":id")) } else if orCtx.IsAdmin { w, err = webhook.GetSystemOrDefaultWebhook(ctx, ctx.ParamsInt64(":id"))