From 178a0a25f82579433875e607f8633248728e7654 Mon Sep 17 00:00:00 2001 From: Andreas Ahlenstorf Date: Sun, 19 Apr 2026 04:24:09 +0200 Subject: [PATCH] chore: flag suspicious OwnerID comparisons (#12184) Resources in Forgejo can also be owned by predefined system users like Ghost or Forgejo Actions. Those have negative user IDs, for example, -2 in the case of Forgejo Actions. `OwnerID` checks oftentimes do not take these users into account, because their existence and how they work isn't well known. A [semgrep](https://semgrep.dev/) check is added that flags such suspicious `OwnerID` checks. See https://codeberg.org/forgejo/forgejo/pulls/12144 for background. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes (can be removed for JavaScript changes) - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Tests for JavaScript changes (can be removed for Go changes) - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12184 Reviewed-by: Mathieu Fenniak Co-authored-by: Andreas Ahlenstorf Co-committed-by: Andreas Ahlenstorf --- .semgrep/config/logic.yaml | 11 +++++++++ .semgrep/tests/logic.go | 35 +++++++++++++++++++++++++++++ models/actions/run_job_list.go | 2 +- models/actions/run_list.go | 2 +- models/actions/runner.go | 3 ++- models/actions/runner_list.go | 1 + models/actions/schedule.go | 2 +- models/actions/task_list.go | 2 +- models/asymkey/gpg_key.go | 2 +- models/asymkey/ssh_key.go | 2 +- models/project/project.go | 3 ++- models/repo/repo.go | 1 + routers/web/repo/setting/webhook.go | 2 +- 13 files changed, 59 insertions(+), 9 deletions(-) create mode 100644 .semgrep/config/logic.yaml create mode 100644 .semgrep/tests/logic.go 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"))