mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
3 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Mathieu Fenniak
|
f93d2cb261 |
ci: detect and prevent empty case statements in Go code (#11593)
One of the security patches released 2026-03-09 [fixed a vulnerability](https://codeberg.org/forgejo/forgejo/pulls/11513/commits/d1c7b04d09f6a13896eaa1322ac690b2021539da) caused by a misapplication of Go `case` statements, where the implementation would have been correct if Go `case` statements automatically fall through to the next case block, but they do not. This PR adds a semgrep rule which detects any empty `case` statement and raises an error, in order to prevent this coding mistake in the future. For example, code like this will now trigger a build error: ```go switch setting.Protocol { case setting.HTTPUnix: case setting.FCGI: case setting.FCGIUnix: default: defaultLocalURL := string(setting.Protocol) + "://" } ``` Example error: ``` cmd/web.go ❯❯❱ semgrep.config.forgejo-switch-empty-case switch has a case block with no content. This is treated as "break" by Go, but developers may confuse it for "fallthrough". To fix this error, disambiguate by using "break" or "fallthrough". 279┆ switch setting.Protocol { 280┆ case setting.HTTPUnix: 281┆ case setting.FCGI: 282┆ case setting.FCGIUnix: 283┆ default: 284┆ defaultLocalURL := string(setting.Protocol) + "://" 285┆ if setting.HTTPAddr == "0.0.0.0" { 286┆ defaultLocalURL += "localhost" 287┆ } else { 288┆ defaultLocalURL += setting.HTTPAddr ``` As described in the error output, this error can be fixed by explicitly listing `break` (the real Go behaviour, to do nothing in the block), or by listing `fallthrough` (if the intent was to fall through). All existing code triggering this detection has been changed to `break` (or, rarely, irrelevant cases have been removed), which should maintain the same code functionality. While performing this fixup, a light analysis was performed on each case and they *appeared* correct, but with ~65 cases I haven't gone into extreme depth. Tests are present for the semgrep rule in `.semgrep/tests/go.go`. ## 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). ### 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. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11593 Reviewed-by: Gusted <gusted@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net> |
||
|
Mathieu Fenniak
|
3d6acf5e8c |
ci: add semgrep detection for API code ignoring repo-specific access tokens (#11476)
This PR is part of a series (#11311). Prevents the usage of three internal APIs in the web API code: - `repo_model.SearchRepoOptions{}` without an `AuthorizationReducer` - `organization.SearchTeamRepoOptions{}` without an `AuthorizationReducer` - `access_model.GetUserRepoPermission()`, which doesn't take an `AuthorizationReducer` -- use `GetUserRepoPermissionWithReducer` instead. A couple lingering usages are marked with `// nosemgrep: ...` as they have been inspected and considered correct as-is. The `GetUserRepoPermission` is tested via the `.semgrep/tests` files; the other rules have been tested manually. ## 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). ### 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. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11476 Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net> |
||
|
Mathieu Fenniak
|
cc47a4057f |
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> |