From 069e7e7c6117dff3ad6aba89720a1e4ae2e93a37 Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 16 Sep 2026 14:40:29 +0000 Subject: [PATCH] fix(api): declare MFA routes session-only so the server boots The MFA routes were missing from routeScopes and serverScopedRoutes, and both boot assertions refused to start the server. They are deliberately not reachable by API tokens, so they go in a new sessionOnlyRoutes set rather than receiving a scope, and are declared exempt from server scoping. Adds a test that registers the real routes and runs both boot assertions, so an undeclared route fails CI instead of production startup. --- CLAUDE.md | 9 ++++++ server/internal/api/bootassert_test.go | 42 +++++++++++++++++++++++++ server/internal/api/scopes.go | 28 +++++++++++++++++ server/internal/api/serverscope.go | 17 ++++++++++ server/internal/api/serverscope_test.go | 2 +- 5 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 server/internal/api/bootassert_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 2732f12..8d8d6d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -746,6 +746,15 @@ purpose - the collection is `api_tokens`, the prefix is `vt_`, the routes are `/api/tokens`, and renaming a published endpoint to match a nav label would break every script already written against it. +**Every `/api` route must be declared twice or the server refuses to boot**: +once in `routeScopes` (or `sessionOnlyRoutes`) and once in `serverScopedRoutes`. +`sessionOnlyRoutes` holds the MFA routes, which no API token may reach - a +token that could enrol a passkey or satisfy step-up would be a way around MFA. +They have no scope on purpose, and `RequireScopes` refuses any unmapped route +to a token. `TestRegisteredRoutesPassBootAssertions` registers the real routes +and runs both boot assertions, so a missing declaration fails CI rather than +production startup. + `server/internal/api/docs/openapi.json` is a **generated, committed** OpenAPI 3.1 document - `swag v2` reading `@…` annotations off the handlers - served at `GET /api/openapi.json` and rendered as a reference page by a vendored Scalar diff --git a/server/internal/api/bootassert_test.go b/server/internal/api/bootassert_test.go new file mode 100644 index 0000000..fd5d222 --- /dev/null +++ b/server/internal/api/bootassert_test.go @@ -0,0 +1,42 @@ +package api + +import ( + "strings" + "testing" + + "github.com/gin-gonic/gin" +) + +// The two boot assertions only run against a real engine in main.go, so a +// route added without its declarations compiled, passed every unit test and +// then refused to start in production. This registers the real routes and +// runs both assertions, so that failure lands in CI instead. +func TestRegisteredRoutesPassBootAssertions(t *testing.T) { + gin.SetMode(gin.TestMode) + r := gin.New() + RegisterRoutes(r) + + if err := AssertScopeMapComplete(r); err != nil { + t.Fatalf("scope map: %v", err) + } + + var routes []string + for _, route := range r.Routes() { + if strings.HasPrefix(route.Path, "/api/") { + routes = append(routes, route.Method+" "+route.Path) + } + } + if err := AssertServerScopeMapComplete(routes); err != nil { + t.Fatalf("server scope map: %v", err) + } +} + +// A route cannot be both session-only and token-reachable: an entry in both +// maps would silently hand the MFA routes to any token holding that scope. +func TestSessionOnlyRoutesHaveNoTokenScope(t *testing.T) { + for route := range sessionOnlyRoutes { + if scope, ok := routeScopes[route]; ok { + t.Errorf("%s is session-only but routeScopes grants it to tokens with %q", route, scope) + } + } +} diff --git a/server/internal/api/scopes.go b/server/internal/api/scopes.go index d0a939d..e7ad2e3 100644 --- a/server/internal/api/scopes.go +++ b/server/internal/api/scopes.go @@ -238,6 +238,30 @@ var routesOutsideAPIGroup = map[string]bool{ "GET /api/secrets/:group/values": true, } +// sessionOnlyRoutes are /api routes that an API token must never reach, so they +// deliberately have no entry in routeScopes. RequireScopes already refuses any +// unmapped route to a token; this set is what tells AssertScopeMapComplete the +// absence is a decision rather than an omission. +// +// Every entry manages the caller's own second factors or proves the caller is +// present. A token that could enrol a passkey, regenerate recovery codes or +// satisfy step-up would turn one leaked token into a way around MFA itself. +var sessionOnlyRoutes = map[string]bool{ + "GET /api/me/mfa": true, + "POST /api/me/mfa/totp/setup": true, + "POST /api/me/mfa/totp/confirm": true, + "DELETE /api/me/mfa/totp": true, + "POST /api/me/mfa/recovery/regenerate": true, + "POST /api/me/passkeys/begin": true, + "POST /api/me/passkeys/finish": true, + "PATCH /api/me/passkeys/:id": true, + "DELETE /api/me/passkeys/:id": true, + "POST /api/me/step-up": true, + "POST /api/me/step-up/webauthn/begin": true, + "POST /api/me/step-up/webauthn/finish": true, + "DELETE /api/org/users/:id/mfa": true, +} + // AssertScopeMapComplete fails boot when a registered /api route has no scope. // // Without it, adding a route silently makes it unreachable by every token, and @@ -253,6 +277,10 @@ func AssertScopeMapComplete(r *gin.Engine) error { if routesOutsideAPIGroup[route.Method+" "+route.Path] { continue } + // Session-only routes are refused to every token by having no scope. + if sessionOnlyRoutes[route.Method+" "+route.Path] { + continue + } if _, ok := routeScopes[route.Method+" "+route.Path]; !ok { missing = append(missing, route.Method+" "+route.Path) } diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index 46b4785..166f4e8 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -424,6 +424,23 @@ var serverScopedRoutes = map[string]scopeDecl{ "GET /api/patch-runs": scoped, "GET /api/patch-runs/:runId": scoped, "POST /api/patch-runs/:runId/cancel": scoped, + + // Multi-factor authentication. These act on the caller's own factors (or, + // for the reset, on another member's), never on a server, and no API token + // can reach them at all - see sessionOnlyRoutes. + "GET /api/me/mfa": exempt, + "POST /api/me/mfa/totp/setup": exempt, + "POST /api/me/mfa/totp/confirm": exempt, + "DELETE /api/me/mfa/totp": exempt, + "POST /api/me/mfa/recovery/regenerate": exempt, + "POST /api/me/passkeys/begin": exempt, + "POST /api/me/passkeys/finish": exempt, + "PATCH /api/me/passkeys/:id": exempt, + "DELETE /api/me/passkeys/:id": exempt, + "POST /api/me/step-up": exempt, + "POST /api/me/step-up/webauthn/begin": exempt, + "POST /api/me/step-up/webauthn/finish": exempt, + "DELETE /api/org/users/:id/mfa": exempt, } // AssertServerScopeMapComplete refuses to boot when any registered /api route diff --git a/server/internal/api/serverscope_test.go b/server/internal/api/serverscope_test.go index b538151..497b959 100644 --- a/server/internal/api/serverscope_test.go +++ b/server/internal/api/serverscope_test.go @@ -16,7 +16,7 @@ func TestServerScopeMapCoversEveryScopedRoute(t *testing.T) { if _, ok := routeScopes[r]; ok { continue } - if routesOutsideAPIGroup[r] { + if routesOutsideAPIGroup[r] || sessionOnlyRoutes[r] { continue } t.Errorf("route %q is declared in serverScopedRoutes but is not a registered route", r)