Skip to content

Commit 2964c87

Browse files
committed
sec: narrow CSRF exemption for approve/cancel POST endpoints (closes #404)
The blanket "/api/purchases/approve/" and "/api/purchases/cancel/" entries in requiresCSRFValidation's exempt list were correct for the original token-only (email-link) flow but became incorrect once the three-mode dispatch (issues #46/#286) introduced a session-authed branch. A logged-in user's browser could be CSRF-forged into a POST to these endpoints because the middleware skipped CSRF validation entirely. Fix: remove the blanket middleware exemption and instead call validateCSRF() at the top of approvePurchaseViaSession and cancelPurchaseViaSession -- the two functions that mutate state under session authentication. The token-only path (authorizeApprovalAction) is stateless and never calls these functions, so it remains unaffected. These routes are already listed in isPublicEndpoint(), so validateSecurity() short-circuits before requiresCSRFValidation() is reached on the email-link path; the middleware exemption was therefore doubly redundant for that path and actively harmful for the session path. Tests: 4 new boundary tests in middleware_test.go - TestApproveViaSession_RequiresCSRF: session POST without CSRF -> 403 - TestApproveViaSession_PassesCSRF: session POST with CSRF -> 200 - TestCancelViaSession_RequiresCSRF: session POST without CSRF -> 403 - TestTokenOnlyApprove_BypassesCSRF: token path never calls CSRF check Updated 6 existing tests to supply ValidateCSRFToken expectations on the session-authed branches they exercise. No migration needed.
1 parent 4956d66 commit 2964c87

5 files changed

Lines changed: 237 additions & 11 deletions

File tree

‎internal/api/coverage_gaps_test.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -461,8 +461,15 @@ func TestHandler_requiresCSRFValidation(t *testing.T) {
461461
// DELETE on protected endpoint requires CSRF
462462
assert.True(t, h.requiresCSRFValidation("DELETE", "/api/plans/123"))
463463

464-
// POST on approve (token-based) is exempt
465-
assert.False(t, h.requiresCSRFValidation("POST", "/api/purchases/approve/uuid"))
464+
// POST on approve/cancel routes: these are AuthPublic so validateSecurity
465+
// skips CSRF via isPublicEndpoint() before requiresCSRFValidation is
466+
// reached. The session-authed sub-paths (approvePurchaseViaSession /
467+
// cancelPurchaseViaSession) enforce CSRF directly inside the handler.
468+
// requiresCSRFValidation is therefore never consulted for these paths --
469+
// but if it ever were, it must NOT return false (that would silently
470+
// double-exempt). The middleware exemption was removed by issue #404.
471+
assert.True(t, h.requiresCSRFValidation("POST", "/api/purchases/approve/uuid"))
472+
assert.True(t, h.requiresCSRFValidation("POST", "/api/purchases/cancel/uuid"))
466473
}
467474

468475
// ---------------------------------------------------------------------------

‎internal/api/handler_purchases.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,13 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam
414414
t0 := time.Now()
415415
logging.Infof("purchase[%s]: approvePurchaseViaSession entry (auth=session)", execution.ExecutionID)
416416

417+
// These endpoints are AuthPublic so the outer middleware skips CSRF.
418+
// Enforce it here for the session-authed sub-path: the session bearer
419+
// token is cookie-equivalent and must be CSRF-protected.
420+
if err := h.validateCSRF(ctx, req); err != nil {
421+
return nil, NewClientError(403, "CSRF validation failed")
422+
}
423+
417424
session, err := h.requireSession(ctx, req)
418425
if err != nil {
419426
return nil, err
@@ -565,6 +572,13 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction
565572
// those rows in the same commit so a crash between the two writes can't
566573
// leave the rec list hiding capacity the user already cancelled.
567574
func (h *Handler) cancelPurchaseViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution) (any, error) {
575+
// These endpoints are AuthPublic so the outer middleware skips CSRF.
576+
// Enforce it here for the session-authed sub-path: the session bearer
577+
// token is cookie-equivalent and must be CSRF-protected.
578+
if err := h.validateCSRF(ctx, req); err != nil {
579+
return nil, NewClientError(403, "CSRF validation failed")
580+
}
581+
568582
session, err := h.requireSession(ctx, req)
569583
if err != nil {
570584
return nil, err

‎internal/api/handler_purchases_test.go‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,8 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T)
299299

300300
mockAuth := new(MockAuthService)
301301
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail, Role: "admin"}, nil)
302+
// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
303+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
302304

303305
mockPurchase := new(MockPurchaseManager)
304306
// CRITICAL ASSERTION: session-authed approve goes through
@@ -342,6 +344,8 @@ func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T
342344

343345
mockAuth := new(MockAuthService)
344346
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail, Role: "admin"}, nil)
347+
// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
348+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
345349

346350
mockPurchase := new(MockPurchaseManager)
347351
mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(errors.New("AWS RI purchase failed"))
@@ -457,6 +461,8 @@ func TestHandler_approvePurchase_AWSOrphanFallsThrough(t *testing.T) {
457461

458462
mockAuth := new(MockAuthService)
459463
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail, Role: "admin"}, nil)
464+
// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
465+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
460466

461467
mockPurchase := new(MockPurchaseManager)
462468
// Guard does not fire; ApproveAndExecute is called normally.
@@ -494,6 +500,8 @@ func TestHandler_approvePurchase_NonOrphanUnchanged(t *testing.T) {
494500

495501
mockAuth := new(MockAuthService)
496502
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail, Role: "admin"}, nil)
503+
// approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404).
504+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil)
497505

498506
mockPurchase := new(MockPurchaseManager)
499507
mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil)
@@ -1815,6 +1823,12 @@ func buildSessionCancelHandler(exec *config.PurchaseExecution, session *Session,
18151823

18161824
mockAuth := new(MockAuthService)
18171825
mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil)
1826+
// cancelPurchaseViaSession enforces CSRF on the session-authed path (issue
1827+
// #404). sessionCancelReq carries no CSRF header, so ValidateCSRFToken is
1828+
// called with an empty csrfToken. Tests exercising the session-authed cancel
1829+
// branch must allow this call; deny tests that only exercise the RBAC check
1830+
// may never reach ValidateCSRFToken if the session is nil.
1831+
mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil).Maybe()
18181832
if session != nil && session.Role != "admin" {
18191833
mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-any", "purchases").Return(hasAny, nil).Maybe()
18201834
mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-own", "purchases").Return(hasOwn, nil).Maybe()
@@ -2082,11 +2096,15 @@ func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) {
20822096
mockConfig.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil)
20832097

20842098
handler := &Handler{config: mockConfig, auth: new(MockAuthService)}
2085-
// No Authorization header → 401, not 403. Tokenless + sessionless is
2086-
// the only state where the user can't reach either branch.
2099+
// No Authorization header → CSRF fires first (issue #404: cancelPurchaseViaSession
2100+
// now enforces CSRF before requireSession). A tokenless request can't provide a
2101+
// valid CSRF binding, so we get a 403 "CSRF validation failed".
20872102
_, err := handler.cancelPurchase(context.Background(), &events.LambdaFunctionURLRequest{}, cancelExecID, "")
20882103
require.Error(t, err)
2089-
assert.Contains(t, err.Error(), "no authorization token provided")
2104+
ce, ok := IsClientError(err)
2105+
require.True(t, ok, "expected a clientError, got: %v", err)
2106+
assert.Equal(t, 403, ce.code)
2107+
assert.Contains(t, ce.Error(), "CSRF validation failed")
20902108
}
20912109

20922110
// TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate is the

‎internal/api/middleware.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -124,17 +124,24 @@ func (h *Handler) requiresCSRFValidation(method, path string) bool {
124124
return false
125125
}
126126

127-
// Auth endpoints that don't have a session yet are exempt
127+
// Auth endpoints that don't have a session yet are exempt.
128+
//
129+
// Note: /api/purchases/approve/ and /api/purchases/cancel/ are NOT listed
130+
// here. Those routes are AuthPublic and listed in isPublicEndpoint(), so
131+
// validateSecurity() short-circuits before requiresCSRFValidation() is
132+
// reached on the token-only (email-link) path. For the session-authed path
133+
// (approvePurchaseViaSession / cancelPurchaseViaSession), CSRF is enforced
134+
// directly inside those functions -- a blanket middleware exemption would
135+
// leave session-authenticated POSTs to these endpoints unprotected (CSRF).
136+
//
137+
// Similarly, /api/ri-exchange/approve/ and /api/ri-exchange/reject/ are
138+
// AuthPublic and therefore exempted by isPublicEndpoint(), not here.
128139
csrfExemptPaths := []string{
129140
"/api/auth/login",
130141
"/api/auth/setup-admin",
131142
"/api/auth/forgot-password",
132143
"/api/auth/reset-password",
133-
"/api/purchases/approve/", // Token-based auth
134-
"/api/purchases/cancel/", // Token-based auth
135-
"/api/ri-exchange/approve/", // Token-based auth
136-
"/api/ri-exchange/reject/", // Token-based auth
137-
"/api/register", // Public registration (no session)
144+
"/api/register", // Public registration (no session)
138145
}
139146

140147
for _, exempt := range csrfExemptPaths {

‎internal/api/middleware_test.go‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,10 @@ import (
55
"errors"
66
"testing"
77

8+
"github.com/LeanerCloud/CUDly/internal/config"
89
"github.com/aws/aws-lambda-go/events"
910
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
1012
)
1113

1214
func TestHandler_isPublicEndpoint(t *testing.T) {
@@ -240,3 +242,181 @@ func TestHandler_authenticate_BearerTokenWithAPIKey(t *testing.T) {
240242
// Should be false because API key is configured and bearer token is invalid
241243
assert.False(t, handler.authenticate(ctx, req))
242244
}
245+
246+
// ---------------------------------------------------------------------------
247+
// CSRF boundary tests for session-authed approve/cancel (issue #404)
248+
// ---------------------------------------------------------------------------
249+
250+
// TestApproveViaSession_RequiresCSRF asserts that a session-authenticated POST
251+
// to approvePurchaseViaSession is rejected when the request carries no CSRF
252+
// token. Pre-fix, the blanket "/api/purchases/approve/" middleware exemption
253+
// meant this call succeeded without a CSRF token.
254+
func TestApproveViaSession_RequiresCSRF(t *testing.T) {
255+
ctx := context.Background()
256+
execID := "12345678-1234-1234-1234-123456789abc"
257+
adminEmail := "admin@example.com"
258+
259+
mockConfig := new(MockConfigStore)
260+
exec := &config.PurchaseExecution{
261+
ExecutionID: execID,
262+
ApprovalToken: "email-tok",
263+
Status: "pending",
264+
Recommendations: []config.RecommendationRecord{{ID: "r1"}},
265+
}
266+
mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
267+
268+
mockAuth := new(MockAuthService)
269+
adminSession := &Session{Email: adminEmail, Role: "admin"}
270+
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
271+
// CSRF token is empty → ValidateCSRFToken must return an error so the
272+
// request is rejected. This is the critical regression assertion for #404.
273+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch"))
274+
t.Cleanup(func() { mockAuth.AssertExpectations(t) })
275+
276+
handler := &Handler{config: mockConfig, auth: mockAuth}
277+
278+
// Request with a valid session but NO CSRF token header.
279+
req := &events.LambdaFunctionURLRequest{
280+
Headers: map[string]string{"authorization": "Bearer sess-tok"},
281+
}
282+
_, err := handler.approvePurchase(ctx, req, execID, "")
283+
require.Error(t, err)
284+
ce, ok := IsClientError(err)
285+
require.True(t, ok, "expected a ClientError, got: %v", err)
286+
assert.Equal(t, 403, ce.code)
287+
assert.Contains(t, ce.Error(), "CSRF validation failed")
288+
}
289+
290+
// TestApproveViaSession_PassesCSRF asserts that a session-authenticated POST
291+
// to approvePurchaseViaSession succeeds when a valid CSRF token is supplied.
292+
// This confirms the CSRF guard does not block the legitimate dashboard flow.
293+
func TestApproveViaSession_PassesCSRF(t *testing.T) {
294+
ctx := context.Background()
295+
execID := "12345678-1234-1234-1234-123456789abc"
296+
adminEmail := "admin@example.com"
297+
298+
mockConfig := new(MockConfigStore)
299+
exec := &config.PurchaseExecution{
300+
ExecutionID: execID,
301+
ApprovalToken: "email-tok",
302+
Status: "pending",
303+
Recommendations: []config.RecommendationRecord{{ID: "r1"}},
304+
}
305+
mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
306+
307+
mockAuth := new(MockAuthService)
308+
adminSession := &Session{Email: adminEmail, Role: "admin"}
309+
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
310+
// Valid CSRF token supplied → ValidateCSRFToken succeeds.
311+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "csrf-abc").Return(nil)
312+
t.Cleanup(func() { mockAuth.AssertExpectations(t) })
313+
314+
mockPurchase := new(MockPurchaseManager)
315+
mockPurchase.On("ApproveAndExecute", ctx, execID, adminEmail).Return(nil)
316+
317+
handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase}
318+
319+
// Request with a valid session AND a valid CSRF token header.
320+
req := &events.LambdaFunctionURLRequest{
321+
Headers: map[string]string{
322+
"authorization": "Bearer sess-tok",
323+
"x-csrf-token": "csrf-abc",
324+
},
325+
}
326+
result, err := handler.approvePurchase(ctx, req, execID, "")
327+
require.NoError(t, err)
328+
assert.Equal(t, "completed", result.(map[string]string)["status"])
329+
}
330+
331+
// TestCancelViaSession_RequiresCSRF asserts that a session-authenticated POST
332+
// to cancelPurchaseViaSession is rejected when the request carries no CSRF
333+
// token. Mirror of TestApproveViaSession_RequiresCSRF for the cancel path.
334+
func TestCancelViaSession_RequiresCSRF(t *testing.T) {
335+
ctx := context.Background()
336+
execID := "55555555-5555-5555-5555-555555555555"
337+
338+
mockConfig := new(MockConfigStore)
339+
exec := &config.PurchaseExecution{
340+
ExecutionID: execID,
341+
Status: "pending",
342+
}
343+
mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
344+
345+
mockAuth := new(MockAuthService)
346+
adminSession := &Session{Email: "admin@example.com", Role: "admin"}
347+
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
348+
// No CSRF token → ValidateCSRFToken must return error to block the request.
349+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch"))
350+
t.Cleanup(func() { mockAuth.AssertExpectations(t) })
351+
352+
handler := &Handler{config: mockConfig, auth: mockAuth}
353+
354+
req := &events.LambdaFunctionURLRequest{
355+
Headers: map[string]string{"authorization": "Bearer sess-tok"},
356+
}
357+
_, err := handler.cancelPurchase(ctx, req, execID, "")
358+
require.Error(t, err)
359+
ce, ok := IsClientError(err)
360+
require.True(t, ok, "expected a ClientError, got: %v", err)
361+
assert.Equal(t, 403, ce.code)
362+
assert.Contains(t, ce.Error(), "CSRF validation failed")
363+
}
364+
365+
// TestTokenOnlyApprove_BypassesCSRF asserts that the email-link token path
366+
// does NOT invoke approvePurchaseViaSession, so CSRF is not checked on that
367+
// code path. The token path uses authorizeApprovalAction which validates the
368+
// signed token directly -- it has no session cookie to forge.
369+
//
370+
// The caller must supply a session whose email matches the contact_email (the
371+
// approval gate added in PR #101 ensures only the intended inbox can confirm
372+
// via the email link). Because the session has no approve-* RBAC, the handler
373+
// falls through from the session-authed branch to the token branch.
374+
func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) {
375+
ctx := context.Background()
376+
execID := "12345678-1234-1234-1234-123456789abc"
377+
contactEmail := "contact@example.com"
378+
379+
mockConfig := new(MockConfigStore)
380+
accountID := "acct-1"
381+
exec := &config.PurchaseExecution{
382+
ExecutionID: execID,
383+
ApprovalToken: "email-token",
384+
Status: "pending",
385+
Recommendations: []config.RecommendationRecord{
386+
{ID: "r1", CloudAccountID: &accountID},
387+
},
388+
}
389+
mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil)
390+
mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
391+
return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil
392+
}
393+
mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
394+
NotificationEmail: &contactEmail,
395+
}, nil)
396+
397+
// Session is present but has no approve-* permissions → RBAC denies,
398+
// falls through to the token branch. CSRF is NOT called on the token
399+
// branch (approvePurchaseViaSession is never reached).
400+
mockAuth := new(MockAuthService)
401+
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil)
402+
mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe()
403+
mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe()
404+
t.Cleanup(func() { mockAuth.AssertExpectations(t) })
405+
406+
mockPurchase := new(MockPurchaseManager)
407+
mockPurchase.On("ApproveExecution", ctx, execID, "email-token", contactEmail).Return(nil)
408+
409+
handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase}
410+
411+
// Session header present (so tryGetSession succeeds) but no CSRF header.
412+
// The session has no approve-* permission → falls to token branch.
413+
// No ValidateCSRFToken mock is registered here, so if the code ever
414+
// called ValidateCSRFToken, the test would panic with "unexpected call".
415+
req := &events.LambdaFunctionURLRequest{
416+
Headers: map[string]string{"authorization": "Bearer sess-tok"},
417+
// Intentionally no x-csrf-token header.
418+
}
419+
result, err := handler.approvePurchase(ctx, req, execID, "email-token")
420+
require.NoError(t, err)
421+
assert.Equal(t, "completed", result.(map[string]string)["status"])
422+
}

0 commit comments

Comments
 (0)