kyush/forgejo
1
0
Fork 0
forked from mirrors/forgejo
Code Pull requests Activity

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

Browse source 
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>
This commit is contained in:
Mathieu Fenniak 2026-03-03 17:55:35 +01:00 • committed by Mathieu Fenniak
commit 3d6acf5e8c
6 changed files with 218 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

78
.semgrep/config/auth.yaml Normal file
View file

@ -0,0 +1,78 @@
rules:
- id: forgejo-api-use-resource-SearchRepoOptions
patterns:
- pattern: |
repo_model.SearchRepoOptions{...}
- pattern-not: |
repo_model.SearchRepoOptions{
...,
AuthorizationReducer: ctx.Reducer,
...
}
languages:
- go
message: >
SearchRepoOptions does not take into account fine-grained access token limitations. Include the
AuthorizationReducer field.
severity: ERROR
paths:
include:
- "/routers/api/**/*.go"
- id: forgejo-api-use-resource-SearchRepoOptions
patterns:
- pattern: |
organization.SearchTeamRepoOptions{...}
- pattern-not: |
organization.SearchTeamRepoOptions{
...,
AuthorizationReducer: ctx.Reducer,
...
}
languages:
- go
message: >
SearchTeamRepoOptions does not take into account fine-grained access token limitations. Include the
AuthorizationReducer field.
severity: ERROR
paths:
include:
- "/routers/api/**/*.go"
- id: forgejo-api-use-resource-GetUserRepoPermission
patterns:
- pattern: |
$X.GetUserRepoPermission($CTX, $REPO, $DOER)
- metavariable-type:
metavariable: $CTX
types:
- "*context.APIContext"
languages:
- go
message: >
GetUserRepoPermission does not take into account fine-grained access token limitations. Use
GetUserRepoPermissionWithReducer.
fix: |
$X.GetUserRepoPermissionWithReducer($CTX, $REPO, $DOER, $CTX.Reducer)
severity: ERROR
- id: forgejo-api-suspicious-GetUserRepoPermission
patterns:
- pattern: $X.GetUserRepoPermission($CTX, $REPO, $DOER)
- pattern-not: # don't match if identical to forgejo-api-use-resource-GetUserRepoPermission
patterns:
- pattern: |
$X.GetUserRepoPermission($CTX, $REPO, $DOER)
- metavariable-type:
metavariable: $CTX
types:
- "*context.APIContext"
languages:
- go
message: >
API code is accessing GetUserRepoPermission which does not take into account fine-grained access token
limitations. Should this use GetUserRepoPermissionWithReducer?
severity: ERROR
paths:
include:
- "/routers/api/**/*.go"

58
.semgrep/tests/auth.fixed.go Normal file
View file

@ -0,0 +1,58 @@
// Copyright 2016 The Gogs Authors. All rights reserved.
// Copyright 2020 The Gitea Authors.
// SPDX-License-Identifier: MIT
package repo
import (
"net/http"
"forgejo.org/models/db"
access_model "forgejo.org/models/perm/access"
repo_model "forgejo.org/models/repo"
api "forgejo.org/modules/structs"
"forgejo.org/routers/api/v1/utils"
"forgejo.org/services/context"
"forgejo.org/services/convert"
)
// ListForks list a repository's forks
func ListForks(ctx *context.APIContext) {
forks, total, err := repo_model.GetForks(ctx, ctx.Repo.Repository, ctx.Doer, utils.GetListOptions(ctx))
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetForks", err)
return
}
apiForks := make([]*api.Repository, len(forks))
for i, fork := range forks {
// ruleid:forgejo-api-use-resource-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermissionWithReducer(ctx, fork, ctx.Doer, ctx.Reducer)
// ok:forgejo-api-use-resource-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermissionWithReducer(ctx, fork, ctx.Doer, ctx.Reducer)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
return
}
apiForks[i] = convert.ToRepo(ctx, fork, permission)
}
}
// getStarredRepos returns the repos that the user with the specified userID has
// starred
func getStarredRepos(ctx std_context.Context, user *user_model.User, private bool, listOptions db.ListOptions) ([]*api.Repository, error) {
starredRepos, err := repo_model.GetStarredRepos(ctx, user.ID, private, listOptions)
if err != nil {
return nil, err
}
repos := make([]*api.Repository, len(starredRepos))
for i, starred := range starredRepos {
// ruleid:forgejo-api-suspicious-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermission(ctx, starred, user)
if err != nil {
return nil, err
}
repos[i] = convert.ToRepo(ctx, starred, permission)
}
return repos, nil
}

58
.semgrep/tests/auth.go Normal file
View file

@ -0,0 +1,58 @@
// Copyright 2016 The Gogs Authors. All rights reserved.
// Copyright 2020 The Gitea Authors.
// SPDX-License-Identifier: MIT
package repo
import (
"net/http"
"forgejo.org/models/db"
access_model "forgejo.org/models/perm/access"
repo_model "forgejo.org/models/repo"
api "forgejo.org/modules/structs"
"forgejo.org/routers/api/v1/utils"
"forgejo.org/services/context"
"forgejo.org/services/convert"
)
// ListForks list a repository's forks
func ListForks(ctx *context.APIContext) {
forks, total, err := repo_model.GetForks(ctx, ctx.Repo.Repository, ctx.Doer, utils.GetListOptions(ctx))
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetForks", err)
return
}
apiForks := make([]*api.Repository, len(forks))
for i, fork := range forks {
// ruleid:forgejo-api-use-resource-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermission(ctx, fork, ctx.Doer)
// ok:forgejo-api-use-resource-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermissionWithReducer(ctx, fork, ctx.Doer, ctx.Reducer)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)
return
}
apiForks[i] = convert.ToRepo(ctx, fork, permission)
}
}
// getStarredRepos returns the repos that the user with the specified userID has
// starred
func getStarredRepos(ctx std_context.Context, user *user_model.User, private bool, listOptions db.ListOptions) ([]*api.Repository, error) {
starredRepos, err := repo_model.GetStarredRepos(ctx, user.ID, private, listOptions)
if err != nil {
return nil, err
}
repos := make([]*api.Repository, len(starredRepos))
for i, starred := range starredRepos {
// ruleid:forgejo-api-suspicious-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermission(ctx, starred, user)
if err != nil {
return nil, err
}
repos[i] = convert.ToRepo(ctx, starred, permission)
}
return repos, nil
}

13
routers/api/packages/npm/npm.go
View file

@ -168,6 +168,19 @@ func UploadPackage(ctx *context.Context) {
canWrite := repo.OwnerID == ctx.Doer.ID
if !canWrite {
// 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
perms, err := access_model.GetUserRepoPermission(ctx, repo, ctx.Doer)
if err != nil {
apiError(ctx, http.StatusInternalServerError, err)

5
routers/api/v1/repo/collaborators.go
View file

@ -301,6 +301,11 @@ func GetRepoPermissions(ctx *context.APIContext) {
return
}
// GetUserRepoPermission is used which doesn't take into account fine-grained access token's permissions, but that's
// appropriate in this rare case because we're trying to load the collaborator's permissions, not the access token's
// permissions.
//
// nosemgrep: forgejo-api-use-resource-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermission(ctx, ctx.Repo.Repository, collaborator)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)

6
routers/api/v1/repo/fork.go
View file

@ -63,6 +63,12 @@ func ListForks(ctx *context.APIContext) {
}
apiForks := make([]*api.Repository, len(forks))
for i, fork := range forks {
// GetUserRepoPermission is used which doesn't take into account fine-grained access token's permissions. A fork
// cannot have a different visibility than the original repo. It makes sense that if you can view a `ctx.Repo`
// (guaranteed by the `repoAssignment` middleware used on the API endpoint), you can view the forks of that
// repo.
//
// nosemgrep: forgejo-api-use-resource-GetUserRepoPermission
permission, err := access_model.GetUserRepoPermission(ctx, fork, ctx.Doer)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err)