From fea837d1864a53bffddf93b56a616967aa4b0332 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 01:49:53 +0100 Subject: [PATCH 1/3] fix(api): answer expired and lost-race token actions alike on revoke and RI exchange Approve and cancel already answer an emailed-token action with 403 for an invalid token, 410 for an expired one and 409 for a wrong status. Revoke answered an expired link with 409, and read the expiry before matching the token. The RI exchange approve and reject links never checked expires_at: the transition CAS refused an expired or already-moved record with a plain error, so both came back as 500. - validateRevokeToken reuses purchase.ValidateApprovalToken: token matched first, then 410 for an expired link, then the revocation window (403). - checkExchangeToken gives RI approve and reject the same 403/410 order. - TransitionRIExchangeStatus wraps ErrRIExchangeExpired and ErrRIExchangeNotInExpectedStatus; riExchangeTransitionError maps them to 410 and 409 on the approve (token and session) and reject paths. - openapi.yaml documents the 410s and the 409 race. Closes #587 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 37 +++--- internal/api/handler_purchases_test.go | 9 +- .../handler_purchases_token_errors_test.go | 10 +- internal/api/handler_ri_exchange.go | 75 +++++++---- internal/api/openapi.yaml | 38 +++++- .../ri_exchange_approve_reject_status_test.go | 57 ++++++++- internal/config/errors.go | 12 ++ internal/config/interfaces.go | 2 + internal/config/store_postgres.go | 7 +- .../config/store_postgres_pgxmock_test.go | 6 + ...ri_exchange_transition_integration_test.go | 117 ++++++++++++++++++ 11 files changed, 311 insertions(+), 59 deletions(-) create mode 100644 internal/config/store_postgres_ri_exchange_transition_integration_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d90c7282b..a10bd5f09 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1872,33 +1872,30 @@ func checkRevokableStatus(execution *config.PurchaseExecution) error { } } -// validateRevokeToken checks that the execution carries a non-empty -// ApprovalToken, that the token has not expired, that the revocation window -// has not closed (config.RevocationWindow after CompletedAt), and that the -// token matches using constant-time comparison. Pre-migration rows without an -// ApprovalTokenExpiresAt pass the expiry check (legacy backward compat). -// Extracted from revokeViaEmailToken to keep that function under the cyclomatic limit. +// validateRevokeToken checks the revocation token with the same status +// mapping as the approve and cancel links (tokenActionError, issue #587): a +// missing or non-matching token is 403 and a matching but expired token is +// 410. It shares purchase.ValidateApprovalToken, so the token is matched (in +// constant time, against the SHA-256 digest stored at rest) before its expiry +// is read and only the token holder learns that the link expired. +// Pre-migration rows without an ApprovalTokenExpiresAt pass the expiry check +// (legacy backward compat). Last, the 24-hour revocation window must still be +// open (config.RevocationWindow after CompletedAt). Extracted from +// revokeViaEmailToken to keep that function under the cyclomatic limit. func validateRevokeToken(execution *config.PurchaseExecution, token string) error { - if execution.ApprovalToken == "" { + switch err := purchase.ValidateApprovalToken(execution, token); { + case errors.Is(err, purchase.ErrInvalidApprovalToken): return NewClientError(403, "invalid revocation token") - } - if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) { - return NewClientError(409, fmt.Sprintf( + case errors.Is(err, purchase.ErrApprovalTokenExpired): + return NewClientError(410, fmt.Sprintf( "execution %s cannot be revoked: the revocation link has expired", execution.ExecutionID)) + case err != nil: + return err } // Enforce the 24-hour revocation window (issue #291 Finding #6). // Use CompletedAt as the reference timestamp; fall back to ExecutedAt // for direct-execute rows which may not have a CompletedAt yet. - if windowErr := checkRevocationWindow(execution); windowErr != nil { - return windowErr - } - // execution.ApprovalToken is the SHA-256 hex digest stored at rest - // (issue #103), not the raw secret; config.ApprovalTokenMatches hashes - // the supplied token and compares digests in constant time. - if !config.ApprovalTokenMatches(execution.ApprovalToken, token) { - return NewClientError(403, "invalid revocation token") - } - return nil + return checkRevocationWindow(execution) } // checkRevocationWindow returns a 403 ClientError when the 24-hour revocation diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 2d6e7346c..37437dee9 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -5896,8 +5896,9 @@ func TestHandler_revokePurchase_NotFound(t *testing.T) { } // TestHandler_revokePurchase_ExpiredToken verifies that a matching token whose -// ApprovalTokenExpiresAt deadline has passed is rejected with 409, so a stale -// completed execution cannot be marked revocation_requested via an old link. +// ApprovalTokenExpiresAt deadline has passed is rejected with 410 (409 before +// issue #587 aligned revoke with approve and cancel), so a stale completed +// execution cannot be marked revocation_requested via an old link. // Regression test for the revoke-token expiry CR finding (PR #889). func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { ctx := context.Background() @@ -5909,7 +5910,7 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { exec := &config.PurchaseExecution{ ExecutionID: execID, - ApprovalToken: token, + ApprovalToken: config.HashApprovalToken(token), // stored as its digest (issue #103) ApprovalTokenExpiresAt: &past, Status: "completed", Recommendations: []config.RecommendationRecord{ @@ -5942,7 +5943,7 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { require.Error(t, err, "expired revocation token must be rejected") ce, ok := IsClientError(err) require.True(t, ok, "expected a client error") - assert.Equal(t, 409, ce.code) + assert.Equal(t, 410, ce.code) assert.Contains(t, ce.message, "expired") } diff --git a/internal/api/handler_purchases_token_errors_test.go b/internal/api/handler_purchases_token_errors_test.go index 91d471794..71a6da9e2 100644 --- a/internal/api/handler_purchases_token_errors_test.go +++ b/internal/api/handler_purchases_token_errors_test.go @@ -17,7 +17,8 @@ import ( // Email-link approve/cancel and the email revoke endpoint are driven through // HandleRequest with the real purchase.Manager so the real token and status -// errors reach the HTTP mapping (issue #564). +// errors reach the HTTP mapping (issue #564). All three share one mapping: +// invalid token 403, expired token 410, wrong status 409 (issue #587). const tokenErrExecID = "12345678-1234-1234-1234-123456789abc" const tokenErrRawToken = "raw-secret-token" @@ -85,6 +86,13 @@ func TestHandleRequest_TokenActions_TokenAndStatusErrorsAre4xx(t *testing.T) { {"cancel invalid token", "cancel", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, "wrong", 403}, {"cancel expired token", "cancel", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410}, {"cancel non-cancelable status", "cancel", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, tokenErrRawToken, 409}, + // Issue #587: revoke uses the same mapping as approve and cancel, and + // the token is matched before its expiry is read. + {"approve wrong token on an expired link", "approve", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, "wrong", 403}, + {"revoke invalid token", "revoke", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, "wrong", 403}, + {"revoke expired token", "revoke", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410}, + {"revoke wrong token on an expired link", "revoke", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, "wrong", 403}, + {"revoke non-revocable status", "revoke", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, tokenErrRawToken, 409}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 89a961297..8f54d04ba 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2334,10 +2334,10 @@ func (h *Handler) approveRIExchangeViaToken(ctx context.Context, req *events.Lam transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", resolveCreatorUserID(session)) if err != nil { - return nil, fmt.Errorf("failed to transition exchange status: %w", err) + return nil, riExchangeTransitionError(err, riExchangeApproveConflictMsg) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was canceled by a newer analysis run") + return nil, NewClientError(409, riExchangeApproveConflictMsg) } result, execErr := h.executeApprovedExchange(ctx, id, record) @@ -2397,10 +2397,10 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L // Session-authed approval: stamp the session user as the actor. transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", "processing", resolveCreatorUserID(session)) if err != nil { - return nil, fmt.Errorf("failed to transition exchange status: %w", err) + return nil, riExchangeTransitionError(err, riExchangeApproveConflictMsg) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was canceled by a newer analysis run") + return nil, NewClientError(409, riExchangeApproveConflictMsg) } result, execErr := h.executeApprovedExchange(ctx, id, record) @@ -2615,19 +2615,56 @@ func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string return nil, NewClientError(404, "exchange record not found") } - if record.ApprovalToken == "" { - return nil, NewClientError(403, "this exchange record does not support approval") + if err := checkExchangeToken(record, token, "approval"); err != nil { + return nil, err } + return record, nil +} + +const ( + riExchangeApproveConflictMsg = "exchange already processed, expired, or was canceled by a newer analysis run" + riExchangeRejectConflictMsg = "exchange already processed, expired, or was canceled" + riExchangeExpiredMsg = "exchange has expired" +) - // record.ApprovalToken is the SHA-256 hex digest stored at rest - // (issue #103); hash the supplied token and compare digests in - // constant time, matching config.ApprovalTokenMatches everywhere else - // in this codebase that validates one of these tokens. +// checkExchangeToken applies the emailed-token status mapping shared with the +// purchase approve, cancel and revoke links (issue #587): a record without a +// token, or a token that does not match, is 403, and a matching token on an +// exchange past its expires_at is 410. The match is checked first, so only +// the token holder learns that the exchange expired. kind is "approval" or +// "rejection" and only shapes the 403 text. +// +// record.ApprovalToken is the SHA-256 hex digest stored at rest (issue #103); +// config.ApprovalTokenMatches hashes the supplied token and compares digests +// in constant time, as everywhere else that validates one of these tokens. +func checkExchangeToken(record *config.RIExchangeRecord, token, kind string) error { + if record.ApprovalToken == "" { + return NewClientError(403, "this exchange record does not support "+kind) + } if !config.ApprovalTokenMatches(record.ApprovalToken, token) { - return nil, NewClientError(403, "invalid approval token") + return NewClientError(403, "invalid "+kind+" token") + } + if record.ExpiresAt != nil && time.Now().After(*record.ExpiresAt) { + return NewClientError(410, riExchangeExpiredMsg) } + return nil +} - return record, nil +// riExchangeTransitionError maps a lost pending -> X transition: the record +// expired (410) or another writer moved its status first (409), the same +// codes as an expired or already-handled purchase email link (issue #587). +// The expiry pre-check in checkExchangeToken cannot close the window alone: +// the record can expire, or a concurrent approve or reject can win, between +// that check and the transition. Anything else stays a server error. +func riExchangeTransitionError(err error, conflictMsg string) error { + switch { + case errors.Is(err, config.ErrRIExchangeExpired): + return NewClientError(410, riExchangeExpiredMsg) + case errors.Is(err, config.ErrRIExchangeNotInExpectedStatus): + return NewClientError(409, conflictMsg) + default: + return fmt.Errorf("failed to transition exchange status: %w", err) + } } // failExchange marks an exchange as failed, logging if the DB write also fails. @@ -2783,23 +2820,17 @@ func (h *Handler) rejectRIExchange(ctx context.Context, id, token string) (any, return nil, NewClientError(404, "exchange record not found") } - if record.ApprovalToken == "" { - return nil, NewClientError(403, "this exchange record does not support rejection") - } - - // See validateExchangeApproval: record.ApprovalToken is a SHA-256 hex - // digest (issue #103), not the raw secret. - if !config.ApprovalTokenMatches(record.ApprovalToken, token) { - return nil, NewClientError(403, "invalid rejection token") + if tokenErr := checkExchangeToken(record, token, "rejection"); tokenErr != nil { + return nil, tokenErr } // Token-based rejection: no session user, so transitioned_by = NULL. transitioned, err := h.config.TransitionRIExchangeStatus(ctx, id, "pending", config.StatusCanceled, nil) if err != nil { - return nil, fmt.Errorf("failed to transition exchange status: %w", err) + return nil, riExchangeTransitionError(err, riExchangeRejectConflictMsg) } if transitioned == nil { - return nil, NewClientError(409, "exchange already processed, expired, or was canceled") + return nil, NewClientError(409, riExchangeRejectConflictMsg) } return map[string]string{"status": config.StatusCanceled}, nil diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 0865e46c3..50c1d8fec 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -945,7 +945,16 @@ paths: '409': description: > The execution status cannot be revoked (still pending or - notified, or not completed), or the revocation link has expired. + notified, or not completed), or its status changed concurrently. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '410': + description: > + Token mode only, the revocation token matches but has expired. + The token is matched first, so a wrong token on an expired link + is 403, as on approve and cancel. content: application/json: schema: @@ -1544,8 +1553,19 @@ paths: $ref: '#/components/responses/NotFound' '409': description: > - The exchange is no longer pending (already processed, expired or - canceled by a newer analysis run). + The exchange is no longer pending (already processed, or canceled + by a newer analysis run), including when a concurrent approve or + reject changed it first. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '410': + description: > + The exchange has expired (its expires_at has passed). Token mode + checks this once the token matches, so a wrong token on an + expired exchange is 403; both modes also get 410 when the + exchange expires before the atomic transition. content: application/json: schema: @@ -1598,7 +1618,17 @@ paths: '404': $ref: '#/components/responses/NotFound' '409': - description: The exchange is no longer pending (already processed, expired or canceled). + description: > + The exchange is no longer pending (already processed or canceled), + including when a concurrent approve or reject changed it first. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + '410': + description: > + The rejection token matches but the exchange has expired (its + expires_at has passed). A wrong token on an expired exchange is 403. content: application/json: schema: diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index 84519eea6..06ccab267 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "errors" + "fmt" "testing" + "time" "github.com/aws/aws-lambda-go/events" "github.com/stretchr/testify/assert" @@ -28,8 +30,36 @@ const ( riRecNoToken riRecMissing riRecAlreadyProcessed + // riRecExpired is pending but past its expires_at. Its transition is + // mocked to succeed, so a 410 proves the token check answered before + // any write (the real CAS would refuse too; see riRecTransitionExpired). + riRecExpired + // riRecTransitionExpired expires between the token check and the + // transition: the store reports ErrRIExchangeExpired, as Postgres does. + riRecTransitionExpired + // riRecTransitionConflict loses the transition to a concurrent approve or + // reject: the store reports ErrRIExchangeNotInExpectedStatus. + riRecTransitionConflict + // riRecTransitionFails hits a store error unrelated to the record state. + riRecTransitionFails ) +// riStatusTransition returns the mocked TransitionRIExchangeStatus result for rec. +func riStatusTransition(rec riStatusRecord) (*config.RIExchangeRecord, error) { + switch rec { + case riRecAlreadyProcessed: + return nil, nil + case riRecTransitionExpired: + return nil, fmt.Errorf("%w: %s", config.ErrRIExchangeExpired, riStatusID) + case riRecTransitionConflict: + return nil, fmt.Errorf("%w: expected status %q but current status is %q", config.ErrRIExchangeNotInExpectedStatus, "pending", "processing") + case riRecTransitionFails: + return nil, errors.New("connection reset") + default: + return &config.RIExchangeRecord{ID: riStatusID, Status: "processing"}, nil + } +} + // riStatusOpts tweaks the fixture for the less common cases. type riStatusOpts struct { fourEyes bool // global 4-eyes mode on; the admin session is the creator @@ -50,6 +80,10 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { if rec == riRecNoToken { record.ApprovalToken = "" } + if rec == riRecExpired { + past := time.Now().Add(-time.Hour) + record.ExpiresAt = &past + } if (o.fourEyes && !o.noCreator) || o.approveOwn { creator := "admin-uuid" if o.approveOwn { @@ -64,12 +98,9 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { } else { store.On("GetRIExchangeRecord", mock.Anything, riStatusID).Return(record, nil).Maybe() } - var transitioned *config.RIExchangeRecord - if rec != riRecAlreadyProcessed { - transitioned = &config.RIExchangeRecord{ID: riStatusID, Status: "processing"} - } + transitioned, transitionErr := riStatusTransition(rec) store.On("TransitionRIExchangeStatus", mock.Anything, riStatusID, "pending", mock.Anything, mock.Anything). - Return(transitioned, nil).Maybe() + Return(transitioned, transitionErr).Maybe() store.On("ReserveApprovedRIExchange", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("500.000000", nil).Maybe() store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{RequireDifferentApprover: o.fourEyes}, nil).Maybe() store.On("FailRIExchange", mock.Anything, riStatusID, mock.Anything).Return(nil).Maybe() @@ -193,6 +224,16 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, {name: "404 missing record with session", rec: riRecMissing, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, + // Issue #587: the same invalid/expired/wrong-status mapping as the + // purchase approve, cancel and revoke links (403/410/409). + {name: "410 token approve of an expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "403 wrong token on an expired exchange does not reveal the expiry", rec: riRecExpired, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, + {name: "410 expired exchange answers before the 4-eyes session check", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{fourEyes: true, noCreator: true}, want: 410, wantError: "exchange has expired"}, + {name: "410 token approve when the exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "409 token approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, + {name: "500 token approve transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, + {name: "410 session approve when the exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessCSRF, want: 410, wantError: "exchange has expired"}, + {name: "409 session approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessCSRF, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, }) } @@ -213,5 +254,11 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled"}, + // Issue #587: 403 invalid, 410 expired, 409 wrong status. + {name: "410 expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "403 wrong token on an expired exchange does not reveal the expiry", rec: riRecExpired, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, + {name: "410 exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, + {name: "409 loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled"}, + {name: "500 transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, }) } diff --git a/internal/config/errors.go b/internal/config/errors.go index 75982d6a4..cd60fa293 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -32,6 +32,18 @@ var ErrManagedPlanLiveExecutions = errors.New("managed plan still has active or // distinguish this legitimate race-loss from a hard DB error. var ErrExecutionNotInExpectedStatus = errors.New("execution not in expected status") +// ErrRIExchangeExpired is returned (wrapped) by TransitionRIExchangeStatus +// when the record exists but its expires_at has passed, so the approval or +// rejection window is closed. The HTTP layer answers it with 410, like an +// expired purchase approval token (issue #587). +var ErrRIExchangeExpired = errors.New("ri exchange has expired") + +// ErrRIExchangeNotInExpectedStatus is returned (wrapped) by +// TransitionRIExchangeStatus when the record exists and has not expired but +// its status is not the expected one: another writer, such as a concurrent +// approve or reject, transitioned it first. The HTTP layer answers it with 409. +var ErrRIExchangeNotInExpectedStatus = errors.New("ri exchange status transition failed") + // ErrApprovalTokenChanged is returned (wrapped) by // TransitionExecutionStatusTokenPinned when the execution is still in an // allowed status but its stored approval token no longer matches the one the diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 6483836dc..93ded0c86 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -468,6 +468,8 @@ type StoreInterface interface { GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]RIExchangeRecord, error) // TransitionRIExchangeStatus atomically transitions an RI exchange record status. // actor is the UUID of the user performing the transition (nil for system-initiated paths). + // A lost transition returns a wrapped ErrRIExchangeExpired when the record + // has expired, or ErrRIExchangeNotInExpectedStatus when its status moved on. TransitionRIExchangeStatus(ctx context.Context, id string, fromStatus string, toStatus string, actor *string) (*RIExchangeRecord, error) CompleteRIExchange(ctx context.Context, id string, exchangeID string) error // CompleteRIExchangeWithPayment marks an RI exchange as completed and diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index c90c71ab5..576a3583a 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3389,7 +3389,8 @@ func (s *PostgresStore) GetRIExchangeHistory(ctx context.Context, since time.Tim // TransitionRIExchangeStatus atomically transitions an RI exchange record status. // Uses a single UPDATE...WHERE...RETURNING for atomicity, then diagnoses failure -// only if zero rows are returned. +// only if zero rows are returned (ErrRIExchangeExpired or +// ErrRIExchangeNotInExpectedStatus, wrapped, for an existing record). // actor is the UUID of the user performing the transition (nil for system-initiated paths). func (s *PostgresStore) TransitionRIExchangeStatus(ctx context.Context, id, fromStatus, toStatus string, actor *string) (*RIExchangeRecord, error) { query := ` @@ -3432,9 +3433,9 @@ func (s *PostgresStore) diagnoseTransitionFailure(ctx context.Context, id, fromS return fmt.Errorf("failed to diagnose transition failure: %w", err) } if expired { - return fmt.Errorf("ri exchange has expired") + return fmt.Errorf("%w: %s", ErrRIExchangeExpired, id) } - return fmt.Errorf("ri exchange status transition failed: expected status %q but current status is %q", fromStatus, currentStatus) + return fmt.Errorf("%w: expected status %q but current status is %q", ErrRIExchangeNotInExpectedStatus, fromStatus, currentStatus) } // CompleteRIExchange marks an RI exchange as completed. diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 9f51b1b03..de01ff74b 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1466,6 +1466,8 @@ func TestPGXMock_TransitionRIExchangeStatus_WrongStatus(t *testing.T) { _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expected status") + assert.ErrorIs(t, err, ErrRIExchangeNotInExpectedStatus, "the API maps this to 409 (issue #587)") + assert.NotErrorIs(t, err, ErrRIExchangeExpired) } func TestPGXMock_TransitionRIExchangeStatus_RecordNotFound(t *testing.T) { @@ -1483,6 +1485,8 @@ func TestPGXMock_TransitionRIExchangeStatus_RecordNotFound(t *testing.T) { _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "not found") + assert.NotErrorIs(t, err, ErrRIExchangeExpired) + assert.NotErrorIs(t, err, ErrRIExchangeNotInExpectedStatus) } func TestPGXMock_TransitionRIExchangeStatus_Expired(t *testing.T) { @@ -1501,6 +1505,8 @@ func TestPGXMock_TransitionRIExchangeStatus_Expired(t *testing.T) { _, err := store.TransitionRIExchangeStatus(ctx, "ri-id", "pending", "processing", nil) require.Error(t, err) assert.Contains(t, err.Error(), "expired") + assert.ErrorIs(t, err, ErrRIExchangeExpired, "the API maps this to 410 (issue #587)") + assert.NotErrorIs(t, err, ErrRIExchangeNotInExpectedStatus) } func TestPGXMock_TransitionRIExchangeStatus_Success(t *testing.T) { diff --git a/internal/config/store_postgres_ri_exchange_transition_integration_test.go b/internal/config/store_postgres_ri_exchange_transition_integration_test.go new file mode 100644 index 000000000..2104613aa --- /dev/null +++ b/internal/config/store_postgres_ri_exchange_transition_integration_test.go @@ -0,0 +1,117 @@ +//go:build integration + +package config + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The RI exchange approve and reject links answer an expired exchange with +// 410 and an exchange another writer already moved with 409 (issue #587). +// Both codes come from the sentinel errors TransitionRIExchangeStatus wraps +// when its compare-and-set matches no row, so these tests pin them on a real +// PostgreSQL: the expiry predicate and the row lock that serializes +// concurrent transitions only exist in the database. + +func saveTransitionTestExchange(ctx context.Context, t *testing.T, store *PostgresStore, expiresAt time.Time) string { + t.Helper() + record := &RIExchangeRecord{ + AccountID: "123456789012", + ExchangeID: "transition-test-exchange", + Region: "us-east-1", + SourceRIIDs: []string{"ri-1"}, + SourceInstanceType: "r5.large", + SourceCount: 1, + TargetOfferingID: "off-1", + TargetInstanceType: "r5.xlarge", + TargetCount: 1, + Status: "pending", + Mode: "manual", + ExpiresAt: &expiresAt, + } + require.NoError(t, store.SaveRIExchangeRecord(ctx, record)) + return record.ID +} + +func TestPostgresStoreDB_TransitionRIExchangeStatus_ExpiredIsErrRIExchangeExpired(t *testing.T) { + conn := setupTestContainerDB(t) + store := NewPostgresStore(conn) + ctx := context.Background() + + id := saveTransitionTestExchange(ctx, t, store, time.Now().Add(-time.Hour)) + + for _, to := range []string{"processing", StatusCanceled} { + got, err := store.TransitionRIExchangeStatus(ctx, id, "pending", to, nil) + require.Error(t, err, "an expired exchange must not transition to %s", to) + assert.Nil(t, got) + assert.ErrorIs(t, err, ErrRIExchangeExpired) + assert.NotErrorIs(t, err, ErrRIExchangeNotInExpectedStatus) + } + + record, err := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, err) + assert.Equal(t, "pending", record.Status, "a refused transition must leave the row alone") +} + +// TestPostgresStoreDB_TransitionRIExchangeStatus_ConcurrentLosersGetNotInExpectedStatus +// races approvals (pending -> processing) against rejections (pending -> +// canceled) on one unexpired row. Exactly one wins; every loser must get +// ErrRIExchangeNotInExpectedStatus (409), never a bare error (500) or the +// expiry sentinel (410). +func TestPostgresStoreDB_TransitionRIExchangeStatus_ConcurrentLosersGetNotInExpectedStatus(t *testing.T) { + conn := setupTestContainerDB(t) + store := NewPostgresStore(conn) + ctx := context.Background() + + const callers = 8 + for round := 0; round < 5; round++ { + id := saveTransitionTestExchange(ctx, t, store, time.Now().Add(time.Hour)) + + start := make(chan struct{}) + type outcome struct { + to string + rec *RIExchangeRecord + err error + } + results := make(chan outcome, callers) + var wg sync.WaitGroup + for i := 0; i < callers; i++ { + to := "processing" + if i%2 == 1 { + to = StatusCanceled + } + wg.Add(1) + go func(to string) { + defer wg.Done() + <-start + rec, err := store.TransitionRIExchangeStatus(ctx, id, "pending", to, nil) + results <- outcome{to: to, rec: rec, err: err} + }(to) + } + close(start) + wg.Wait() + close(results) + + var winners []string + for r := range results { + if r.err == nil { + require.NotNil(t, r.rec) + winners = append(winners, r.to) + continue + } + assert.ErrorIs(t, r.err, ErrRIExchangeNotInExpectedStatus, "round %d: a lost race must map to 409", round) + assert.NotErrorIs(t, r.err, ErrRIExchangeExpired, "round %d", round) + } + require.Len(t, winners, 1, "round %d: exactly one transition must win", round) + + record, err := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, err) + assert.Equal(t, winners[0], record.Status, "round %d", round) + } +} From 8999bc2f8d8fc2caa0e55339d58bb401d4273e63 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 02:07:02 +0100 Subject: [PATCH 2/3] fix(api): answer a missing RI exchange like a wrong token on approve and reject The RI exchange approve and reject email links are open to callers with no session, since the token alone authorizes them. They answered a wrong token on an existing exchange with 403 but a missing exchange id with 404 (with the in-memory mocks) or 500 (on Postgres, whose GetRIExchangeRecord returned a bare error for no row), so anyone could probe which exchange ids exist. A 401 before the lookup, the purchase routes' fix (#435), does not fit: it would break the token-only email flow. Instead token mode now gives one answer to every caller who does not hold the token: - checkExchangeToken treats a missing record and a record without a token like a wrong one: 403 "invalid approval token" / "invalid rejection token". The #587 order is unchanged: the token match comes first, then 410 for an expired exchange, then the transition's 409/410. - PostgresStore.GetRIExchangeRecord wraps ErrNotFound for no row. getRIExchangeRecord maps it (or a nil record) to "missing", so the session approve path answers a missing exchange with the documented 404 instead of 500. Other store errors stay 500. - openapi.yaml: 404 is session mode only on approve and is gone from reject; the 403s say an unknown id answers like a wrong token. Closes #601 Co-Authored-By: Claude Opus 5.5 --- internal/api/coverage_extras_test.go | 8 +- internal/api/handler_ri_exchange.go | 59 ++++---- internal/api/handler_ri_exchange_test.go | 3 +- internal/api/openapi.yaml | 33 +++-- .../ri_exchange_approve_reject_status_test.go | 86 +++++++++-- ..._exchange_token_oracle_integration_test.go | 136 ++++++++++++++++++ internal/api/router_handlers_test.go | 11 +- internal/config/interfaces.go | 1 + internal/config/store_postgres.go | 6 +- .../config/store_postgres_pgxmock_test.go | 33 +++++ 10 files changed, 323 insertions(+), 53 deletions(-) create mode 100644 internal/api/ri_exchange_token_oracle_integration_test.go diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 3e0d9842f..d8b532c65 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -318,8 +318,12 @@ func TestHandler_rejectRIExchange_RecordNotFound(t *testing.T) { h := &Handler{config: mockStore} _, err := h.rejectRIExchange(ctx, "11111111-1111-1111-1111-111111111111", "tok") - assert.Error(t, err) - assert.Contains(t, err.Error(), "not found") + // A missing record answers like a wrong token (issue #601). + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %v", err) + assert.Equal(t, 403, ce.code) + assert.Equal(t, "invalid rejection token", ce.message) } func TestHandler_rejectRIExchange_WrongToken(t *testing.T) { diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 8f54d04ba..86d5df17f 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2431,9 +2431,9 @@ func (h *Handler) fetchAndAuthorizeRIExchange(ctx context.Context, session *Sess return nil, errNotFound } - record, err := h.config.GetRIExchangeRecord(ctx, id) + record, err := h.getRIExchangeRecord(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to look up exchange record: %w", err) + return nil, err } if record == nil { return nil, NewClientError(404, "exchange record not found") @@ -2598,6 +2598,22 @@ func (h *Handler) requireDifferentRIExchangeApprover(ctx context.Context, sessio return NewClientError(500, "4-eyes approval mode is enabled but no session could be resolved; sign in before approving") } +// getRIExchangeRecord loads an exchange record for the approve and reject +// routes. A missing record is (nil, nil) whether the store reports it as a +// nil record or as a wrapped config.ErrNotFound (PostgresStore), so callers +// answer it as a client error and not a 500 (issue #601). Any other store +// error stays a server error. +func (h *Handler) getRIExchangeRecord(ctx context.Context, id string) (*config.RIExchangeRecord, error) { + record, err := h.config.GetRIExchangeRecord(ctx, id) + if errors.Is(err, config.ErrNotFound) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to look up exchange record: %w", err) + } + return record, nil +} + // validateExchangeApproval validates ID, token, and record state for an exchange approval. func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string) (*config.RIExchangeRecord, error) { if err := validateUUID(id); err != nil { @@ -2607,14 +2623,10 @@ func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string return nil, NewClientError(400, "approval token is required") } - record, err := h.config.GetRIExchangeRecord(ctx, id) + record, err := h.getRIExchangeRecord(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to look up exchange record: %w", err) - } - if record == nil { - return nil, NewClientError(404, "exchange record not found") + return nil, err } - if err := checkExchangeToken(record, token, "approval"); err != nil { return nil, err } @@ -2628,20 +2640,23 @@ const ( ) // checkExchangeToken applies the emailed-token status mapping shared with the -// purchase approve, cancel and revoke links (issue #587): a record without a -// token, or a token that does not match, is 403, and a matching token on an -// exchange past its expires_at is 410. The match is checked first, so only -// the token holder learns that the exchange expired. kind is "approval" or -// "rejection" and only shapes the 403 text. +// purchase approve, cancel and revoke links (issue #587): a token that does +// not match is 403, and a matching token on an exchange past its expires_at +// is 410. The match is checked first, so only the token holder learns that +// the exchange expired. kind is "approval" or "rejection" and only shapes the +// 403 text. +// +// A missing record (nil) and a record without a token get the same 403 as a +// wrong token (issue #601): the token alone authorizes these routes, so they +// are open to callers with no session, and a distinct answer would tell such +// a caller which exchange ids exist. // // record.ApprovalToken is the SHA-256 hex digest stored at rest (issue #103); // config.ApprovalTokenMatches hashes the supplied token and compares digests -// in constant time, as everywhere else that validates one of these tokens. +// in constant time, as everywhere else that validates one of these tokens, +// and never matches an empty stored digest. func checkExchangeToken(record *config.RIExchangeRecord, token, kind string) error { - if record.ApprovalToken == "" { - return NewClientError(403, "this exchange record does not support "+kind) - } - if !config.ApprovalTokenMatches(record.ApprovalToken, token) { + if record == nil || !config.ApprovalTokenMatches(record.ApprovalToken, token) { return NewClientError(403, "invalid "+kind+" token") } if record.ExpiresAt != nil && time.Now().After(*record.ExpiresAt) { @@ -2812,14 +2827,10 @@ func (h *Handler) rejectRIExchange(ctx context.Context, id, token string) (any, return nil, NewClientError(400, "rejection token is required") } - record, err := h.config.GetRIExchangeRecord(ctx, id) + record, err := h.getRIExchangeRecord(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to look up exchange record: %w", err) - } - if record == nil { - return nil, NewClientError(404, "exchange record not found") + return nil, err } - if tokenErr := checkExchangeToken(record, token, "rejection"); tokenErr != nil { return nil, tokenErr } diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 9e3818c71..558ae2b43 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -893,7 +893,8 @@ func TestRejectRIExchange_EmptyStoredToken(t *testing.T) { ce, ok := IsClientError(err) assert.True(t, ok) assert.Equal(t, 403, ce.code) - assert.Contains(t, err.Error(), "does not support rejection") + // Same answer as a wrong token or a missing record (issue #601). + assert.Contains(t, err.Error(), "invalid rejection token") mockStore.AssertExpectations(t) } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 50c1d8fec..292db1e79 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1512,8 +1512,10 @@ paths: present; a CSRF rejection never falls through. The token is read from the query string only. This route never returns 401: a request with no token and no usable session - fails CSRF validation with 403. A missing record (404) and a wrong - token (403) are distinguishable for unauthenticated callers. + fails CSRF validation with 403. The token alone authorizes, so the + route is open to callers without a session; token mode answers an + unknown exchange id with the same 403 as a wrong token, so it does + not reveal which ids exist. security: - bearerAuth: [] - {} @@ -1542,15 +1544,22 @@ paths: description: > CSRF validation failed (including a request with neither a token nor a valid session), the session lacks approve permission, the - token is invalid, the record does not support approval, or, when a - session exists, 4-eyes mode rejects the approver (for example the - creator approving their own exchange). + token is invalid, or, when a session exists, 4-eyes mode rejects + the approver (for example the creator approving their own + exchange). In token mode an unknown exchange id, an exchange with + no approval token and a wrong token get the same 403 and body. content: application/json: schema: $ref: '#/components/schemas/Error' '404': - $ref: '#/components/responses/NotFound' + description: > + Session mode only, the exchange does not exist. Token mode answers + an unknown id with 403, like a wrong token. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' '409': description: > The exchange is no longer pending (already processed, or canceled @@ -1590,9 +1599,9 @@ paths: summary: Reject a pending RI exchange (token-based, no session required) description: > Authorized by the rejection token in the query string alone. A session - is ignored and no CSRF check runs. A missing token is 400; a missing - record (404) and a wrong token (403) are distinguishable for - unauthenticated callers. + is ignored and no CSRF check runs. A missing token is 400. An unknown + exchange id gets the same 403 and body as a wrong token, so the route + does not reveal which ids exist. security: [] parameters: - name: token @@ -1610,13 +1619,13 @@ paths: '400': $ref: '#/components/responses/BadRequest' '403': - description: The rejection token is invalid, or the record does not support rejection. + description: > + The rejection token is invalid. An unknown exchange id and an + exchange with no token get the same answer. content: application/json: schema: $ref: '#/components/schemas/Error' - '404': - $ref: '#/components/responses/NotFound' '409': description: > The exchange is no longer pending (already processed or canceled), diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index 06ccab267..b4c5a3480 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -29,6 +29,9 @@ const ( riRecPending riStatusRecord = iota riRecNoToken riRecMissing + // riRecMissingNotFound is missing as PostgresStore reports it: a wrapped + // config.ErrNotFound instead of a nil record (issue #601). + riRecMissingNotFound riRecAlreadyProcessed // riRecExpired is pending but past its expires_at. Its transition is // mocked to succeed, so a 410 proves the token check answered before @@ -72,7 +75,7 @@ type riStatusOpts struct { func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { var record *config.RIExchangeRecord - if rec != riRecMissing { + if rec != riRecMissing && rec != riRecMissingNotFound { record = &config.RIExchangeRecord{ ID: riStatusID, Status: "pending", SourceRIIDs: []string{"ri-1"}, PaymentDue: "0", ApprovalToken: config.HashApprovalToken(riStatusRawToken), @@ -93,9 +96,13 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { } } store := new(MockConfigStore) - if o.dbError { + switch { + case o.dbError: store.On("GetRIExchangeRecord", mock.Anything, riStatusID).Return(nil, errors.New("db down")).Maybe() - } else { + case rec == riRecMissingNotFound: + store.On("GetRIExchangeRecord", mock.Anything, riStatusID). + Return(nil, fmt.Errorf("ri exchange record %s: %w", riStatusID, config.ErrNotFound)).Maybe() + default: store.On("GetRIExchangeRecord", mock.Anything, riStatusID).Return(record, nil).Maybe() } transitioned, transitionErr := riStatusTransition(rec) @@ -209,7 +216,7 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "403 session without CSRF and no token", rec: riRecPending, sess: riSessNoCSRF, want: 403, wantError: "CSRF validation failed"}, {name: "403 session without CSRF does not fall through to a valid token", rec: riRecPending, sess: riSessNoCSRF, token: riStatusRawToken, want: 403, wantError: "CSRF validation failed"}, {name: "403 wrong token without session", rec: riRecPending, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, - {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "this exchange record does not support approval"}, + {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid approval token"}, {name: "400 invalid UUID", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, id: "not-a-uuid", want: 400, wantError: "invalid ID format: must be a valid UUID"}, {name: "403 session without approve right and no token", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{noApprove: true}, want: 403, wantError: "permission denied: requires approve-any or approve-own on purchases"}, {name: "403 4-eyes creator cannot approve own exchange", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{fourEyes: true}, want: 403, wantError: "approval declined: 4-eyes mode requires a different approver than the requester"}, @@ -220,9 +227,13 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "403 body token is not read", rec: riRecPending, sess: riSessNone, body: `{"token":"` + riStatusRawToken + `"}`, want: 403, wantError: "CSRF validation failed"}, {name: "429 rate limited", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{rateLimited: true}, want: 429, wantError: "too many requests, please try again later"}, {name: "500 record lookup fails", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{dbError: true}, want: 500}, - {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, - {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, + // Issue #601: in token mode a missing record answers like a wrong + // token, so the route does not reveal which ids exist. + {name: "403 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid approval token"}, + {name: "403 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, + {name: "403 missing record reported as ErrNotFound with token", rec: riRecMissingNotFound, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid approval token"}, {name: "404 missing record with session", rec: riRecMissing, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, + {name: "404 missing record reported as ErrNotFound with session", rec: riRecMissingNotFound, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, // Issue #587: the same invalid/expired/wrong-status mapping as the // purchase approve, cancel and revoke links (403/410/409). @@ -250,9 +261,11 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "429 rate limited", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{rateLimited: true}, want: 429, wantError: "too many requests, please try again later"}, {name: "500 record lookup fails", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{dbError: true}, want: 500}, {name: "403 wrong token", rec: riRecPending, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, - {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "this exchange record does not support rejection"}, - {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, - {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, + {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid rejection token"}, + // Issue #601: a missing record answers like a wrong token. + {name: "403 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid rejection token"}, + {name: "403 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, + {name: "403 missing record reported as ErrNotFound with token", rec: riRecMissingNotFound, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid rejection token"}, {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled"}, // Issue #587: 403 invalid, 410 expired, 409 wrong status. {name: "410 expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, @@ -262,3 +275,58 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "500 transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, }) } + +// TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken is the +// issue #601 regression: a caller that does not hold the emailed token gets +// the same status, headers and body for a missing exchange id as for an +// existing one, so approve and reject cannot be used to learn which ids +// exist. A 401 before the lookup (the purchase routes' fix, #435) does not +// fit here: the token alone authorizes these routes, with no session. +func TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken(t *testing.T) { + type probe struct { + name string + rec riStatusRecord + token string + } + // The baseline is a missing record as the mocks report it; every other + // probe must answer exactly like it. + baseline := probe{"missing record", riRecMissing, "wrong"} + probes := []probe{ + {"missing record reported as ErrNotFound", riRecMissingNotFound, "wrong"}, + {"missing record reported as ErrNotFound with the real token's value", riRecMissingNotFound, riStatusRawToken}, + {"pending record with a wrong token", riRecPending, "wrong"}, + {"record without a token", riRecNoToken, "wrong"}, + {"expired record with a wrong token", riRecExpired, "wrong"}, + } + callers := []struct { + name string + sess riStatusSession + opts riStatusOpts + }{ + {"no session", riSessNone, riStatusOpts{}}, + {"invalid session", riSessInvalid, riStatusOpts{}}, + {"session without approve permission", riSessCSRF, riStatusOpts{noApprove: true}}, + } + for _, action := range []string{"approve", "reject"} { + for _, caller := range callers { + do := func(p probe) *events.LambdaFunctionURLResponse { + resp, err := riStatusHandler(p.rec, caller.opts).HandleRequest(context.Background(), riStatusRequest(action, caller.sess, p.token)) + require.NoError(t, err) + return resp + } + want := do(baseline) + t.Run(action+"/"+caller.name+"/answers 403 invalid token", func(t *testing.T) { + assert.Equal(t, 403, want.StatusCode, want.Body) + assert.Contains(t, want.Body, "invalid "+map[string]string{"approve": "approval", "reject": "rejection"}[action]+" token") + }) + for _, p := range probes { + t.Run(action+"/"+caller.name+"/"+p.name, func(t *testing.T) { + got := do(p) + assert.Equal(t, want.StatusCode, got.StatusCode, got.Body) + assert.Equal(t, want.Body, got.Body) + assert.Equal(t, want.Headers, got.Headers) + }) + } + } + } +} diff --git a/internal/api/ri_exchange_token_oracle_integration_test.go b/internal/api/ri_exchange_token_oracle_integration_test.go new file mode 100644 index 000000000..4bc95d63e --- /dev/null +++ b/internal/api/ri_exchange_token_oracle_integration_test.go @@ -0,0 +1,136 @@ +//go:build integration + +package api + +// Real-Postgres check for issue #601. PostgresStore reports a missing RI +// exchange as an error, not a nil record, so before the fix the approve and +// reject email links answered a missing id with 500 and an existing one with +// a wrong token with 403: an oracle for which exchange ids exist, open to +// callers with no session. These tests drive HandleRequest against the real +// store and compare the two answers. + +import ( + "context" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +const oracleExchangeRawToken = "oracle-exchange-raw-token" + +// oracleMissingID is a well-formed UUID that no seeded exchange uses. +const oracleMissingID = "550e8400-e29b-41d4-a716-446655440601" + +func setupOracleExchangeStore(ctx context.Context, t *testing.T) *config.PostgresStore { + t.Helper() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { _ = container.Cleanup(context.Background()) }) + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + return config.NewPostgresStore(container.DB) +} + +// seedOracleExchange stores a pending exchange; rawToken "" stores none. +func seedOracleExchange(ctx context.Context, t *testing.T, store *config.PostgresStore, rawToken string, expiresAt time.Time) string { + t.Helper() + rec := &config.RIExchangeRecord{ + AccountID: "123456789012", + Region: "us-east-1", + SourceRIIDs: []string{"ri-601"}, + SourceInstanceType: "m5.large", + SourceCount: 1, + TargetOfferingID: "offering-601", + TargetInstanceType: "c5.large", + TargetCount: 1, + PaymentDue: "10.00", + Status: "pending", + ApprovalToken: rawToken, + Mode: "manual", + ExpiresAt: &expiresAt, + } + require.NoError(t, store.SaveRIExchangeRecord(ctx, rec)) + return rec.ID +} + +func oracleRequest(action, id, token string, headers map[string]string) *events.LambdaFunctionURLRequest { + if headers == nil { + headers = map[string]string{} + } + return &events.LambdaFunctionURLRequest{ + Headers: headers, + QueryStringParameters: map[string]string{"token": token}, + RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "POST", Path: "/api/ri-exchange/" + action + "/" + id}}, + } +} + +func TestRIExchangeTokenRoutes_Integration_MissingAnswersLikeWrongToken(t *testing.T) { + ctx := context.Background() + store := setupOracleExchangeStore(ctx, t) + h := &Handler{config: store} + + _, lookupErr := store.GetRIExchangeRecord(ctx, oracleMissingID) + require.ErrorIs(t, lookupErr, config.ErrNotFound, "fixture: the id must be missing, reported as PostgresStore does") + + pending := seedOracleExchange(ctx, t, store, oracleExchangeRawToken, time.Now().Add(24*time.Hour)) + noToken := seedOracleExchange(ctx, t, store, "", time.Now().Add(24*time.Hour)) + expired := seedOracleExchange(ctx, t, store, oracleExchangeRawToken+"-expired", time.Now().Add(-time.Hour)) // tokens are unique + + for _, action := range []string{"approve", "reject"} { + missing, missErr := h.HandleRequest(ctx, oracleRequest(action, oracleMissingID, "wrong", nil)) + require.NoError(t, missErr) + t.Run(action+"/missing id is a 403, not a 500", func(t *testing.T) { + assert.Equal(t, 403, missing.StatusCode, missing.Body) + }) + for name, id := range map[string]string{"pending": pending, "no token": noToken, "expired": expired} { + t.Run(action+"/wrong token on "+name+" answers like a missing id", func(t *testing.T) { + got, err := h.HandleRequest(ctx, oracleRequest(action, id, "wrong", nil)) + require.NoError(t, err) + assert.Equal(t, missing.StatusCode, got.StatusCode, got.Body) + assert.Equal(t, missing.Body, got.Body) + assert.Equal(t, missing.Headers, got.Headers) + }) + } + } + + // The refused requests must not have touched the rows. + for _, id := range []string{pending, noToken, expired} { + rec, getErr := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, getErr) + assert.Equal(t, "pending", rec.Status) + } + + // Control: the token holder still rejects, so the 403s above come from + // the token check and not from a fixture the routes cannot act on. + resp, err := h.HandleRequest(ctx, oracleRequest("reject", pending, oracleExchangeRawToken, nil)) + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode, resp.Body) +} + +// TestApproveRIExchange_Integration_SessionMissingIs404 covers the +// session-authed approve: a missing exchange is the documented 404, not the +// 500 the bare store error used to produce. +func TestApproveRIExchange_Integration_SessionMissingIs404(t *testing.T) { + ctx := context.Background() + store := setupOracleExchangeStore(ctx, t) + + a := new(MockAuthService) + a.On("ValidateSession", mock.Anything, "admin-bearer").Return(&Session{UserID: "admin-uuid", Email: "admin@example.com"}, nil).Maybe() + a.grantAdminPurchaser() + a.On("ValidateCSRFToken", mock.Anything, "admin-bearer", "csrf-ok").Return(nil).Maybe() + h := &Handler{config: store, auth: a} + + resp, err := h.HandleRequest(ctx, oracleRequest("approve", oracleMissingID, "", + map[string]string{"authorization": "Bearer admin-bearer", "x-csrf-token": "csrf-ok"})) + require.NoError(t, err) + assert.Equal(t, 404, resp.StatusCode, resp.Body) + assert.Contains(t, resp.Body, "exchange record not found") +} diff --git a/internal/api/router_handlers_test.go b/internal/api/router_handlers_test.go index 243ea4539..67f1b1a8e 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -632,8 +632,12 @@ func TestHandler_validateExchangeApproval_RecordNotFound(t *testing.T) { h := &Handler{config: mockStore} _, err := h.validateExchangeApproval(ctx, "11111111-1111-1111-1111-111111111111", "some-token") - assert.Error(t, err) - assert.Contains(t, err.Error(), "not found") + // A missing record answers like a wrong token (issue #601). + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %v", err) + assert.Equal(t, 403, ce.code) + assert.Equal(t, "invalid approval token", ce.message) } func TestHandler_validateExchangeApproval_NoApprovalToken(t *testing.T) { @@ -645,7 +649,8 @@ func TestHandler_validateExchangeApproval_NoApprovalToken(t *testing.T) { h := &Handler{config: mockStore} _, err := h.validateExchangeApproval(ctx, "11111111-1111-1111-1111-111111111111", "some-token") assert.Error(t, err) - assert.Contains(t, err.Error(), "does not support approval") + // Same answer as a wrong token or a missing record (issue #601). + assert.Contains(t, err.Error(), "invalid approval token") } func TestHandler_validateExchangeApproval_WrongToken(t *testing.T) { diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 93ded0c86..b4ca221b2 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -463,6 +463,7 @@ type StoreInterface interface { // processing (approved) exchange under the shared daily-cap lock, excluding // the row's own amount, and stores the ceiling as its payment_due. ReserveApprovedRIExchange(ctx context.Context, id, dailyCapUSD, perExchangeCapUSD string) (string, error) + // GetRIExchangeRecord returns a wrapped ErrNotFound when no record has id. GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) GetRIExchangeRecordByToken(ctx context.Context, token string) (*RIExchangeRecord, error) GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]RIExchangeRecord, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 576a3583a..8d6deb150 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3317,7 +3317,9 @@ func riExchangeUSD(micros *big.Int) string { return fmt.Sprintf("%s.%06d", whole, fractional) } -// GetRIExchangeRecord retrieves an RI exchange record by ID. +// GetRIExchangeRecord retrieves an RI exchange record by ID. A missing record +// is a wrapped ErrNotFound, so callers can tell it from a failed query and +// answer it as a client error rather than a 500 (issue #601). func (s *PostgresStore) GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) { query := ` SELECT id, account_id, exchange_id, region, source_ri_ids, @@ -3336,7 +3338,7 @@ func (s *PostgresStore) GetRIExchangeRecord(ctx context.Context, id string) (*RI } if len(records) == 0 { - return nil, fmt.Errorf("ri exchange record not found: %s", id) + return nil, fmt.Errorf("ri exchange record %s: %w", id, ErrNotFound) } return &records[0], nil diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index de01ff74b..0678144cd 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1418,6 +1418,39 @@ func TestPGXMock_GetRIExchangeRecord_WithTimestamps(t *testing.T) { assert.Equal(t, "some error", rec.Error) } +// TestPGXMock_GetRIExchangeRecord_NotFound pins the missing-record error the +// RI exchange approve and reject routes rely on: a wrapped ErrNotFound, which +// they answer as a 4xx, not a bare error they would answer with 500 (#601). +func TestPGXMock_GetRIExchangeRecord_NotFound(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery("SELECT").WithArgs("ri-id").WillReturnRows(pgxmock.NewRows(riExchangeCols)) + + rec, err := store.GetRIExchangeRecord(ctx, "ri-id") + require.Error(t, err) + assert.Nil(t, rec) + assert.ErrorIs(t, err, ErrNotFound) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetRIExchangeRecord_QueryErrorIsNotErrNotFound keeps a failed +// query a server error: only an empty result is ErrNotFound. +func TestPGXMock_GetRIExchangeRecord_QueryErrorIsNotErrNotFound(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery("SELECT").WithArgs("ri-id").WillReturnError(errors.New("connection reset")) + + rec, err := store.GetRIExchangeRecord(ctx, "ri-id") + require.Error(t, err) + assert.Nil(t, rec) + assert.NotErrorIs(t, err, ErrNotFound) + assert.NoError(t, mock.ExpectationsWereMet()) +} + func TestPGXMock_GetRIExchangeRecordByToken_Success(t *testing.T) { mock := newMock(t) store := storeWith(mock) From ff88e481705a794aeef30100ff47e78dd64597ae Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 04:31:39 +0000 Subject: [PATCH 3/3] fix(api): log lost RI exchange transitions and drop "expired" from the 409 text Review follow-ups on #1172. riExchangeTransitionError turns the store's error, which names the exchange and its current status, into a bare ClientError, and handleRequestError never logs ClientErrors, so a lost approve/reject race or a mid-flight expiry left no server log line. Log the store error at info level in the 409 and 410 branches. Expiry is answered with 410 now, so the 409 conflict messages no longer list "expired" as a cause. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 10 ++++++++-- internal/api/ri_exchange_approve_reject_status_test.go | 10 +++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index b034f6fb6..69ff4161e 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2737,8 +2737,8 @@ func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string } const ( - riExchangeApproveConflictMsg = "exchange already processed, expired, or was canceled by a newer analysis run" - riExchangeRejectConflictMsg = "exchange already processed, expired, or was canceled" + riExchangeApproveConflictMsg = "exchange already processed or was canceled by a newer analysis run" + riExchangeRejectConflictMsg = "exchange already processed or was canceled" riExchangeExpiredMsg = "exchange has expired" ) @@ -2771,11 +2771,17 @@ func checkExchangeToken(record *config.RIExchangeRecord, token, kind string) err // The expiry pre-check in checkExchangeToken cannot close the window alone: // the record can expire, or a concurrent approve or reject can win, between // that check and the transition. Anything else stays a server error. +// +// handleRequestError does not log ClientErrors, so the 409 and 410 branches +// log the store's error themselves: it names the exchange and its current +// status, which the response body does not. func riExchangeTransitionError(err error, conflictMsg string) error { switch { case errors.Is(err, config.ErrRIExchangeExpired): + logging.Infof("ri-exchange transition lost to expiry: %v", err) return NewClientError(410, riExchangeExpiredMsg) case errors.Is(err, config.ErrRIExchangeNotInExpectedStatus): + logging.Infof("ri-exchange transition lost to a concurrent writer: %v", err) return NewClientError(409, conflictMsg) default: return fmt.Errorf("failed to transition exchange status: %w", err) diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index 06ccab267..380b4d1ea 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -223,17 +223,17 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, {name: "404 missing record with session", rec: riRecMissing, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, - {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, + {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, // Issue #587: the same invalid/expired/wrong-status mapping as the // purchase approve, cancel and revoke links (403/410/409). {name: "410 token approve of an expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, {name: "403 wrong token on an expired exchange does not reveal the expiry", rec: riRecExpired, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, {name: "410 expired exchange answers before the 4-eyes session check", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{fourEyes: true, noCreator: true}, want: 410, wantError: "exchange has expired"}, {name: "410 token approve when the exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, - {name: "409 token approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, + {name: "409 token approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, {name: "500 token approve transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, {name: "410 session approve when the exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessCSRF, want: 410, wantError: "exchange has expired"}, - {name: "409 session approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessCSRF, want: 409, wantError: "exchange already processed, expired, or was canceled by a newer analysis run"}, + {name: "409 session approve loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessCSRF, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, }) } @@ -253,12 +253,12 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "this exchange record does not support rejection"}, {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, - {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled"}, + {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled"}, // Issue #587: 403 invalid, 410 expired, 409 wrong status. {name: "410 expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, {name: "403 wrong token on an expired exchange does not reveal the expiry", rec: riRecExpired, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, {name: "410 exchange expires before the transition", rec: riRecTransitionExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, - {name: "409 loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed, expired, or was canceled"}, + {name: "409 loses the transition to a concurrent writer", rec: riRecTransitionConflict, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled"}, {name: "500 transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, }) }