fix(security): enforce AuthUser at the router layer, not just middleware (closes #60)

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) {

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)

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)
+}