sec: narrow CSRF exemption for approve/cancel POST endpoints (closes #404)

internal/api/coverage_gaps_test.go

@@ -471,8 +471,15 @@ func TestHandler_requiresCSRFValidation(t *testing.T) {
 	// DELETE on protected endpoint requires CSRF
 	assert.True(t, h.requiresCSRFValidation("DELETE", "/api/plans/123"))
 
-	// POST on approve (token-based) is exempt
-	assert.False(t, h.requiresCSRFValidation("POST", "/api/purchases/approve/uuid"))
+	// 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"))
 }
 
 // ---------------------------------------------------------------------------

internal/api/handler_purchases.go

@@ -427,6 +427,13 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam
 	t0 := time.Now()
 	logging.Infof("purchase[%s]: approvePurchaseViaSession entry (auth=session)", execution.ExecutionID)
 
+	// These endpoints are AuthPublic so the outer middleware skips CSRF.
+	// Enforce it here for the session-authed sub-path: the session bearer
+	// token is cookie-equivalent and must be CSRF-protected.
+	if err := h.validateCSRF(ctx, req); err != nil {
+		return nil, NewClientError(403, "CSRF validation failed")
+	}
+
 	session, err := h.requireSession(ctx, req)
 	if err != nil {
 		return nil, err
@@ -582,6 +589,13 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction
 // those rows in the same commit so a crash between the two writes can't
 // leave the rec list hiding capacity the user already cancelled.
 func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (any, error) {
+	// These endpoints are AuthPublic so the outer middleware skips CSRF.
+	// Enforce it here for the session-authed sub-path: the session bearer
+	// token is cookie-equivalent and must be CSRF-protected.
+	if err := h.validateCSRF(ctx, req); err != nil {
+		return nil, NewClientError(403, "CSRF validation failed")
+	}
+
 	session, err := h.requireSession(ctx, req)
 	if err != nil {
 		return nil, err

internal/api/handler_purchases_test.go

@@ -300,6 +300,8 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T)
 	mockAuth := new(MockAuthService)
 	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil)
 	mockAuth.grantAdmin()
+	// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
 
 	mockPurchase := new(MockPurchaseManager)
 	// CRITICAL ASSERTION: session-authed approve goes through
@@ -344,6 +346,8 @@ func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T
 	mockAuth := new(MockAuthService)
 	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil)
 	mockAuth.grantAdmin()
+	// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
 
 	mockPurchase := new(MockPurchaseManager)
 	mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(errors.New("AWS RI purchase failed"))
@@ -462,6 +466,8 @@ func TestHandler_approvePurchase_AWSOrphanFallsThrough(t *testing.T) {
 	mockAuth := new(MockAuthService)
 	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil)
 	mockAuth.grantAdmin()
+	// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
 
 	mockPurchase := new(MockPurchaseManager)
 	// Guard does not fire; ApproveAndExecute is called normally.
@@ -500,6 +506,8 @@ func TestHandler_approvePurchase_NonOrphanUnchanged(t *testing.T) {
 	mockAuth := new(MockAuthService)
 	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil)
 	mockAuth.grantAdmin()
+	// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
 
 	mockPurchase := new(MockPurchaseManager)
 	mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil)
@@ -1976,6 +1984,12 @@ func buildSessionCancelHandler(exec *config.PurchaseExecution, session *Session,
 
 	mockAuth := new(MockAuthService)
 	mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil)
+	// cancelPurchaseViaSession enforces CSRF on the session-authed path (issue
+	// #404). sessionCancelReq carries no CSRF header, so ValidateCSRFToken is
+	// called with an empty csrfToken. Tests exercising the session-authed cancel
+	// branch must allow this call; deny tests that only exercise the RBAC check
+	// may never reach ValidateCSRFToken if the session is nil.
+	mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil).Maybe()
 	// Authorization is permission-based for every caller now (issue #907): even
 	// an Administrators-group member resolves cancel-any/cancel-own through
 	// HasPermissionAPI, so register the permission mocks unconditionally.
@@ -2252,11 +2266,15 @@ func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) {
 	mockConfig.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil)
 
 	handler := &Handler{config: mockConfig, auth: new(MockAuthService)}
-	// No Authorization header → 401, not 403. Tokenless + sessionless is
-	// the only state where the user can't reach either branch.
+	// No Authorization header → CSRF fires first (issue #404: cancelPurchaseViaSession
+	// now enforces CSRF before requireSession). A tokenless request can't provide a
+	// valid CSRF binding, so we get a 403 "CSRF validation failed".
 	_, err := handler.cancelPurchase(context.Background(), &events.LambdaFunctionURLRequest{}, cancelExecID, "")
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "no authorization token provided")
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a clientError, got: %v", err)
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, ce.Error(), "CSRF validation failed")
 }
 
 // TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate is the

internal/api/middleware.go

@@ -126,17 +126,24 @@ func (h *Handler) requiresCSRFValidation(method, path string) bool {
 		return false
 	}
 
-	// Auth endpoints that don't have a session yet are exempt
+	// 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{
 		"/api/auth/login",
 		"/api/auth/setup-admin",
 		"/api/auth/forgot-password",
 		"/api/auth/reset-password",
-		"/api/purchases/approve/",   // Token-based auth
-		"/api/purchases/cancel/",    // Token-based auth
-		"/api/ri-exchange/approve/", // Token-based auth
-		"/api/ri-exchange/reject/",  // Token-based auth
-		"/api/register",             // Public registration (no session)
+		"/api/register", // Public registration (no session)
 	}
 
 	for _, exempt := range csrfExemptPaths {

internal/api/middleware_test.go

@@ -5,8 +5,10 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/LeanerCloud/CUDly/internal/config"
 	"github.com/aws/aws-lambda-go/events"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestHandler_isPublicEndpoint(t *testing.T) {
@@ -238,3 +240,192 @@ func TestHandler_authenticate_BearerTokenWithAPIKey(t *testing.T) {
 	// Should be false because API key is configured and bearer token is invalid
 	assert.False(t, handler.authenticate(ctx, req))
 }
+
+// ---------------------------------------------------------------------------
+// CSRF boundary tests for session-authed approve/cancel (issue #404)
+// ---------------------------------------------------------------------------
+
+// TestApproveViaSession_RequiresCSRF asserts that a session-authenticated POST
+// to approvePurchaseViaSession is rejected when the request carries no CSRF
+// token.  Pre-fix, the blanket "/api/purchases/approve/" middleware exemption
+// meant this call succeeded without a CSRF token.
+func TestApproveViaSession_RequiresCSRF(t *testing.T) {
+	ctx := context.Background()
+	execID := "12345678-1234-1234-1234-123456789abc"
+	adminEmail := "admin@example.com"
+
+	mockConfig := new(MockConfigStore)
+	exec := &config.PurchaseExecution{
+		ExecutionID:     execID,
+		ApprovalToken:   "email-tok",
+		Status:          "pending",
+		Recommendations: []config.RecommendationRecord{{ID: "r1"}},
+	}
+	mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+
+	mockAuth := new(MockAuthService)
+	adminSession := &Session{Email: adminEmail}
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
+	// Authorization is group-membership-only after issue #907: the session must
+	// pass the approve-* HasPermissionAPI check to reach the CSRF guard, since
+	// the dispatcher authorizes before invoking approvePurchaseViaSession.
+	mockAuth.grantAdmin()
+	// CSRF token is empty → ValidateCSRFToken must return an error so the
+	// request is rejected. This is the critical regression assertion for #404.
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch"))
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	handler := &Handler{config: mockConfig, auth: mockAuth}
+
+	// Request with a valid session but NO CSRF token header.
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer sess-tok"},
+	}
+	_, err := handler.approvePurchase(ctx, req, execID, "")
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a ClientError, got: %v", err)
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, ce.Error(), "CSRF validation failed")
+}
+
+// TestApproveViaSession_PassesCSRF asserts that a session-authenticated POST
+// to approvePurchaseViaSession succeeds when a valid CSRF token is supplied.
+// This confirms the CSRF guard does not block the legitimate dashboard flow.
+func TestApproveViaSession_PassesCSRF(t *testing.T) {
+	ctx := context.Background()
+	execID := "12345678-1234-1234-1234-123456789abc"
+	adminEmail := "admin@example.com"
+
+	mockConfig := new(MockConfigStore)
+	exec := &config.PurchaseExecution{
+		ExecutionID:     execID,
+		ApprovalToken:   "email-tok",
+		Status:          "pending",
+		Recommendations: []config.RecommendationRecord{{ID: "r1"}},
+	}
+	mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+
+	mockAuth := new(MockAuthService)
+	adminSession := &Session{Email: adminEmail}
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
+	// Authorization is group-membership-only after issue #907: the session must
+	// pass the approve-* HasPermissionAPI check before the CSRF guard runs.
+	mockAuth.grantAdmin()
+	// Valid CSRF token supplied → ValidateCSRFToken succeeds.
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "csrf-abc").Return(nil)
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	mockPurchase := new(MockPurchaseManager)
+	mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil)
+
+	handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase}
+
+	// Request with a valid session AND a valid CSRF token header.
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{
+			"authorization": "Bearer sess-tok",
+			"x-csrf-token":  "csrf-abc",
+		},
+	}
+	result, err := handler.approvePurchase(ctx, req, execID, "")
+	require.NoError(t, err)
+	assert.Equal(t, "completed", result.(map[string]string)["status"])
+}
+
+// TestCancelViaSession_RequiresCSRF asserts that a session-authenticated POST
+// to cancelPurchaseViaSession is rejected when the request carries no CSRF
+// token.  Mirror of TestApproveViaSession_RequiresCSRF for the cancel path.
+func TestCancelViaSession_RequiresCSRF(t *testing.T) {
+	ctx := context.Background()
+	execID := "55555555-5555-5555-5555-555555555555"
+
+	mockConfig := new(MockConfigStore)
+	exec := &config.PurchaseExecution{
+		ExecutionID: execID,
+		Status:      "pending",
+	}
+	mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+
+	mockAuth := new(MockAuthService)
+	adminSession := &Session{Email: "admin@example.com"}
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
+	// Authorization is group-membership-only after issue #907: the session must
+	// pass the cancel-* HasPermissionAPI check to reach the CSRF guard, since
+	// the dispatcher authorizes before invoking cancelPurchaseViaSession.
+	mockAuth.grantAdmin()
+	// No CSRF token → ValidateCSRFToken must return error to block the request.
+	mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch"))
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	handler := &Handler{config: mockConfig, auth: mockAuth}
+
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer sess-tok"},
+	}
+	_, err := handler.cancelPurchase(ctx, req, execID, "")
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a ClientError, got: %v", err)
+	assert.Equal(t, 403, ce.code)
+	assert.Contains(t, ce.Error(), "CSRF validation failed")
+}
+
+// TestTokenOnlyApprove_BypassesCSRF asserts that the email-link token path
+// does NOT invoke approvePurchaseViaSession, so CSRF is not checked on that
+// code path. The token path uses authorizeApprovalAction which validates the
+// signed token directly -- it has no session cookie to forge.
+//
+// The caller must supply a session whose email matches the contact_email (the
+// approval gate added in PR #101 ensures only the intended inbox can confirm
+// via the email link). Because the session has no approve-* RBAC, the handler
+// falls through from the session-authed branch to the token branch.
+func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) {
+	ctx := context.Background()
+	execID := "12345678-1234-1234-1234-123456789abc"
+	contactEmail := "contact@example.com"
+
+	mockConfig := new(MockConfigStore)
+	accountID := "acct-1"
+	exec := &config.PurchaseExecution{
+		ExecutionID:   execID,
+		ApprovalToken: "email-token",
+		Status:        "pending",
+		Recommendations: []config.RecommendationRecord{
+			{ID: "r1", CloudAccountID: &accountID},
+		},
+	}
+	mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+	mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil
+	}
+	mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
+		NotificationEmail: &contactEmail,
+	}, nil)
+
+	// Session is present but has no approve-* permissions → RBAC denies,
+	// falls through to the token branch. CSRF is NOT called on the token
+	// branch (approvePurchaseViaSession is never reached).
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil)
+	mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe()
+	mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe()
+	t.Cleanup(func() { mockAuth.AssertExpectations(t) })
+
+	mockPurchase := new(MockPurchaseManager)
+	mockPurchase.On("ApproveExecution", ctx, execID, "email-token", contactEmail).Return(nil)
+
+	handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase}
+
+	// Session header present (so tryGetSession succeeds) but no CSRF header.
+	// The session has no approve-* permission → falls to token branch.
+	// No ValidateCSRFToken mock is registered here, so if the code ever
+	// called ValidateCSRFToken, the test would panic with "unexpected call".
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer sess-tok"},
+		// Intentionally no x-csrf-token header.
+	}
+	result, err := handler.approvePurchase(ctx, req, execID, "email-token")
+	require.NoError(t, err)
+	assert.Equal(t, "completed", result.(map[string]string)["status"])
+}