From cc47a4057fb75b8f777f0f6e91e25cbe504fef29 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Sat, 7 Feb 2026 21:52:43 +0100 Subject: [PATCH] ci: introduce `semgrep` to prevent using `xorm.Sync()` incorrectly in new migrations (#11142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a CI check which detects any usage of xorm's `Sync` method that doesn't include `IgnoreDropIndices: true`, and causes an error. `semgrep` is a semantic grep tool that allows for the relatively easy authoring of linting tools that are customized to a project's specific needs, rather than generic like `golangci` and related tools. Although `semgrep` offers a suite of out-of-the-box rules (and a paid set of rules), neither of those are used here -- only one Forgejo-specific rule is added in `.semgrep/xorm.yaml`. My intent with this change is to introduce the idea and infrastructure of `semgrep` with a single minimal rule. Once in-place, this will become a tool that we can use when we recognize bad coding patterns and wish to correct them permanently, rather than relying on human code review. While generic linting tools do this well for general patterns, this will allow Forgejo to apply domain-specific checks. For example, in #11112, an error indicates that it might be appropriate for us to always use `.StorageEngine("InnoDB")` when using an xorm engine -- if we made that determination, it could be cemented in-place with a `semgrep` rule relatively easily. This specific rule looks for any access for xorm's `Sync` or `SyncWithOptions` methods on the `*xorm.Engine` or `*xorm.Session`. They are then considered errors if they don't include `IgnoreDropIndices: true`. This is *typically* correct and safe, but can also be ignored when specifically needed. In the `.semgrep/tests` folder, test code is added which validates that the `semgrep` rule matches the expected patterns; this self-test is run before `semgrep` runs on the PR in CI. As a demonstration, when `IgnoreDropIndices` is removed from a migration, here's an error: https://codeberg.org/forgejo/forgejo/actions/runs/135750/jobs/12/attempt/1 ``` models/forgejo_migrations/v14b_add-action_run-preexecutionerrorcode.go ❯❯❱ semgrep.xorm-sync-missing-ignore-drop-indices xorm Sync operation may drop indices if used on an incomplete bean definition for an existing table. Use SyncWithOptions with IgnoreDropIndices: true instead. 22┆ _, err := x.SyncWithOptions(xorm.SyncOptions{}, new(ActionRun)) ``` ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. 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 - 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 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/11142 Reviewed-by: Gusted Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- .forgejo/workflows/testing.yml | 13 ++++++ .semgrep/config/xorm.yaml | 24 ++++++++++ .semgrep/tests/xorm.go | 45 +++++++++++++++++++ models/forgejo_migrations/migrate.go | 2 +- .../v14a_actions-approval-and-trust.go | 2 +- .../v14a_add-forgejo-migrations-table.go | 2 +- .../v14b_action-reindexing.go | 2 +- 7 files changed, 86 insertions(+), 4 deletions(-) create mode 100644 .semgrep/config/xorm.yaml create mode 100644 .semgrep/tests/xorm.go diff --git a/.forgejo/workflows/testing.yml b/.forgejo/workflows/testing.yml index 395e6c99e5..64c5dbc72d 100644 --- a/.forgejo/workflows/testing.yml +++ b/.forgejo/workflows/testing.yml @@ -301,3 +301,16 @@ jobs: - uses: ./.forgejo/workflows-composite/setup-env - run: su forgejo -c 'make deps-backend deps-tools' - run: su forgejo -c 'make security-check' + semgrep: + if: vars.ROLE == 'forgejo-coding' || vars.ROLE == 'forgejo-testing' + name: semgrep/ci + runs-on: docker + container: + image: 'data.forgejo.org/oci/semgrep:latest' + steps: + - run: apk add nodejs # required for actions/checkout + - uses: https://data.forgejo.org/actions/checkout@v6 + - name: self-check semgrep rules + run: semgrep --test .semgrep/tests/ --config .semgrep/config/ + - name: semgrep ci + run: semgrep ci --config .semgrep/config/ --metrics=off diff --git a/.semgrep/config/xorm.yaml b/.semgrep/config/xorm.yaml new file mode 100644 index 0000000000..057e1d3aef --- /dev/null +++ b/.semgrep/config/xorm.yaml @@ -0,0 +1,24 @@ +rules: + - id: xorm-sync-missing-ignore-drop-indices + patterns: + - pattern-either: + - pattern: | + $X.Sync(...) + - pattern: | + $X.SyncWithOptions($OPTS, ...) + - pattern-not: | + $X.SyncWithOptions(xorm.SyncOptions{..., IgnoreDropIndices: true, ...}, ...) + - metavariable-type: + metavariable: $X + types: + - "*xorm.Engine" + - "*xorm.Session" + paths: + exclude: + - /models/gitea_migrations/**/*.go + - /models/forgejo_migrations_legacy/**/*.go + languages: + - go + message: | + xorm Sync operation may drop indices if used on an incomplete bean definition for an existing table. Use SyncWithOptions with IgnoreDropIndices: true instead. + severity: ERROR diff --git a/.semgrep/tests/xorm.go b/.semgrep/tests/xorm.go new file mode 100644 index 0000000000..00b86acee1 --- /dev/null +++ b/.semgrep/tests/xorm.go @@ -0,0 +1,45 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "io/fs" + + "forgejo.org/modules/timeutil" + + "xorm.io/xorm" +) + +type ActionUser struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(user, id)"` + RepoID int64 `xorm:"INDEX UNIQUE(action_user_index) REFERENCES(repository, id)"` + + TrustedWithPullRequests bool + + LastAccess timeutil.TimeStamp `xorm:"INDEX"` +} + +func testSyncBad1(x *xorm.Engine) error { + // ruleid:xorm-sync-missing-ignore-drop-indices + return x.Sync(new(ActionUser)) +} + +func testSyncBad2(x *xorm.Engine) error { + // ruleid:xorm-sync-missing-ignore-drop-indices + _, err = x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: false}, bean) + return err +} + +func testSyncGood1(x *xorm.Engine) error { + // ok:xorm-sync-missing-ignore-drop-indices + _, err = x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, bean) + return err +} + +func testSyncGood2(x *fs.File) error { + // ok:xorm-sync-missing-ignore-drop-indices + _, err = x.Sync() + return err +} diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 53ab95d216..d64009db51 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -153,7 +153,7 @@ func Migrate(x *xorm.Engine, freshDB bool) error { // Set a new clean the default mapper to GonicMapper as that is the default for . x.SetMapper(names.GonicMapper{}) - if err := x.Sync(new(ForgejoMigration)); err != nil { + if _, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, new(ForgejoMigration)); err != nil { return fmt.Errorf("sync: %w", err) } diff --git a/models/forgejo_migrations/v14a_actions-approval-and-trust.go b/models/forgejo_migrations/v14a_actions-approval-and-trust.go index 657d512548..9f6691c4f0 100644 --- a/models/forgejo_migrations/v14a_actions-approval-and-trust.go +++ b/models/forgejo_migrations/v14a_actions-approval-and-trust.go @@ -41,7 +41,7 @@ func v14ActionsApprovalAndTrustCreateTableActionUser(x *xorm.Engine) error { LastAccess timeutil.TimeStamp `xorm:"INDEX"` } - return x.Sync(new(ActionUser)) + return x.Sync(new(ActionUser)) // nosemgrep:xorm-sync-missing-ignore-drop-indices } func v14ActionsApprovalAndTrustAddActionsRunFields(x *xorm.Engine) error { diff --git a/models/forgejo_migrations/v14a_add-forgejo-migrations-table.go b/models/forgejo_migrations/v14a_add-forgejo-migrations-table.go index f78eec8789..8f22983e80 100644 --- a/models/forgejo_migrations/v14a_add-forgejo-migrations-table.go +++ b/models/forgejo_migrations/v14a_add-forgejo-migrations-table.go @@ -21,5 +21,5 @@ func addForgejoMigration(x *xorm.Engine) error { ID string `xorm:"pk"` CreatedUnix timeutil.TimeStamp `xorm:"created"` } - return x.Sync(new(ForgejoMigration)) + return x.Sync(new(ForgejoMigration)) // nosemgrep:xorm-sync-missing-ignore-drop-indices } diff --git a/models/forgejo_migrations/v14b_action-reindexing.go b/models/forgejo_migrations/v14b_action-reindexing.go index 6b5608a5d5..83dc452191 100644 --- a/models/forgejo_migrations/v14b_action-reindexing.go +++ b/models/forgejo_migrations/v14b_action-reindexing.go @@ -69,5 +69,5 @@ func reworkActionIndexes(x *xorm.Engine) error { return err } - return x.Sync(new(v14bAction)) + return x.Sync(new(v14bAction)) // nosemgrep:xorm-sync-missing-ignore-drop-indices }