From 4adaf1c7b6a41c837e42e5f61df7277c8e25f769 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 25 Apr 2026 15:42:16 +0200 Subject: [PATCH] fix(security): defence-in-depth AuthUser check in Router.Route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Router.Route previously enforced authentication only when the matched Route had Auth == AuthAdmin. Routes declared with Auth: AuthUser (/api/auth/logout, /api/auth/me, /api/auth/profile, /api/auth/change-password, /api/api-keys, /api/federation/iac) fell through with no router-level check, relying entirely on the validateSecurity → authenticate middleware in handler.go. That worked today, but a future refactor that reordered middleware or added a route that bypassed validateSecurity would silently expose every AuthUser endpoint. Add a switch on route.Auth in Router.Route that calls a new h.requireAuth(ctx, req) helper for AuthUser routes. requireAuth reuses the existing h.authenticate path (admin API key, user API key, or session bearer token), so the accepted credentials are identical to the middleware — this is purely a belt-and-braces re-check at dispatch time. AuthPublic remains a no-op (still gated by isPublicEndpoint in the middleware). Also tighten the AuthLevel doc comments to spell out where each level is enforced (Router.Route via requireAdmin/requireAuth, isPublicEndpoint for AuthPublic), per the issue's second ask. Tests in router_authuser_test.go cover: - AuthUser route rejects when no credential is presented (401) - AuthUser route rejects when bearer token is invalid (401) - AuthUser route accepts a valid non-admin user session - AuthPublic route still dispatches with no credentials (no regression) - requireAuth unit tests for admin API key / user session / no cred Closes #60 --- internal/api/middleware.go | 15 +++ internal/api/router.go | 37 +++++-- internal/api/router_authuser_test.go | 138 +++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 7 deletions(-) create mode 100644 internal/api/router_authuser_test.go diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 05fb2cd3..21468c31 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -209,6 +209,21 @@ func logMissingCSRFToken(req *events.LambdaFunctionURLRequest, csrfToken string) req.RequestContext.HTTP.Method, req.RequestContext.HTTP.Path, req.RequestContext.HTTP.SourceIP) } +// requireAuth verifies the request carries a valid authentication credential +// of any kind (admin API key, user API key, or session bearer token). +// +// Used as a defence-in-depth check by Router.Route for AuthUser routes: +// validateSecurity → authenticate already runs before dispatch, but if a +// future refactor reorders middleware or a new route bypasses +// validateSecurity, this check still rejects unauthenticated requests at +// the router level. Returns nil on success, a 401 ClientError otherwise. +func (h *Handler) requireAuth(ctx context.Context, req *events.LambdaFunctionURLRequest) error { + if h.authenticate(ctx, req) { + return nil + } + return NewClientError(401, "authentication required") +} + // 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 6a514531..0f9176a8 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -14,17 +14,26 @@ type RouteHandler func(ctx context.Context, req *events.LambdaFunctionURLRequest // 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. +// +// Router.Route enforces these levels itself as a defence-in-depth check, +// in addition to the validateSecurity → authenticate middleware that runs +// earlier in the request pipeline. If middleware ordering ever changes or +// a new route bypasses validateSecurity, the router-level enforcement +// still rejects unauthorized requests. 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. + // AuthAdmin requires admin role (admin API key or admin bearer-token + // session). Zero value — any Route without an explicit Auth field gets + // this. Enforced by Router.Route via h.requireAdmin. AuthAdmin AuthLevel = iota - // AuthUser requires any authenticated user. Use for self-service - // endpoints (logout, profile, API key management). + // AuthUser requires any authenticated user (admin API key, user API + // key, or any valid bearer-token session). Use for self-service + // endpoints (logout, profile, API key management). Enforced by + // Router.Route via h.requireAuth. AuthUser // AuthPublic requires no authentication. Must also be listed in - // isPublicEndpoint() for middleware bypass. + // isPublicEndpoint() so the middleware skips its auth/CSRF checks. AuthPublic ) @@ -214,14 +223,28 @@ 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. +// +// Authentication enforcement is defence-in-depth: validateSecurity → +// authenticate already runs in the middleware pipeline before dispatch, +// but Router.Route also enforces the per-route Auth level so routes stay +// protected even if middleware ordering changes or a new code path +// bypasses validateSecurity. Routes with Auth == AuthAdmin (the default) +// require admin access; AuthUser routes require any authenticated user; +// AuthPublic routes are unauthenticated. 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.requireAuth(ctx, req); err != nil { + return nil, err + } + case AuthPublic: + // no auth check; relied upon by middleware via isPublicEndpoint } params := r.extractParams(route, path) return route.Handler(ctx, req, params) diff --git a/internal/api/router_authuser_test.go b/internal/api/router_authuser_test.go new file mode 100644 index 00000000..eb0858fa --- /dev/null +++ b/internal/api/router_authuser_test.go @@ -0,0 +1,138 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// These tests cover the defence-in-depth AuthUser enforcement added to +// Router.Route — see issue #60. Before the fix, AuthUser routes (e.g. +// /api/auth/logout, /api/api-keys, /api/federation/iac) fell through the +// router with no auth check; only the validateSecurity middleware +// protected them. These tests pin the router-level enforcement so a +// future middleware refactor can't silently expose them. + +// TestRouterAuthUser_NoCredentials_Rejects verifies that an AuthUser +// route returns a 401 ClientError when no credential is presented. +func TestRouterAuthUser_NoCredentials_Rejects(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, + } + + _, err := r.Route(ctx, "POST", "/api/auth/logout", req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 401, ce.code) +} + +// TestRouterAuthUser_InvalidBearerToken_Rejects verifies that an AuthUser +// route returns 401 when the bearer token is not recognised by the auth +// service. +func TestRouterAuthUser_InvalidBearerToken_Rejects(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateUserAPIKeyAPI", ctx, ""). + Return(nil, nil, errors.New("empty key")) + mockAuth.On("ValidateSession", ctx, "bad-token"). + Return(nil, errors.New("expired")) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer bad-token"}, + } + + _, err := r.Route(ctx, "GET", "/api/api-keys", req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 401, ce.code) +} + +// TestRouterAuthUser_ValidUserSession_Accepts verifies that an AuthUser +// route dispatches to the handler when a valid non-admin user session is +// present. Before the fix, AuthUser dispatch was unconditional — this +// test ensures the new check doesn't accidentally reject legitimate +// users. +func TestRouterAuthUser_ValidUserSession_Accepts(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + userSession := &Session{UserID: "11111111-1111-1111-1111-111111111111", Role: "user"} + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + mockAuth.On("Logout", ctx, "user-token").Return(nil) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + } + + result, err := r.Route(ctx, "POST", "/api/auth/logout", req) + require.NoError(t, err) + assert.NotNil(t, result) +} + +// TestRouterAuthPublic_NoCredentials_Accepts verifies that AuthPublic +// routes still dispatch with no credentials — the new switch in +// Router.Route must not regress public-endpoint behaviour. +func TestRouterAuthPublic_NoCredentials_Accepts(t *testing.T) { + ctx := context.Background() + h := &Handler{} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, + } + + // /api/health is AuthPublic; healthCheckHandler doesn't depend on auth. + _, err := r.Route(ctx, "GET", "/api/health", req) + require.NoError(t, err) +} + +// TestRequireAuth_AdminAPIKey verifies the new requireAuth helper accepts +// the admin API key. +func TestRequireAuth_AdminAPIKey(t *testing.T) { + h := &Handler{apiKey: "admin-secret"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "admin-secret"}, + } + require.NoError(t, h.requireAuth(context.Background(), req)) +} + +// TestRequireAuth_UserSession verifies requireAuth accepts a valid +// non-admin user session. +func TestRequireAuth_UserSession(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + userSession := &Session{UserID: "uid", Role: "user"} + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + } + require.NoError(t, h.requireAuth(ctx, req)) +} + +// TestRequireAuth_NoCredential_Rejects verifies requireAuth returns a 401 +// ClientError when no credential is presented. +func TestRequireAuth_NoCredential_Rejects(t *testing.T) { + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{}} + err := h.requireAuth(context.Background(), req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) +}