Skip to content

Commit 6026bb3

Browse files
committed
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 = <nil> 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
1 parent b7d7c1c commit 6026bb3

3 files changed

Lines changed: 86 additions & 2 deletions

File tree

‎internal/api/handler_ri_exchange.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2057,7 +2057,15 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct
20572057
}
20582058
// Record-level RBAC denied (e.g. approve-own user is not the creator).
20592059
// If a token is present, preserve legacy token flow; otherwise surface the error.
2060-
if token == "" || !isPermissionDenied(sessErr) {
2060+
//
2061+
// A CSRF rejection must NOT fall through, even with a token present.
2062+
// isPermissionDenied is a bare 403 test, so without this it cannot tell a
2063+
// forged cross-site request from a legitimate approve-own denial, and the
2064+
// request would proceed on the token alone: the exchange executes, but as
2065+
// an unattributed system approval (transitioned_by NULL, no approved_by
2066+
// stamp) on a money path. Checked first so the sentinel is never reached
2067+
// by the 403 test below.
2068+
if errors.Is(sessErr, errCSRFRejected) || token == "" || !isPermissionDenied(sessErr) {
20612069
return nil, sessErr
20622070
}
20632071
case isPermissionDenied(err):
@@ -2110,7 +2118,7 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L
21102118
// #1757 -- this call was previously reachable with no CSRF token at
21112119
// all, despite a comment claiming parity with the purchases path).
21122120
if err := h.validateCSRF(ctx, req); err != nil {
2113-
return nil, NewClientError(403, "CSRF validation failed")
2121+
return nil, errCSRFRejected
21142122
}
21152123

21162124
var err error

‎internal/api/handler_router.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,14 @@ func (h *Handler) routeRequest(ctx context.Context, method, path string, req *ev
1818
// errNotFound is a sentinel error for 404 responses.
1919
var errNotFound = &notFoundError{}
2020

21+
// errCSRFRejected marks a CSRF validation failure so it stays distinguishable
22+
// from an authorization denial. Both surface as 403, and isPermissionDenied
23+
// tests the code alone, so a dispatch that falls back to a token flow on 403
24+
// cannot otherwise tell a forged cross-site request from a legitimate
25+
// approve-own denial. Compare with errors.Is before any 403 test that decides
26+
// whether to continue rather than return (issue #1757).
27+
var errCSRFRejected = NewClientError(403, "CSRF validation failed")
28+
2129
type notFoundError struct{}
2230

2331
func (e *notFoundError) Error() string {

‎internal/api/middleware_test.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,74 @@ func TestApproveRIExchangeViaSession_RequiresCSRF(t *testing.T) {
500500
assert.Contains(t, ce.Error(), "CSRF validation failed")
501501
}
502502

503+
// TestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken covers the case
504+
// the empty-token test above cannot reach: a session-authed approve that fails
505+
// CSRF while carrying a VALID ?token=.
506+
//
507+
// approveRIExchange's dispatch falls back to the token flow on a 403 from the
508+
// session path, so email-link holders can still approve. isPermissionDenied
509+
// tests the status code alone, so without a distinguishable CSRF error a forged
510+
// cross-site request is indistinguishable from a legitimate approve-own denial
511+
// and proceeds on the token: the exchange executes, but as an unattributed
512+
// system approval (transitioned_by NULL, no approved_by stamp) on a money path.
513+
//
514+
// Verified by mutation: replacing errCSRFRejected with a plain
515+
// NewClientError(403, ...) makes the TransitionRIExchangeStatus assertion below
516+
// fail, because control reaches approveRIExchangeViaToken.
517+
func TestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken(t *testing.T) {
518+
ctx := context.Background()
519+
id := "550e8400-e29b-41d4-a716-446655440021"
520+
creatorID := "creator-uuid"
521+
522+
mockStore := new(MockConfigStore)
523+
mockAuth := new(MockAuthService)
524+
adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"}
525+
mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil)
526+
mockAuth.grantAdminPurchaser()
527+
mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch"))
528+
t.Cleanup(func() { mockAuth.AssertExpectations(t) })
529+
530+
// Same full happy-path fixture as the empty-token test, and for the same
531+
// reason: registered .Maybe() so a broken guard runs on to the state
532+
// transition and fails the assertion, rather than panicking earlier and
533+
// proving only that some intermediate call was not reached. ApprovalToken
534+
// matches the token passed below, so the token path would genuinely succeed
535+
// if the CSRF failure were swallowed.
536+
mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{
537+
ID: id,
538+
Status: "pending",
539+
ApprovalToken: "tok",
540+
SourceRIIDs: []string{"ri-123"},
541+
PaymentDue: "100.00",
542+
CreatedByUserID: &creatorID,
543+
}, nil).Maybe()
544+
mockStore.On("TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything).
545+
Return(&config.RIExchangeRecord{ID: id, Status: "processing", SourceRIIDs: []string{"ri-123"}, PaymentDue: "100.00"}, nil).Maybe()
546+
mockStore.On("GetRIExchangeDailySpend", mock.Anything, mock.Anything).Return("0", nil).Maybe()
547+
mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{
548+
RIExchangeMaxDailyUSD: 1000,
549+
RIExchangeMaxPerExchangeUSD: 500,
550+
}, nil).Maybe()
551+
mockStore.On("FailRIExchange", ctx, id, mock.AnythingOfType("string")).Return(nil).Maybe()
552+
mockStore.On("StampRIExchangeApprovedBy", ctx, id, adminSession.Email).Return(nil).Maybe()
553+
554+
h := &Handler{config: mockStore, auth: mockAuth}
555+
req := &events.LambdaFunctionURLRequest{
556+
Headers: map[string]string{"authorization": "Bearer sess-tok"},
557+
}
558+
// Valid token, so the fallback path would succeed if CSRF were swallowed.
559+
_, err := h.approveRIExchange(ctx, req, id, "tok")
560+
561+
// Primary assertion: the money-moving call is never reached.
562+
mockStore.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
563+
564+
require.Error(t, err)
565+
ce, ok := IsClientError(err)
566+
require.True(t, ok, "expected a ClientError, got: %v", err)
567+
assert.Equal(t, 403, ce.code)
568+
assert.Contains(t, ce.Error(), "CSRF validation failed")
569+
}
570+
503571
// TestApproveRIExchangeViaSession_PassesCSRF confirms the CSRF guard does
504572
// not block the legitimate dashboard flow: a valid CSRF token still reaches
505573
// TransitionRIExchangeStatus. Without this, "add the CSRF check" could ship

0 commit comments

Comments
 (0)