From f92c282ba73b0a500eeb9a25cc2a342ce70a6cca Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:17:45 +0200 Subject: [PATCH 1/3] sec(api): enforce CSRF on RI-exchange session-authed approve approveRIExchangeViaSession had no CSRF check at all. The endpoint is AuthPublic, so the generic middleware CSRF gate never runs for it (validateSecurityContext returns before requiresCSRFValidation is reached), same as purchases approve/cancel -- but unlike purchases, nothing enforced CSRF inside the handler either. A comment at middleware.go:288-289 claimed parity with the purchases path; it didn't hold. Confirmed live by execution with a purchases control in the same probe run (not committed): same shape, valid session, zero CSRF headers -- purchases correctly refused ("CSRF validation failed"), RI-exchange transitioned the record from pending to processing with no CSRF token at all. Filed as #1757. Adds h.validateCSRF(ctx, req) at the top of approveRIExchangeViaSession, mirroring approvePurchaseViaSession / cancelPurchaseViaSession exactly (including the explanatory comment about AuthPublic routes). Adds TestApproveRIExchangeViaSession_RequiresCSRF and its PassesCSRF companion, both driving the real handler.approveRIExchange entry point (not requiresCSRFValidation in isolation). Fixes four existing tests (TestApproveRIExchange_SessionAdmin, both subtests of TestApproveRIExchange_SessionApproveOwn, TestApproveRIExchange_ SessionActorStamped) that never supplied a CSRF token and would now fail against the fix. Corrects the middleware.go:288-289 comment, which asserted parity that didn't exist and is why this survived -- the next reader now sees where CSRF is actually enforced for this path. Adds a scope-limit comment to TestRequiresCSRFValidation_ TokenBasedPathsWithSession (kept, not deleted: it's the only test pinning requiresCSRFValidation's own per-path table, which would matter again if any of these routes stopped being AuthPublic) -- it calls requiresCSRFValidation directly, bypassing the isPublicEndpoint gate that keeps the real dispatch from ever consulting it for these paths, so a passing assertion there was never proof of end-to-end enforcement, and was misread as exactly that here. Systemic check: enumerated all 26 AuthPublic routes in router.go and read every handler with a session-authed branch. approvePurchase / cancelPurchase (protected via validateCSRF), revokeViaEmailToken (protected on its only session-authed mutating path), rejectRIExchange (no session-authed path at all -- unconditionally token-gated), unsubscribeHandler (no session concept -- pure signed-token, RFC 8058), submitRegistrationHandler (no session concept -- public submission, separate AuthAdmin approval step). approveRIExchange was the only gap. Fixes #1757. --- internal/api/handler_ri_exchange.go | 10 ++ internal/api/handler_ri_exchange_test.go | 31 +++++- internal/api/middleware.go | 8 ++ internal/api/middleware_test.go | 120 ++++++++++++++++++++++- 4 files changed, 161 insertions(+), 8 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 86224e207..8cfea2f86 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2103,6 +2103,16 @@ func (h *Handler) approveRIExchangeViaToken(ctx context.Context, id, token strin // The session parameter may be non-nil (already validated by the caller) or nil // (requireSession will validate it and return 401 if absent). func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.LambdaFunctionURLRequest, id string, session *Session) (any, error) { + // This endpoint is AuthPublic so the outer middleware skips CSRF. + // Enforce it here for the session-authed sub-path, mirroring + // approvePurchaseViaSession / cancelPurchaseViaSession: the session + // bearer token is cookie-equivalent and must be CSRF-protected (issue + // #1757 -- this call was previously reachable with no CSRF token at + // all, despite a comment claiming parity with the purchases path). + if err := h.validateCSRF(ctx, req); err != nil { + return nil, NewClientError(403, "CSRF validation failed") + } + var err error if session == nil { session, err = h.requireSession(ctx, req) diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 0d616249a..3720a879d 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -370,6 +370,10 @@ func TestApproveRIExchange_SessionAdmin(t *testing.T) { adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "admin-bearer").Return(adminSession, nil) mockAuth.grantAdminPurchaser() + // approveRIExchangeViaSession validates CSRF before anything else (issue + // #1757); a valid token must be supplied for this session-authed path + // to reach the record fetch at all. + mockAuth.On("ValidateCSRFToken", ctx, "admin-bearer", "csrf-abc").Return(nil) // authorizeSessionApproveRIExchange: admin role short-circuits (no HasPermissionAPI call) @@ -398,7 +402,10 @@ func TestApproveRIExchange_SessionAdmin(t *testing.T) { mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil) req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"authorization": "Bearer admin-bearer"}, + Headers: map[string]string{ + "authorization": "Bearer admin-bearer", + "x-csrf-token": "csrf-abc", + }, } _, err := h.approveRIExchange(ctx, req, id, "") require.NoError(t, err) @@ -427,6 +434,8 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { mockAuth.On("ValidateSession", ctx, "owner-bearer").Return(ownerSession, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveAny, auth.ResourcePurchases).Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveOwn, auth.ResourcePurchases).Return(true, nil) + // Issue #1757: CSRF must be validated before the record fetch. + mockAuth.On("ValidateCSRFToken", ctx, "owner-bearer", "csrf-abc").Return(nil) // authorizeSessionApproveRIExchange fetches record for ownership check mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ @@ -449,7 +458,10 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { mockStore.On("StampRIExchangeApprovedBy", ctx, id, ownerSession.Email).Return(nil) req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"authorization": "Bearer owner-bearer"}, + Headers: map[string]string{ + "authorization": "Bearer owner-bearer", + "x-csrf-token": "csrf-abc", + }, } _, err := h.approveRIExchange(ctx, req, id, "") require.NoError(t, err) @@ -465,6 +477,9 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { mockAuth.On("ValidateSession", ctx, "owner-bearer").Return(ownerSession, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveAny, auth.ResourcePurchases).Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, auth.ActionApproveOwn, auth.ResourcePurchases).Return(true, nil) + // Issue #1757: CSRF must be validated before the record fetch, so this + // test needs a valid token to reach the ownership check it targets. + mockAuth.On("ValidateCSRFToken", ctx, "owner-bearer", "csrf-abc").Return(nil) // authorizeSessionApproveRIExchange fetches record — creator does not match mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ @@ -475,7 +490,10 @@ func TestApproveRIExchange_SessionApproveOwn(t *testing.T) { }, nil) req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"authorization": "Bearer owner-bearer"}, + Headers: map[string]string{ + "authorization": "Bearer owner-bearer", + "x-csrf-token": "csrf-abc", + }, } _, err := h.approveRIExchange(ctx, req, id, "") require.Error(t, err) @@ -1758,6 +1776,8 @@ func TestApproveRIExchange_SessionActorStamped(t *testing.T) { adminSession := &Session{UserID: actorID, Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "admin-bearer").Return(adminSession, nil) mockAuth.grantAdminPurchaser() + // Issue #1757: CSRF must be validated before the record fetch. + mockAuth.On("ValidateCSRFToken", ctx, "admin-bearer", "csrf-abc").Return(nil) mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ ID: id, Status: "pending", ApprovalToken: "tok", SourceRIIDs: []string{"ri-1"}, PaymentDue: "10.00", @@ -1774,7 +1794,10 @@ func TestApproveRIExchange_SessionActorStamped(t *testing.T) { mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil) req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"authorization": "Bearer admin-bearer"}, + Headers: map[string]string{ + "authorization": "Bearer admin-bearer", + "x-csrf-token": "csrf-abc", + }, } _, err := (&Handler{config: mockStore, auth: mockAuth}).approveRIExchange(ctx, req, id, "") require.NoError(t, err) diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 7a9e054c3..1ad44b699 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -287,6 +287,14 @@ func (h *Handler) requiresCSRFValidation(method, path string, req *events.Lambda // // Similarly, /api/ri-exchange/approve/ and /api/ri-exchange/reject/ are // AuthPublic and therefore exempted by isPublicEndpoint(), not here. + // approve/ has a session-authed sub-path (approveRIExchangeViaSession) + // and, like the purchases functions above, enforces CSRF directly + // inside that function -- NOT here, and not automatically just because + // it is "similar" to purchases (issue #1757: this exemption used to + // exist in prose only, with no actual validateCSRF call backing it). + // reject/ has no session-authed path at all -- it is unconditionally + // gated on a constant-time comparison against the record's stored + // approval token, so CSRF does not apply to it. // // All exemptions use exact-match to prevent a path like // /api/register-malicious from bypassing CSRF via accidental prefix overlap. diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 018ac17df..e3edbff64 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -8,6 +8,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/config" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -421,6 +422,98 @@ func TestCancelViaSession_RequiresCSRF(t *testing.T) { assert.Contains(t, ce.Error(), "CSRF validation failed") } +// TestApproveRIExchangeViaSession_RequiresCSRF is the RI-exchange analog of +// TestApproveViaSession_RequiresCSRF (issue #1757). Before this fix, +// approveRIExchangeViaSession had no CSRF check at all -- the endpoint is +// AuthPublic, so the outer middleware skips CSRF (same as purchases), but +// unlike purchases nothing enforced it inside the handler. A comment at +// middleware.go claimed parity with the purchases path; it didn't hold. +// +// Asserts on the call to TransitionRIExchangeStatus directly, not just on +// the returned error: an error alone doesn't prove the state transition was +// prevented, only that something failed somewhere. +func TestApproveRIExchangeViaSession_RequiresCSRF(t *testing.T) { + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440020" + + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil) + mockAuth.grantAdmin() + // CSRF token is empty → ValidateCSRFToken must return an error so the + // request is rejected before ever fetching the exchange record. + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch")) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + h := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + _, err := h.approveRIExchange(ctx, req, id, "") + 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") + + // The state-changing assertion: CSRF must be checked before the record + // is even fetched, let alone transitioned. Both must be untouched. + mockStore.AssertNotCalled(t, "GetRIExchangeRecord", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +// TestApproveRIExchangeViaSession_PassesCSRF confirms the CSRF guard does +// not block the legitimate dashboard flow: a valid CSRF token still reaches +// TransitionRIExchangeStatus. Without this, "add the CSRF check" could ship +// a fix that also breaks the happy path. +func TestApproveRIExchangeViaSession_PassesCSRF(t *testing.T) { + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440021" + creatorID := "creator-uuid" + + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil) + // approve-any:purchases is carved out of admin:* (#923); grantAdminPurchaser + // grants it explicitly so this test reaches TransitionRIExchangeStatus + // without also depending on the ownership comparison, matching the sibling + // TestApproveRIExchange_SessionAdmin fixture. + mockAuth.grantAdminPurchaser() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "csrf-abc").Return(nil) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + ApprovalToken: "tok", + SourceRIIDs: []string{"ri-123"}, + PaymentDue: "100.00", + CreatedByUserID: &creatorID, + }, nil).Once() + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). + Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-123"}, PaymentDue: "100.00"}, nil) + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil) + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil) + mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil) + + h := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "authorization": "Bearer sess-tok", + "x-csrf-token": "csrf-abc", + }, + } + _, err := h.approveRIExchange(ctx, req, id, "") + require.NoError(t, err) + mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything) +} + // 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 @@ -480,10 +573,29 @@ func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) { 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. +// Regression test for #404: pins requiresCSRFValidation's own per-path +// decision for the approve/cancel/reject paths. +// +// IMPORTANT SCOPE LIMIT (issue #1757): this calls requiresCSRFValidation +// directly, bypassing validateSecurityContext's isPublicEndpoint gate. Every +// path in tokenPaths below is registered AuthPublic in router.go, so in the +// real request path isPublicEndpoint short-circuits BEFORE +// requiresCSRFValidation is ever reached -- this function's return value for +// these four paths is never actually consulted by the live dispatch. A +// passing assertion here is NOT proof that CSRF is enforced end-to-end; it +// was misread as exactly that for approveRIExchangeViaSession, which had no +// validateCSRF call of its own until this issue. The end-to-end guarantee +// for a given AuthPublic route comes only from that route's handler calling +// h.validateCSRF directly (see TestApproveViaSession_RequiresCSRF / +// TestCancelViaSession_RequiresCSRF / TestApproveRIExchangeViaSession_ +// RequiresCSRF, which drive the real handler.approvePurchase / +// cancelPurchase / approveRIExchange entry points instead). +// +// Kept rather than deleted: requiresCSRFValidation's per-path table is real +// logic that would matter again if any of these routes ever stopped being +// AuthPublic, and this is the only test that pins it. Deleting it would +// trade a mislabeled test for no coverage at all; the comment above is the +// fix for the mislabeling. func TestRequiresCSRFValidation_TokenBasedPathsWithSession(t *testing.T) { h := &Handler{} From b7d7c1c3b96bb6cdf1e2d5f291c113f2c2501563 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:24:39 +0200 Subject: [PATCH 2/3] test(api): make the CSRF regression test assert on the money-moving call TestApproveRIExchangeViaSession_RequiresCSRF only mocked ValidateCSRFToken; under mutation (CSRF guard removed) it failed via a panic on the first unmocked call (GetRIExchangeRecord) rather than through its own AssertNotCalled assertions, which never ran. A panic on an unrelated unmocked call proves the record wasn't fetched, not that the state transition (the actual money-moving call) was prevented -- the same class of fiction-vs-mechanism gap flagged on PR #1758. Adds the full happy-path fixture (.Maybe(), never meant to be consumed under the fix) so a reintroduced bug can run all the way to TransitionRIExchangeStatus instead of crashing early, and reorders the state-changing AssertNotCalled checks ahead of the error-shape assertions so they are the primary, always-run check. Re-verified by mutation (inverse-edit revert of the CSRF guard, not git checkout): fails cleanly with "TransitionRIExchangeStatus was called 1 time(s)... expected none" and the recorded call showing the pending->processing transition -- the assertion the test needs to protect is now the one that actually fires. --- internal/api/middleware_test.go | 49 +++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index e3edbff64..4f4d9f1e0 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -435,32 +435,69 @@ func TestCancelViaSession_RequiresCSRF(t *testing.T) { func TestApproveRIExchangeViaSession_RequiresCSRF(t *testing.T) { ctx := context.Background() id := "550e8400-e29b-41d4-a716-446655440020" + creatorID := "creator-uuid" mockStore := new(MockConfigStore) mockAuth := new(MockAuthService) adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // CSRF token is empty → ValidateCSRFToken must return an error so the // request is rejected before ever fetching the exchange record. mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch")) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + // Full happy-path fixture, registered but never meant to be consumed + // (.Maybe()): if the CSRF guard were ever removed or reordered, this lets + // execution run all the way to TransitionRIExchangeStatus instead of + // panicking on an earlier unmocked call. The assertion below on + // TransitionRIExchangeStatus is what must actually fail under that + // mutation -- an early panic on GetRIExchangeRecord would prove only + // that record wasn't reached, not that the state transition (the + // money-moving call) was prevented. Verified: reverting the CSRF guard + // with this fixture present makes the TransitionRIExchangeStatus + // AssertNotCalled fail cleanly rather than crashing on an earlier call. + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + ApprovalToken: "tok", + SourceRIIDs: []string{"ri-123"}, + PaymentDue: "100.00", + CreatedByUserID: &creatorID, + }, nil).Maybe() + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). + Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-123"}, PaymentDue: "100.00"}, nil).Maybe() + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil).Maybe() + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil).Maybe() + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil).Maybe() + mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil).Maybe() + h := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{"authorization": "Bearer sess-tok"}, } _, err := h.approveRIExchange(ctx, req, id, "") + + // The state-changing assertion comes FIRST and is the primary check: + // the money-moving call must never be reached, full stop. Checked ahead + // of (and independent of) the error-shape assertions below, using + // assert (not require) so it always runs and reports on its own even if + // the error-shape checks would also fail. This is the assertion the + // mutation verification (see fixture comment above) proved fails + // cleanly -- "An error is expected but got nil" plus a call-count + // mismatch on TransitionRIExchangeStatus -- when the CSRF guard is + // removed, rather than a panic on an earlier unmocked call. + mockStore.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "GetRIExchangeRecord", mock.Anything, mock.Anything) + 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") - - // The state-changing assertion: CSRF must be checked before the record - // is even fetched, let alone transitioned. Both must be untouched. - mockStore.AssertNotCalled(t, "GetRIExchangeRecord", mock.Anything, mock.Anything) - mockStore.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) } // TestApproveRIExchangeViaSession_PassesCSRF confirms the CSRF guard does From 6026bb3c76c1d32c0cd36db30ebccdaae46cc167 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 11 Aug 2026 02:20:20 +0200 Subject: [PATCH 3/3] sec(api): stop a CSRF rejection falling through to the token approve path Review finding F1. approveRIExchange falls back to the token flow when the session path returns 403, so email-link holders can still approve. That test is isPermissionDenied, which compares the status code alone, so a CSRF rejection was indistinguishable from a legitimate approve-own denial and the request proceeded on the token. Measured before fixing, session with no CSRF headers and a valid ?token=: err = TransitionRIExchangeStatus called = true ExecuteExchange called = true money moved transition actor was NIL = true audit attribution lost StampRIExchangeApprovedBy called = false Not a reopened CSRF hole: the caller still needs the unguessable ApprovalToken, which is constant-time compared and is sufficient authorization by design. But a legitimate session-authed approve carrying a token degraded to an unattributed system approval on a money path, and the code comment claimed parity with approvePurchaseViaSession, which returns its CSRF 403 directly rather than swallowing it. Shipping a #1757 fix whose comment overstates what the code does is the failure mode #1757 was about. CSRF rejection is now the errCSRFRejected sentinel. Note precisely what protects it: isPermissionDenied(errCSRFRejected) is still true, because the sentinel is a 403. What keeps it out of the fallback is the short-circuit ordering of || at the dispatch, with the errors.Is term first. "Reached first" rather than "unreachable", and the two have different failure modes: reordering that expression reopens F1. The new test drives that exact path, so a reorder fails it. Chose the sentinel over widening isPermissionDenied because that predicate has three other call sites in handler_purchases.go, and narrowing its meaning globally is a larger blast radius than this finding warrants. Verified the record-level approve-own denial still falls through, which is the case the fallback exists for. The new test drives the dispatch with a valid token, which the existing empty-token test cannot reach, and asserts the money-moving call is never made. Mutation-verified: replacing the sentinel with a plain NewClientError(403, ...) makes it fail on both the TransitionRIExchangeStatus assertion and "an error is expected but got nil", by assertion rather than by panic. Restored afterwards and confirmed byte-identical. Refs #1757 --- internal/api/handler_ri_exchange.go | 12 ++++- internal/api/handler_router.go | 8 ++++ internal/api/middleware_test.go | 68 +++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 2 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 8cfea2f86..2065c539a 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2057,7 +2057,15 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct } // Record-level RBAC denied (e.g. approve-own user is not the creator). // If a token is present, preserve legacy token flow; otherwise surface the error. - if token == "" || !isPermissionDenied(sessErr) { + // + // A CSRF rejection must NOT fall through, even with a token present. + // isPermissionDenied is a bare 403 test, so without this it cannot tell a + // forged cross-site request from a legitimate approve-own denial, and the + // request would proceed on the token alone: the exchange executes, but as + // an unattributed system approval (transitioned_by NULL, no approved_by + // stamp) on a money path. Checked first so the sentinel is never reached + // by the 403 test below. + if errors.Is(sessErr, errCSRFRejected) || token == "" || !isPermissionDenied(sessErr) { return nil, sessErr } case isPermissionDenied(err): @@ -2110,7 +2118,7 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L // #1757 -- this call was previously reachable with no CSRF token at // all, despite a comment claiming parity with the purchases path). if err := h.validateCSRF(ctx, req); err != nil { - return nil, NewClientError(403, "CSRF validation failed") + return nil, errCSRFRejected } var err error diff --git a/internal/api/handler_router.go b/internal/api/handler_router.go index 2889cf2d0..ec6dc6a6b 100644 --- a/internal/api/handler_router.go +++ b/internal/api/handler_router.go @@ -18,6 +18,14 @@ func (h *Handler) routeRequest(ctx context.Context, method, path string, req *ev // errNotFound is a sentinel error for 404 responses. var errNotFound = ¬FoundError{} +// errCSRFRejected marks a CSRF validation failure so it stays distinguishable +// from an authorization denial. Both surface as 403, and isPermissionDenied +// tests the code alone, so a dispatch that falls back to a token flow on 403 +// cannot otherwise tell a forged cross-site request from a legitimate +// approve-own denial. Compare with errors.Is before any 403 test that decides +// whether to continue rather than return (issue #1757). +var errCSRFRejected = NewClientError(403, "CSRF validation failed") + type notFoundError struct{} func (e *notFoundError) Error() string { diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 4f4d9f1e0..8fe01c55a 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -500,6 +500,74 @@ func TestApproveRIExchangeViaSession_RequiresCSRF(t *testing.T) { assert.Contains(t, ce.Error(), "CSRF validation failed") } +// TestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken covers the case +// the empty-token test above cannot reach: a session-authed approve that fails +// CSRF while carrying a VALID ?token=. +// +// approveRIExchange's dispatch falls back to the token flow on a 403 from the +// session path, so email-link holders can still approve. isPermissionDenied +// tests the status code alone, so without a distinguishable CSRF error a forged +// cross-site request is indistinguishable from a legitimate approve-own denial +// and proceeds on the token: the exchange executes, but as an unattributed +// system approval (transitioned_by NULL, no approved_by stamp) on a money path. +// +// Verified by mutation: replacing errCSRFRejected with a plain +// NewClientError(403, ...) makes the TransitionRIExchangeStatus assertion below +// fail, because control reaches approveRIExchangeViaToken. +func TestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken(t *testing.T) { + ctx := context.Background() + id := "550e8400-e29b-41d4-a716-446655440021" + creatorID := "creator-uuid" + + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil) + mockAuth.grantAdminPurchaser() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch")) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + // Same full happy-path fixture as the empty-token test, and for the same + // reason: registered .Maybe() so a broken guard runs on to the state + // transition and fails the assertion, rather than panicking earlier and + // proving only that some intermediate call was not reached. ApprovalToken + // matches the token passed below, so the token path would genuinely succeed + // if the CSRF failure were swallowed. + mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ + ID: id, + Status: "pending", + ApprovalToken: "tok", + SourceRIIDs: []string{"ri-123"}, + PaymentDue: "100.00", + CreatedByUserID: &creatorID, + }, nil).Maybe() + mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything). + Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-123"}, PaymentDue: "100.00"}, nil).Maybe() + mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil).Maybe() + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RIExchangeMaxDailyUSD: 1000, + RIExchangeMaxPerExchangeUSD: 500, + }, nil).Maybe() + mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil).Maybe() + mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil).Maybe() + + h := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + // Valid token, so the fallback path would succeed if CSRF were swallowed. + _, err := h.approveRIExchange(ctx, req, id, "tok") + + // Primary assertion: the money-moving call is never reached. + mockStore.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + + 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") +} + // TestApproveRIExchangeViaSession_PassesCSRF confirms the CSRF guard does // not block the legitimate dashboard flow: a valid CSRF token still reaches // TransitionRIExchangeStatus. Without this, "add the CSRF check" could ship