diff --git a/.semgrep/config/auth.yaml b/.semgrep/config/auth.yaml new file mode 100644 index 0000000000..fdd0a5cf72 --- /dev/null +++ b/.semgrep/config/auth.yaml @@ -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" diff --git a/.semgrep/tests/auth.fixed.go b/.semgrep/tests/auth.fixed.go new file mode 100644 index 0000000000..99703b2005 --- /dev/null +++ b/.semgrep/tests/auth.fixed.go @@ -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 +} diff --git a/.semgrep/tests/auth.go b/.semgrep/tests/auth.go new file mode 100644 index 0000000000..686dc5f9f7 --- /dev/null +++ b/.semgrep/tests/auth.go @@ -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 +} diff --git a/routers/api/packages/npm/npm.go b/routers/api/packages/npm/npm.go index bf9d247b30..c7313e78c0 100644 --- a/routers/api/packages/npm/npm.go +++ b/routers/api/packages/npm/npm.go @@ -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) diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 3ef7721ef5..eae25d75d7 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -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) diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index edb85cf54f..49093579b4 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -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)