fix(api): answer expired and lost-race token actions alike on revoke and RI exchange

internal/api/handler_purchases.go

@@ -1965,29 +1965,29 @@ func checkRevokableStatus(execution *config.PurchaseExecution) error {
 	}
 }
 
-// validateRevokeToken checks that the execution carries a non-empty
-// ApprovalToken that matches the supplied token (constant-time comparison),
-// then that the token has not expired and that the revocation window has not
-// closed (config.RevocationWindow after CompletedAt). Pre-migration rows
-// without an ApprovalTokenExpiresAt pass the expiry check (legacy backward
-// compat).
+// 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. 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).
 //
-// The token is compared first, as on the approve path (ValidateApprovalToken):
-// a wrong token must get the same "invalid" answer whatever the row's expiry or
-// window, or the expiry/window message tells a token guesser that the link
-// exists and has lapsed (issue #1190).
+// The token is compared first, as on the approve path: it shares
+// purchase.ValidateApprovalToken, which matches the token (in constant time,
+// against the SHA-256 digest stored at rest, issue #103) before it reads the
+// expiry. A wrong token must get the same "invalid" answer whatever the row's
+// expiry or window, or the expiry/window message tells a token guesser that
+// the link exists and has lapsed (issue #1190).
 // Extracted from revokeViaEmailToken to keep that function under the cyclomatic limit.
 func validateRevokeToken(execution *config.PurchaseExecution, token string) error {
-	// 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, and is false
-	// for an empty stored or supplied token.
-	if !config.ApprovalTokenMatches(execution.ApprovalToken, token) {
+	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

internal/api/handler_purchases_test.go

@@ -5937,8 +5937,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()
@@ -5985,7 +5986,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")
 }
 

internal/api/handler_purchases_token_errors_test.go

@@ -18,7 +18,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"
@@ -86,6 +87,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) {
@@ -185,7 +193,7 @@ func TestHandleRequest_RevokeViaEmail_WrongTokenOnLapsedRowIsInvalid(t *testing.
 	}{
 		{"wrong token on an expired link", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, "wrong", 403, "invalid revocation token"},
 		{"wrong token past the revocation window", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future, CompletedAt: &longAgo}, "wrong", 403, "invalid revocation token"},
-		{"right token on an expired link", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 409, "expired"},
+		{"right token on an expired link", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410, "expired"},
 		{"right token past the revocation window", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future, CompletedAt: &longAgo}, tokenErrRawToken, 403, "revocation window has closed"},
 	}
 	for _, tc := range cases {

internal/api/handler_ri_exchange.go

@@ -2449,10 +2449,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)
@@ -2512,10 +2512,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)
@@ -2730,19 +2730,62 @@ 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 or was canceled by a newer analysis run"
+	riExchangeRejectConflictMsg  = "exchange already processed 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.
+//
+// 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)
+	}
 }
 
 // failExchange marks an exchange as failed, logging if the DB write also fails.
@@ -2898,23 +2941,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

internal/api/openapi.yaml

@@ -1234,7 +1234,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:
@@ -2515,8 +2524,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:
@@ -2569,7 +2589,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: