From 5f57132a2b2d8c297f7f59fe228c2894c006d711 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Apr 2026 16:36:11 +0200 Subject: [PATCH] fix(security): enforce AuthUser at the router layer, not just middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #60. `Router.Route` previously checked the route's Auth level only when it equalled `AuthAdmin`. Routes declared as `Auth: AuthUser` (`/api/auth/logout`, `/api/auth/me`, `/api/auth/profile`, `/api/auth/change-password`, `/api/api-keys/*`, `/api/commitment-options`, `/api/federation/iac`) fell through the if-block with no router-level enforcement. The endpoints were still protected by `validateSecurity → authenticate` middleware, but a future refactor that reorders middleware or adds a path bypassing `validateSecurity` would silently expose every AuthUser endpoint without any error at the router boundary. Fix: replace the `if route.Auth == AuthAdmin` branch with a switch that dispatches AuthAdmin → `requireAdmin`, AuthUser → new `requireUser`, AuthPublic → no router-level check. New `requireUser` mirrors `requireAdmin` but does not require the admin role — admin API key OR any valid session passes; missing/invalid auth returns 401 via ClientError before the handler runs. Updated the AuthLevel doc comment so the router-layer enforcement contract is explicit. Tests: 5 new router-auth tests pin the contract: - AuthUser rejects missing Authorization header (401). - AuthUser rejects invalid Bearer token (401). - AuthUser accepts a valid session. - AuthUser accepts an admin API key (admins are users). - AuthPublic still runs with no auth (guard against the switch's default branch tightening accidentally). No behavioural change for routes that were already passing through `validateSecurity` — the router-layer check is now belt-and-braces. Routes that bypass validateSecurity for any reason (e.g., a future test runner, a misconfigured handler chain) are now protected at the router boundary. --- internal/api/middleware.go | 31 ++++++++ internal/api/router.go | 46 ++++++++--- internal/api/router_auth_test.go | 127 +++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 internal/api/router_auth_test.go diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 05fb2cd3..6df1f084 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -209,6 +209,37 @@ func logMissingCSRFToken(req *events.LambdaFunctionURLRequest, csrfToken string) req.RequestContext.HTTP.Method, req.RequestContext.HTTP.Path, req.RequestContext.HTTP.SourceIP) } +// requireUser checks that the request carries a valid authentication +// (admin API key OR any valid Bearer-token session) and returns the +// resolved Session. Returns 401 on absence/invalid credentials. Used by +// Router.Route to enforce AuthUser routes at the router layer rather +// than relying solely on validateSecurity middleware ordering — see #60. +// +// Unlike requireAdmin, this does NOT require admin role. Any +// authenticated identity passes. +func (h *Handler) requireUser(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { + // Admin API key first (stateless). + apiKey := extractAPIKey(req) + if h.checkAdminAPIKey(apiKey) { + return &Session{Role: "admin", UserID: "admin-api-key"}, nil + } + + if h.auth == nil { + return nil, fmt.Errorf("authentication service not configured") + } + + token := h.extractBearerToken(req) + if token == "" { + return nil, NewClientError(401, "no authorization token provided") + } + + session, err := h.auth.ValidateSession(ctx, token) + if err != nil || session == nil { + return nil, NewClientError(401, "invalid session") + } + return session, nil +} + // requireAdmin checks if the current user has admin role. // Accepts both admin API-key auth and Bearer token auth. func (h *Handler) requireAdmin(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { diff --git a/internal/api/router.go b/internal/api/router.go index b7aa500f..cdf2a295 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -11,20 +11,31 @@ import ( // RouteHandler is a function that handles a matched route type RouteHandler func(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) -// AuthLevel controls how Router.Route() enforces authentication. -// The zero value is AuthAdmin — secure by default. -// Any Route without an explicit Auth field requires admin access. +// AuthLevel controls how Router.Route() enforces authentication. Each +// non-AuthPublic level is enforced AT THE ROUTER LAYER (in Route() +// below) — not solely via validateSecurity / authenticate middleware, +// so a future refactor that reorders middleware or adds a path that +// bypasses validateSecurity cannot silently expose AuthUser routes +// (see #60). +// +// The zero value is AuthAdmin — secure by default. Any Route without +// an explicit Auth field requires admin access. type AuthLevel int const ( // AuthAdmin requires admin role (API key or admin bearer token). - // Zero value — any Route without an explicit Auth field gets this. + // Enforced via h.requireAdmin in Router.Route — returns 401/403 + // before the handler runs. Zero value — any Route without an + // explicit Auth field gets this. AuthAdmin AuthLevel = iota - // AuthUser requires any authenticated user. Use for self-service + // AuthUser requires any authenticated user (admin API key OR a + // valid session). Enforced via h.requireUser in Router.Route — + // returns 401 before the handler runs. Use for self-service // endpoints (logout, profile, API key management). AuthUser - // AuthPublic requires no authentication. Must also be listed in - // isPublicEndpoint() for middleware bypass. + // AuthPublic requires no authentication. The router does NOT call + // any auth helper for these routes; the handler runs directly. + // Must also be listed in isPublicEndpoint() for middleware bypass. AuthPublic ) @@ -217,14 +228,31 @@ func (r *Router) registerRoutes() { } // Route finds and executes the matching route handler. -// Routes with Auth == AuthAdmin (the default) require admin access before the handler is called. +// +// Auth enforcement happens here at the router layer so a future +// refactor that reorders or removes validateSecurity middleware cannot +// silently expose AuthUser/AuthAdmin routes (see #60): +// +// - AuthAdmin → r.h.requireAdmin (admin API key OR admin-role session) +// - AuthUser → r.h.requireUser (admin API key OR any valid session) +// - AuthPublic → no router-level auth check; the handler runs directly +// +// Both helpers return ClientError-wrapped 401/403 which the transport +// layer converts to the matching HTTP response. func (r *Router) Route(ctx context.Context, method, path string, req *events.LambdaFunctionURLRequest) (any, error) { for _, route := range r.routes { if r.matches(route, method, path) { - if route.Auth == AuthAdmin { + switch route.Auth { + case AuthAdmin: if _, err := r.h.requireAdmin(ctx, req); err != nil { return nil, err } + case AuthUser: + if _, err := r.h.requireUser(ctx, req); err != nil { + return nil, err + } + case AuthPublic: + // no router-level auth — handler runs directly. } params := r.extractParams(route, path) return route.Handler(ctx, req, params) diff --git a/internal/api/router_auth_test.go b/internal/api/router_auth_test.go new file mode 100644 index 00000000..0152804c --- /dev/null +++ b/internal/api/router_auth_test.go @@ -0,0 +1,127 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// userSession returns a session for a non-admin authenticated user. +func userSession() *Session { + return &Session{ + UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", + Email: "user@example.com", + Role: "user", + } +} + +// stubHandler is a no-op route handler — its presence proves the router +// reached the handler stage rather than short-circuiting on auth. +func stubHandler(_ context.Context, _ *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) { + return map[string]string{"status": "ok"}, nil +} + +// TestRouter_AuthUser_RejectsMissingAuth pins the issue-#60 regression: +// a route declared as Auth: AuthUser must be rejected by Router.Route at +// the router layer when no Authorization header is present, regardless +// of whether validateSecurity middleware ran beforehand. +func TestRouter_AuthUser_RejectsMissingAuth(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + r := &Router{h: h} + r.routes = []Route{ + {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, // NO Authorization header + } + _, err := r.Route(ctx, "GET", "/api/test/user", req) + require.Error(t, err, "router must reject AuthUser routes without auth at the router layer") + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T", err) + assert.Equal(t, 401, ce.code) +} + +// TestRouter_AuthUser_RejectsInvalidSession pins that an invalid Bearer +// token (session not found / expired) returns 401 from the router layer. +func TestRouter_AuthUser_RejectsInvalidSession(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "stale-token").Return(nil, errors.New("session not found")) + h := &Handler{auth: mockAuth} + r := &Router{h: h} + r.routes = []Route{ + {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer stale-token"}, + } + _, err := r.Route(ctx, "GET", "/api/test/user", req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) +} + +// TestRouter_AuthUser_AcceptsValidSession confirms the router-layer +// check passes through to the handler when a valid session is present. +// This is the positive case for the new requireUser helper. +func TestRouter_AuthUser_AcceptsValidSession(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession(), nil) + h := &Handler{auth: mockAuth} + r := &Router{h: h} + r.routes = []Route{ + {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + } + result, err := r.Route(ctx, "GET", "/api/test/user", req) + require.NoError(t, err) + assert.Equal(t, map[string]string{"status": "ok"}, result) +} + +// TestRouter_AuthUser_AcceptsAdminAPIKey confirms that an admin API key +// also satisfies the AuthUser requirement (admins are users). +func TestRouter_AuthUser_AcceptsAdminAPIKey(t *testing.T) { + ctx := context.Background() + h := &Handler{apiKey: "admin-key"} // checkAdminAPIKey compares against this + r := &Router{h: h} + r.routes = []Route{ + {ExactPath: "/api/test/user", Method: "GET", Handler: stubHandler, Auth: AuthUser}, + } + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "admin-key"}, + } + result, err := r.Route(ctx, "GET", "/api/test/user", req) + require.NoError(t, err) + assert.Equal(t, map[string]string{"status": "ok"}, result) +} + +// TestRouter_AuthPublic_NoAuthCheck confirms AuthPublic routes still +// run with no auth at all — guard against accidentally tightening to +// AuthUser/AuthAdmin via the new switch's default branch. +func TestRouter_AuthPublic_NoAuthCheck(t *testing.T) { + ctx := context.Background() + h := &Handler{} + r := &Router{h: h} + r.routes = []Route{ + {ExactPath: "/api/test/public", Method: "GET", Handler: stubHandler, Auth: AuthPublic}, + } + + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{}} + result, err := r.Route(ctx, "GET", "/api/test/public", req) + require.NoError(t, err) + assert.Equal(t, map[string]string{"status": "ok"}, result) +}