sec: narrow CSRF exemption for approve/cancel to token-only requests

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

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)

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
 }
 

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