mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-02-18 21:27:49 -05:00
ci: introduce semgrep to prevent using xorm.Sync() incorrectly in new migrations (#11142)
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/<pull request number>.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 <gusted@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
parent
64ff61c046
commit
cc47a4057f
7 changed files with 86 additions and 4 deletions
|
|
@ -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
|
||||
|
|
|
|||
24
.semgrep/config/xorm.yaml
Normal file
24
.semgrep/config/xorm.yaml
Normal file
|
|
@ -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
|
||||
45
.semgrep/tests/xorm.go
Normal file
45
.semgrep/tests/xorm.go
Normal file
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue