diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index c131c8c29..beea013d3 100644 --- a/internal/api/coverage_gaps_test.go +++ b/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")) } // --------------------------------------------------------------------------- diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d8700039a..ffeba9743 100644 --- a/internal/api/handler_purchases.go +++ b/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 diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 7fa74b4e0..bab4e4990 100644 --- a/internal/api/handler_purchases_test.go +++ b/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 diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 3d0dc851c..68d6ff277 100644 --- a/internal/api/middleware.go +++ b/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 { diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 557204e13..58edd4222 100644 --- a/internal/api/middleware_test.go +++ b/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"]) +}