From 6a0f39b252a81fa4b19dc56dc889183acc9225ae Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Apr 2026 13:52:00 +0200 Subject: [PATCH] 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 --- pkg/models/api_routes.go | 73 +++++++------- .../api_token_method_matching_test.go | 94 +++++++++++++++++++ 2 files changed, 127 insertions(+), 40 deletions(-) create mode 100644 pkg/webtests/api_token_method_matching_test.go diff --git a/pkg/models/api_routes.go b/pkg/models/api_routes.go index 2f52237a7..0f8fbcbeb 100644 --- a/pkg/models/api_routes.go +++ b/pkg/models/api_routes.go @@ -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 } diff --git a/pkg/webtests/api_token_method_matching_test.go b/pkg/webtests/api_token_method_matching_test.go new file mode 100644 index 000000000..43759cccb --- /dev/null +++ b/pkg/webtests/api_token_method_matching_test.go @@ -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 . + +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, + ) + } + } + } +}