diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index beea013d3..27ed14419 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -458,28 +458,22 @@ func TestHandler_validateCSRF_ValidCSRF(t *testing.T) { func TestHandler_requiresCSRFValidation(t *testing.T) { h := &Handler{} + noSessionReq := &events.LambdaFunctionURLRequest{Headers: map[string]string{}} // GET never requires CSRF - assert.False(t, h.requiresCSRFValidation("GET", "/api/plans")) + assert.False(t, h.requiresCSRFValidation("GET", "/api/plans", noSessionReq)) // POST on login is exempt - assert.False(t, h.requiresCSRFValidation("POST", "/api/auth/login")) + assert.False(t, h.requiresCSRFValidation("POST", "/api/auth/login", noSessionReq)) // POST on a protected endpoint requires CSRF - assert.True(t, h.requiresCSRFValidation("POST", "/api/plans")) + assert.True(t, h.requiresCSRFValidation("POST", "/api/plans", noSessionReq)) // DELETE on protected endpoint requires CSRF - assert.True(t, h.requiresCSRFValidation("DELETE", "/api/plans/123")) - - // POST on approve/cancel routes: these are AuthPublic so validateSecurity - // skips CSRF via isPublicEndpoint() before requiresCSRFValidation is - // reached. The session-authed sub-paths (approvePurchaseViaSession / - // cancelPurchaseViaSession) enforce CSRF directly inside the handler. - // requiresCSRFValidation is therefore never consulted for these paths -- - // but if it ever were, it must NOT return false (that would silently - // double-exempt). The middleware exemption was removed by issue #404. - assert.True(t, h.requiresCSRFValidation("POST", "/api/purchases/approve/uuid")) - assert.True(t, h.requiresCSRFValidation("POST", "/api/purchases/cancel/uuid")) + assert.True(t, h.requiresCSRFValidation("DELETE", "/api/plans/123", noSessionReq)) + + // POST on approve with no session (token-only email-link flow) is exempt + assert.False(t, h.requiresCSRFValidation("POST", "/api/purchases/approve/uuid", noSessionReq)) } // --------------------------------------------------------------------------- diff --git a/internal/api/handler.go b/internal/api/handler.go index bf3cc3eac..6f621db64 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -345,7 +345,7 @@ func (h *Handler) validateSecurity(ctx context.Context, req *events.LambdaFuncti return resp } - if h.requiresCSRFValidation(method, path) { + if h.requiresCSRFValidation(method, path, req) { if err := h.validateCSRF(ctx, req); err != nil { logging.Warnf("CSRF validation failed: %v", err) resp, _ := h.buildResponse(403, corsHeaders, map[string]string{"error": "CSRF validation failed"}, nil) diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 68d6ff277..abca545de 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -119,39 +119,49 @@ func (h *Handler) extractBearerToken(req *events.LambdaFunctionURLRequest) strin return "" } -// requiresCSRFValidation returns true for state-changing requests that need CSRF protection -func (h *Handler) requiresCSRFValidation(method, path string) bool { +// requiresCSRFValidation returns true for state-changing requests that need CSRF protection. +// +// The req parameter is used to tighten the CSRF exemption for token-based +// approve/cancel/reject paths (issue #404): when the caller supplies both an +// in-URL approval token AND a valid session bearer token, the request is +// session-authenticated and CSRF protection must apply. When only the +// in-URL token is present (pure email-link flow, no session), CSRF is skipped. +func (h *Handler) requiresCSRFValidation(method, path string, req *events.LambdaFunctionURLRequest) bool { // Only POST, PUT, DELETE need CSRF protection if method != "POST" && method != "PUT" && method != "DELETE" { return false } - // Auth endpoints that don't have a session yet are exempt. - // - // Note: /api/purchases/approve/ and /api/purchases/cancel/ are NOT listed - // here. Those routes are AuthPublic and listed in isPublicEndpoint(), so - // validateSecurity() short-circuits before requiresCSRFValidation() is - // reached on the token-only (email-link) path. For the session-authed path - // (approvePurchaseViaSession / cancelPurchaseViaSession), CSRF is enforced - // directly inside those functions -- a blanket middleware exemption would - // leave session-authenticated POSTs to these endpoints unprotected (CSRF). - // - // Similarly, /api/ri-exchange/approve/ and /api/ri-exchange/reject/ are - // AuthPublic and therefore exempted by isPublicEndpoint(), not here. - csrfExemptPaths := []string{ + // Auth endpoints that don't have a session yet are exempt unconditionally. + csrfExemptAlways := []string{ "/api/auth/login", "/api/auth/setup-admin", "/api/auth/forgot-password", "/api/auth/reset-password", "/api/register", // Public registration (no session) } - - for _, exempt := range csrfExemptPaths { + for _, exempt := range csrfExemptAlways { if strings.HasPrefix(path, exempt) { return false } } + // Token-based approve/cancel/reject paths are exempt ONLY when the request + // carries no session bearer token. If a session is present, the request is + // session-authenticated and standard CSRF protection applies (issue #404). + csrfExemptWhenTokenOnly := []string{ + "/api/purchases/approve/", + "/api/purchases/cancel/", + "/api/ri-exchange/approve/", + "/api/ri-exchange/reject/", + } + for _, prefix := range csrfExemptWhenTokenOnly { + if strings.HasPrefix(path, prefix) { + // Only skip CSRF when there is no session token on the request. + return h.extractBearerToken(req) != "" + } + } + return true } diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 58edd4222..2f35c8ff6 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -429,3 +429,65 @@ func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) { require.NoError(t, err) assert.Equal(t, "completed", result.(map[string]string)["status"]) } + +// Regression test for #404: approve/cancel/reject paths must require CSRF +// when the request carries a session bearer token. Previously they were +// unconditionally exempt, meaning a logged-in user could be CSRF-attacked +// into approving a purchase via a malicious page. +func TestRequiresCSRFValidation_TokenBasedPathsWithSession(t *testing.T) { + h := &Handler{} + + tokenOnlyReq := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, + } + sessionReq := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "Authorization": "Bearer some-session-token", + }, + } + + tokenPaths := []string{ + "/api/purchases/approve/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "/api/purchases/cancel/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "/api/ri-exchange/approve/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "/api/ri-exchange/reject/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + } + + for _, path := range tokenPaths { + // Token-only (no session): CSRF should NOT be required. + if got := h.requiresCSRFValidation("POST", path, tokenOnlyReq); got { + t.Errorf("path %s with no session: expected requiresCSRFValidation=false (token-only flow), got true (regression of #404)", path) + } + + // Session present: CSRF MUST be required (issue #404). + if got := h.requiresCSRFValidation("POST", path, sessionReq); !got { + t.Errorf("path %s with session: expected requiresCSRFValidation=true (session flow requires CSRF), got false (regression of #404)", path) + } + } +} + +// Regression guard: unconditionally-exempt paths (login, setup-admin, etc.) must +// remain exempt regardless of whether a session is present. +func TestRequiresCSRFValidation_AlwaysExemptPaths(t *testing.T) { + h := &Handler{} + + sessionReq := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "Authorization": "Bearer some-session-token", + }, + } + + alwaysExempt := []string{ + "/api/auth/login", + "/api/auth/setup-admin", + "/api/auth/forgot-password", + "/api/auth/reset-password", + "/api/register", + } + + for _, path := range alwaysExempt { + if got := h.requiresCSRFValidation("POST", path, sessionReq); got { + t.Errorf("always-exempt path %s: expected requiresCSRFValidation=false even with session, got true", path) + } + } +}