fix(security): enforce HTTP method and path in scoped API token matcher

CanDoAPIRoute's non-CRUD fallback branch compared a path-derived
permission name to the token's permission strings without checking
the request method. A token with projects.background (registered for
GET /projects/:project/background) could therefore invoke DELETE on
the same path. The same method-confusion affected the whole
/projects/:project/views/:view/buckets[/:bucket] cluster, where a
token with projects.views_buckets (registered for GET) authorized
PUT, POST, and DELETE on any accessible view's buckets.

The matcher also leaked CRUD permissions between parent and nested
sub-resource groups. When the request targeted a nested CRUD resource
(e.g. projects_teams, projects_shares, projects_users, projects_views,
projects_webhooks, projects_views_tasks, tasks_assignees, tasks_labels,
tasks_comments, tasks_relations, tasks_attachments, teams_members),
the matcher fell back from the specific group to the parent's permission
list but then looked the permission name up inside the sub-resource's
RouteDetail map. The effect was that a token holding only projects.read_all
also authorized GET on every nested projects_* list endpoint, and the
same held for create/update/delete and for the tasks.* family.

Rewrite the matcher to iterate the token's own permissions and accept
only when the stored RouteDetail's (Path, Method) matches the request.
This removes all the path-derived group guessing and makes the stored
detail the single source of truth. Preserve the tasks.read_all quirk
(one permission, two list endpoints) as an explicit two-path allowlist
inside the loop.

Extract a GetAPITokenRoutes accessor so the new property-based webtest
can consume the same snapshot served by GET /api/v1/routes.

Add TestAPITokenMethodMatching in pkg/webtests: using the live echo
router and the live apiTokenRoutes map, it iterates every advertised
permission against every registered route and asserts the matcher
accepts iff the stored (Path, Method) matches. Any future collision
introduced by a new non-CRUD route on a shared path will be caught.

After this change, previously-dead permissions like
projects.background_delete, projects.views_buckets_{put,post,delete},
other.avatar, other.ws and caldav.access start working as their UI
labels imply. Tokens that relied on the over-broad background /
views_buckets grants, or on cross-cluster CRUD bleed-through, will
lose the extra access - that is the fix.

Refs: GHSA-v479-vf79-mg83
This commit is contained in:
kolaente 2026-04-09 13:52:00 +02:00 committed by kolaente
parent 27a88dd17a
commit 6a0f39b252
2 changed files with 127 additions and 40 deletions

View File

@ -287,6 +287,12 @@ func CollectRoutesForAPITokenUsage(route echo.RouteInfo, requiresJWT bool) {
}
// GetAPITokenRoutes exposes the registered scoped-token routes so tests
// and the /api/v1/routes handler share a single source of truth.
func GetAPITokenRoutes() map[string]APITokenRoute {
return apiTokenRoutes
}
// GetAvailableAPIRoutesForToken returns a list of all API routes which are available for token usage.
// @Summary Get a list of all token api routes
// @Description Returns a list of all API routes which are available to use with an api token, not a user login.
@ -296,10 +302,15 @@ func CollectRoutesForAPITokenUsage(route echo.RouteInfo, requiresJWT bool) {
// @Success 200 {array} models.APITokenRoute "The list of all routes."
// @Router /routes [get]
func GetAvailableAPIRoutesForToken(c *echo.Context) error {
return c.JSON(http.StatusOK, apiTokenRoutes)
return c.JSON(http.StatusOK, GetAPITokenRoutes())
}
// CanDoAPIRoute checks if a token is allowed to use the current api route
// CanDoAPIRoute checks if a token is allowed to use the current api route.
//
// Each permission is authoritative: the request is allowed only if the
// stored (Path, Method) for that permission matches exactly. This closes
// GHSA-v479-vf79-mg83 and the wider method/sub-resource confusion it
// enabled. The one exception is the tasks.read_all quirk handled below.
func CanDoAPIRoute(c *echo.Context, token *APIToken) (can bool) {
path := c.Path()
if path == "" {
@ -307,50 +318,32 @@ func CanDoAPIRoute(c *echo.Context, token *APIToken) (can bool) {
// the route used during registration which is what we need.
path = c.Request().URL.Path
}
method := c.Request().Method
routeGroupName, routeParts := getRouteGroupName(path)
routeGroupName = strings.TrimSuffix(routeGroupName, "_bulk")
if routeGroupName == "user" ||
routeGroupName == "users" ||
routeGroupName == "routes" {
routeGroupName = "other"
}
group, hasGroup := token.APIPermissions[routeGroupName]
if !hasGroup {
group, hasGroup = token.APIPermissions[routeParts[0]]
if !hasGroup {
return false
}
}
var route string
routes, has := apiTokenRoutes[routeGroupName]
if !has {
routes, has = apiTokenRoutes[routeParts[0]]
for group, perms := range token.APIPermissions {
routes, has := apiTokenRoutes[group]
if !has {
return false
continue
}
route = strings.Join(routeParts[1:], "_")
}
// The tasks read_all route is available as /:project/tasks and /tasks/all - therefore we need this workaround here.
if routeGroupName == "tasks" && path == "/api/v1/projects/:project/tasks" && c.Request().Method == http.MethodGet {
route = "read_all"
}
for _, p := range group {
if route == "" && routes[p] != nil && routes[p].Path == path && routes[p].Method == c.Request().Method {
return true
}
if route != "" && p == route {
return true
for _, p := range perms {
rd := routes[p]
if rd == nil {
continue
}
if rd.Method == method && rd.Path == path {
return true
}
// Two list endpoints share tasks.read_all but only one
// survives collection, so allow either explicitly.
if group == "tasks" && p == "read_all" && method == http.MethodGet &&
(path == "/api/v1/tasks" || path == "/api/v1/projects/:project/tasks") {
return true
}
}
}
log.Debugf("[auth] Token %d tried to use route %s which requires permission %s but has only %v", token.ID, path, route, token.APIPermissions)
log.Debugf("[auth] Token %d tried to use route %s %s which is not covered by its permissions %v",
token.ID, method, path, token.APIPermissions)
return false
}

View File

@ -0,0 +1,94 @@
// Vikunja is a to-do list application to facilitate your life.
// Copyright 2018-present Vikunja and contributors. All rights reserved.
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.
package webtests
import (
"net/http/httptest"
"strings"
"testing"
"code.vikunja.io/api/pkg/models"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// TestAPITokenMethodMatching is the standing guard for GHSA-v479-vf79-mg83:
// for every advertised permission it builds a single-permission token and
// asserts CanDoAPIRoute matches exactly the permission's stored (method,
// path) across every registered route. Any future contributor who adds a
// non-CRUD route on a shared path, or otherwise reintroduces method
// confusion, fails here. The tasks.read_all quirk is the only exception.
func TestAPITokenMethodMatching(t *testing.T) {
e, err := setupTestEnv()
require.NoError(t, err)
type apiRoute struct{ Method, Path string }
var allRoutes []apiRoute
for _, r := range e.Router().Routes() {
if !strings.HasPrefix(r.Path, "/api/v1") {
continue
}
if r.Method == "echo_route_not_found" {
continue
}
allRoutes = append(allRoutes, apiRoute{Method: r.Method, Path: r.Path})
}
require.NotEmpty(t, allRoutes, "echo router should have registered routes")
advertised := models.GetAPITokenRoutes()
require.NotEmpty(t, advertised, "GetAPITokenRoutes should be populated by RegisterRoutes")
// Spec the matcher must conform to.
expectedAuthorized := func(group, perm string, rd *models.RouteDetail, method, path string) bool {
if rd.Method == method && rd.Path == path {
return true
}
if group == "tasks" && perm == "read_all" && method == "GET" &&
(path == "/api/v1/tasks" || path == "/api/v1/projects/:project/tasks") {
return true
}
return false
}
for group, perms := range advertised {
for perm, rd := range perms {
token := &models.APIToken{
APIPermissions: models.APIPermissions{group: []string{perm}},
}
req := httptest.NewRequest(rd.Method, rd.Path, nil)
c := e.NewContext(req, httptest.NewRecorder())
assert.Truef(t, models.CanDoAPIRoute(c, token),
"%s.%s must authorize its own stored route %s %s",
group, perm, rd.Method, rd.Path,
)
for _, r := range allRoutes {
want := expectedAuthorized(group, perm, rd, r.Method, r.Path)
req := httptest.NewRequest(r.Method, r.Path, nil)
c := e.NewContext(req, httptest.NewRecorder())
got := models.CanDoAPIRoute(c, token)
assert.Equalf(t, want, got,
"token %s.%s (stored for %s %s) on request %s %s: got=%v want=%v",
group, perm, rd.Method, rd.Path,
r.Method, r.Path, got, want,
)
}
}
}
}