ci: add semgrep detection for API code ignoring repo-specific access tokens #11476

mfenniak · 2026-03-02T23:39:07+01:00

mfenniak commented

2026-03-02 23:39:07 +01:00

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 contains information that will be helpful to first time contributors. There also are a few conditions for merging Pull Requests in Forgejo repositories. You are also welcome to join the Forgejo development chatroom.

Documentation

I created a pull request to the documentation to explain to Forgejo users how to use this change.
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.
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.

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.

mfenniak added the

test

not-needed

label

2026-03-02 23:39:07 +01:00

mfenniak added 5 commits

2026-03-02 23:39:07 +01:00

refactor: exempt npm UploadPackage from fine-grained access token usage d7f46750b0

refactor: exempt /repos/{owner}/{repo}/collaborators/{collaborator}/permission from fine-grained access token usage 8e4365951b

refactor: exempt /repos/{owner}/{repo}/forks from fine-grained access token usage f9e3b3f60c

ci: add semgrep detection for incorrect Repo searches 0eb8e85fcf

ci: add semgrep detection for incorrect GetUserRepoPermission

issue-labels / backporting (pull_request_target) Has been skipped

Details

testing / semgrep/ci (pull_request) Successful in 18s

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

Integration tests for the release process / release-simulation (push) Successful in 5m34s

Details

issue-labels / release-notes (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 3s

Details

testing / frontend-checks (pull_request) Successful in 1m21s

Details

testing / backend-checks (pull_request) Successful in 3m37s

Details

testing / test-unit (pull_request) Successful in 7m50s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m24s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 3m12s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m25s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m28s

Details

testing / test-mysql (pull_request) Successful in 28m18s

Details

testing / test-e2e (pull_request) Successful in 28m20s

Details

testing / test-sqlite (pull_request) Successful in 34m23s

Details

testing / test-pgsql (pull_request) Successful in 40m39s

Details

testing / security-check (pull_request) Successful in 1m44s

Details

a274a3d662

Whenever GetUserRepoPermission is invoked with a `*context.APIContext`,
or whenever it is invoked from code in `routers/api`, a semgrep-powered
error will identify that fine-grained access token limitations aren't
implemented and that GetUserRepoPermissionWithReducer may be preferred.

mfenniak requested reviews from Cyborus, Gusted

2026-03-02 23:39:07 +01:00

mfenniak referenced this pull request

2026-03-02 23:40:33 +01:00

WIP: feat: backend support for fine-grained repo-level access tokens [skip ci] #11311

aahlenst reviewed

2026-03-03 15:39:07 +01:00

routers/api/packages/npm/npm.go Outdated

					
				@ -171,0 +173,4 @@

							// permission checks.  Repo-specific access tokens aren't expected to be used with `package:write` scope, so

							// there's currently no need to change this to GetUserRepoPermissionWithReducer -- and because package APIs

							// doesn't take use an `APIContext`, we don't have access to a reducer to provide to it.

							//

Repo-specific access tokens aren't expected to be used with package:write scope.

I don't follow. Packages, at least OCI images, can be attached to repositories.

In any case, I find the description a bit unclear. I tried to reword it:

AuthorizationReducer is not applicable to this usage of GetUserRepoPermission() because it is used to figure out whether the user has the permission package:write. Repository-specific access tokens are not supposed to curtail package:write.

> Repo-specific access tokens aren't expected to be used with `package:write` scope. I don't follow. Packages, at least OCI images, can be attached to repositories. In any case, I find the description a bit unclear. I tried to reword it: ``` AuthorizationReducer is not applicable to this usage of GetUserRepoPermission() because it is used to figure out whether the user has the permission package:write. Repository-specific access tokens are not supposed to curtail package:write. ```

@aahlenst wrote in #11476 (comment):

Repo-specific access tokens aren't expected to be used with package:write scope.

I don't follow. Packages, at least OCI images, can be attached to repositories.

Repo-specific access tokens will be restricted to only providing the read:repository, write:repository, read:issue, and write:issue scopes. Any other configuration will fail to create an access token (I'm still authoring the PRs to create repo-specific access tokens, so I don't have code to link here). This is the meaning of the statement "Repo-specific access tokens aren't expected to be used with package:write scope."

I'll take a pass at rewriting this comment to clarify it.

@aahlenst wrote in https://codeberg.org/forgejo/forgejo/pulls/11476#issuecomment-11163926: > > Repo-specific access tokens aren't expected to be used with `package:write` scope. > > I don't follow. Packages, at least OCI images, can be attached to repositories. Repo-specific access tokens will be restricted to only providing the `read:repository`, `write:repository`, `read:issue`, and `write:issue` scopes. Any other configuration will fail to create an access token (I'm still authoring the PRs to create repo-specific access tokens, so I don't have code to link here). This is the meaning of the statement "Repo-specific access tokens aren't expected to be used with `package:write` scope." I'll take a pass at rewriting this comment to clarify it.

Revision a274a3d662..a8fef29b10:

// GetUserRepoPermission is used, which doesn't take into account any `AuthorizationReducer` from access
// tokens.  At present, `AuthorizationReducer` only provides capabilities to implement repo-specific access
// tokens.  It is not possible to create a repo-specific access token with the `package:write` scope, and
// that scope is required to call this API, so implementing `AuthorizationReducer` is not necessary here.
//
// This access check would need revision if either: package-specific access tokens were created, or,
// `package:write` scope was permitted on a repo-specific access token.
//
// (Because package APIs doesn't take use an `APIContext`, we don't have access to a reducer to provide to
// GetUserRepoPermissionWithReducer, otherwise we'd just do it and not spend so much time describing why we
// don't have to.)
//
// nosemgrep: forgejo-api-suspicious-GetUserRepoPermission

(The fact that this is a weird access check is removed from the comment because it's irrelevant to the ignore. While packages in general can be linked to repositories, as you've noted, this appears to be the only access check in the package API that looks for permission to write to the linked repository. 🤷 It stands out as a possible mistake, but, not really relevant to why we can ignore repo-specific access tokens here.)

Revision https://codeberg.org/forgejo/forgejo/compare/a274a3d66253075ce278bc187bf14f71904609a7..a8fef29b1024d70ea5b0619b345ce9177c76057b: ```go // GetUserRepoPermission is used, which doesn't take into account any `AuthorizationReducer` from access // tokens. At present, `AuthorizationReducer` only provides capabilities to implement repo-specific access // tokens. It is not possible to create a repo-specific access token with the `package:write` scope, and // that scope is required to call this API, so implementing `AuthorizationReducer` is not necessary here. // // This access check would need revision if either: package-specific access tokens were created, or, // `package:write` scope was permitted on a repo-specific access token. // // (Because package APIs doesn't take use an `APIContext`, we don't have access to a reducer to provide to // GetUserRepoPermissionWithReducer, otherwise we'd just do it and not spend so much time describing why we // don't have to.) // // nosemgrep: forgejo-api-suspicious-GetUserRepoPermission ``` (The fact that this is a weird access check is removed from the comment because it's irrelevant to the ignore. While packages in general can be linked to repositories, as you've noted, this appears to be the only access check in the package API that looks for permission to write to the linked repository. 🤷 It stands out as a possible mistake, but, not really relevant to why we can ignore repo-specific access tokens here.)

aahlenst marked this conversation as resolved

mfenniak force-pushed finegrained-pr7-semgrep from a274a3d662

issue-labels / backporting (pull_request_target) Has been skipped

Details

testing / semgrep/ci (pull_request) Successful in 18s

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

Integration tests for the release process / release-simulation (push) Successful in 5m34s

Details

issue-labels / release-notes (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 3s

Details

testing / frontend-checks (pull_request) Successful in 1m21s

Details

testing / backend-checks (pull_request) Successful in 3m37s

Details

testing / test-unit (pull_request) Successful in 7m50s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m24s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 3m12s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m25s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m28s

Details

testing / test-mysql (pull_request) Successful in 28m18s

Details

testing / test-e2e (pull_request) Successful in 28m20s

Details

testing / test-sqlite (pull_request) Successful in 34m23s

Details

testing / test-pgsql (pull_request) Successful in 40m39s

Details

testing / security-check (pull_request) Successful in 1m44s

Details

to a8fef29b10

issue-labels / release-notes (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 2s

Details

testing / backend-checks (pull_request) Successful in 4m0s

Details

testing / frontend-checks (pull_request) Successful in 1m12s

Details

testing / semgrep/ci (pull_request) Successful in 16s

Details

testing / test-unit (pull_request) Successful in 7m12s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m6s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 2m12s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m14s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m18s

Details

testing / test-e2e (pull_request) Successful in 22m20s

Details

testing / test-mysql (pull_request) Successful in 24m3s

Details

testing / test-sqlite (pull_request) Successful in 27m10s

Details

testing / test-pgsql (pull_request) Successful in 32m15s

Details

testing / security-check (pull_request) Successful in 57s

Details

issue-labels / backporting (pull_request_target) Has been skipped

Details

milestone / set (pull_request_target) Successful in 15s

Details

2026-03-03 17:07:29 +01:00

Compare

aahlenst approved these changes

2026-03-03 17:17:19 +01:00

mfenniak merged commit 3d6acf5e8c into forgejo

2026-03-03 17:55:37 +01:00

mfenniak deleted branch finegrained-pr7-semgrep

2026-03-03 17:55:39 +01:00

mfenniak referenced this pull request from a commit

2026-03-03 17:55:39 +01:00

ci: add `semgrep` detection for API code ignoring repo-specific access tokens (#11476)

forgejo-release-notes-assistant added this to the Forgejo v15.0.0 milestone

2026-03-03 17:56:15 +01:00

Sign in to join this conversation.

No reviewers

No labels

backport/v1.21/forgejo

backport/v10.0/forgejo

backport/v11.0/forgejo

backport/v12.0/forgejo

backport/v13.0/forgejo

backport/v14.0/forgejo

backport/v7.0/forgejo

backport/v8.0/forgejo

backport/v9.0/forgejo

Rows
Columns

ci: add semgrep detection for API code ignoring repo-specific access tokens #11476

Checklist

Documentation

Release notes

ci: add `semgrep` detection for API code ignoring repo-specific access tokens #11476