sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first

internal/analytics/collector_test.go

@@ -355,6 +355,10 @@ func (m *mockConfigStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx,
 	return false, "", nil
 }
 
+func (m *mockConfigStore) CancelExecutionAtomicTokenPinned(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string, rawToken string) (bool, string, error) {
+	return false, "", nil
+}
+
 func (m *mockConfigStore) CancelScheduledExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (bool, string, error) {
 	return false, "", nil
 }

internal/api/handler_purchases.go

@@ -956,8 +956,10 @@ func concealExecutionDenial(err error) error {
 
 // tokenActionError maps the email-link approve/cancel failures that are the
 // caller's fault to a 4xx: a wrong token is 403 (as on the RI exchange
-// approve path), an expired token is 410, and an execution
-// whose status no longer allows the action is 409. Anything else stays a server error.
+// approve path), as is a token rotated or expired between the snapshot check
+// and the status CAS (config.ErrApprovalTokenChanged, issue #1190), an expired
+// token is 410, and an execution whose status no longer allows the action is
+// 409. Anything else stays a server error.
 func tokenActionError(err error) error {
 	switch {
 	case errors.Is(err, config.ErrAuditLoss):
@@ -966,7 +968,7 @@ func tokenActionError(err error) error {
 		return claimedRowRefusalError(nil, err)
 	case errors.Is(err, purchase.ErrStaleAzurePricing):
 		return NewClientError(409, err.Error())
-	case errors.Is(err, purchase.ErrInvalidApprovalToken):
+	case errors.Is(err, purchase.ErrInvalidApprovalToken), errors.Is(err, config.ErrApprovalTokenChanged):
 		return NewClientError(403, "invalid approval token")
 	case errors.Is(err, purchase.ErrApprovalTokenExpired):
 		return NewClientError(410, "approval token has expired")
@@ -1961,13 +1963,23 @@ 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).
+// 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).
+//
+// 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).
 // Extracted from revokeViaEmailToken to keep that function under the cyclomatic limit.
 func validateRevokeToken(execution *config.PurchaseExecution, token string) error {
-	if execution.ApprovalToken == "" {
+	// 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) {
 		return NewClientError(403, "invalid revocation token")
 	}
 	if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) {
@@ -1977,16 +1989,7 @@ func validateRevokeToken(execution *config.PurchaseExecution, token string) erro
 	// 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

internal/api/handler_purchases_cancel_constraints_test.go

@@ -200,6 +200,7 @@ func cancelRouteHandler(exec *config.PurchaseExecution, perms ...auth.Permission
 	store.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil)
 	store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe()
 	store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything).Return(true, "canceled", nil).Maybe()
+	store.On("CancelExecutionAtomicTokenPinned", mock.Anything, mock.Anything, cancelExecID, mock.Anything, mock.Anything).Return(true, "canceled", nil).Maybe()
 	store.On("DeleteSuppressionsByExecutionTx", mock.Anything, mock.Anything, cancelExecID).Return(nil).Maybe()
 	store.On("TransitionExecutionStatus", mock.Anything, cancelExecID, mock.Anything, mock.Anything, mock.Anything).
 		Return(&config.PurchaseExecution{ExecutionID: cancelExecID, Status: "canceled"}, nil).Maybe()
@@ -226,6 +227,7 @@ func TestCancelPurchase_ConstrainedCancelAnyIsDeniedWithAndWithoutToken(t *testi
 			_, err := h.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, token)
 			requireRevokeForbidden(t, err, constraintDenied)
 			store.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+			store.AssertNotCalled(t, "CancelExecutionAtomicTokenPinned", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
 		})
 	}
 }
@@ -236,7 +238,7 @@ func TestCancelPurchase_ContactApproverWithoutCancelVerbStillUsesToken(t *testin
 	res, err := h.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, tokenErrRawToken)
 	require.NoError(t, err)
 	assert.Equal(t, "canceled", res.(map[string]string)["status"])
-	store.AssertCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, cancelExecID, mock.Anything)
+	store.AssertCalled(t, "CancelExecutionAtomicTokenPinned", mock.Anything, mock.Anything, cancelExecID, mock.Anything, tokenErrRawToken)
 }
 
 func TestCancelPurchase_InScopeCancelAnyStillCancels(t *testing.T) {

internal/api/handler_purchases_test.go

@@ -5919,8 +5919,10 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) {
 	past := time.Now().Add(-1 * time.Hour)
 
 	exec := &config.PurchaseExecution{
-		ExecutionID:            execID,
-		ApprovalToken:          token,
+		ExecutionID: execID,
+		// Stored at rest as the hash (issue #103), so the token really matches
+		// and the expiry is what rejects it (the hash is compared first, issue #1190).
+		ApprovalToken:          config.HashApprovalToken(token),
 		ApprovalTokenExpiresAt: &past,
 		Status:                 "completed",
 		Recommendations: []config.RecommendationRecord{

internal/api/handler_purchases_token_errors_test.go

@@ -3,6 +3,7 @@ package api
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 	"time"
 
@@ -105,7 +106,7 @@ func TestHandleRequest_ApproveViaToken_NonPendingExecutionIs409(t *testing.T) {
 	exec := &config.PurchaseExecution{Status: "completed", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future}
 	h := tokenErrHandler(t, exec, nil)
 	store := h.config.(*MockConfigStore)
-	store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, mock.Anything, "running", mock.Anything).
+	store.On("TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, mock.Anything, "running", mock.Anything, tokenErrRawToken).
 		Return(nil, config.ErrExecutionNotInExpectedStatus)
 
 	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken))
@@ -125,10 +126,80 @@ func TestHandleRequest_CancelViaToken_LostCASIs409(t *testing.T) {
 	exec := &config.PurchaseExecution{Status: "pending", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future}
 	h := tokenErrHandler(t, exec, nil)
 	store := h.config.(*MockConfigStore)
-	store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, tokenErrExecID, mock.Anything).
+	store.On("CancelExecutionAtomicTokenPinned", mock.Anything, mock.Anything, tokenErrExecID, mock.Anything, tokenErrRawToken).
 		Return(false, "approved", nil)
 
 	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("cancel", tokenErrRawToken))
 	require.NoError(t, err)
 	assert.Equal(t, 409, resp.StatusCode, resp.Body)
 }
+
+// Issue #1190: the email-link approve and cancel pin the approval token inside
+// the status CAS. A token rotated (re-notification) between the snapshot check
+// and the CAS loses the pin with config.ErrApprovalTokenChanged, which is the
+// caller's fault (their link is no longer the current one), so it answers 403
+// "invalid approval token" like a wrong token, not a 500.
+func TestHandleRequest_TokenActions_TokenRotatedBeforeCASIs403(t *testing.T) {
+	future := time.Now().Add(time.Hour)
+	rotated := fmt.Errorf("approve: %w: execution %s", config.ErrApprovalTokenChanged, tokenErrExecID)
+	for _, action := range []string{"approve", "cancel"} {
+		t.Run(action, func(t *testing.T) {
+			exec := &config.PurchaseExecution{Status: "notified", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future}
+			h := tokenErrHandler(t, exec, nil)
+			store := h.config.(*MockConfigStore)
+			store.On("TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "running", (*string)(nil), tokenErrRawToken).
+				Return(nil, rotated).Maybe()
+			store.On("CancelExecutionAtomicTokenPinned", mock.Anything, mock.Anything, tokenErrExecID, mock.Anything, tokenErrRawToken).
+				Return(false, "notified", rotated).Maybe()
+
+			resp, err := h.HandleRequest(context.Background(), tokenErrRequest(action, tokenErrRawToken))
+			require.NoError(t, err)
+			assert.Equal(t, 403, resp.StatusCode, resp.Body)
+			assert.Contains(t, resp.Body, "invalid approval token")
+			if action == "approve" {
+				store.AssertCalled(t, "TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "running", (*string)(nil), tokenErrRawToken)
+			} else {
+				store.AssertCalled(t, "CancelExecutionAtomicTokenPinned", mock.Anything, mock.Anything, tokenErrExecID, mock.Anything, tokenErrRawToken)
+			}
+			store.AssertNotCalled(t, "TransitionExecutionStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+			store.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+		})
+	}
+}
+
+// Issue #1190: the revoke endpoint compares the token before the link's expiry
+// and the revocation window, as approve does, so a wrong token gets the same
+// 403 "invalid revocation token" whether the row's link is live, expired or
+// past its window; only the right token learns that the link has lapsed.
+func TestHandleRequest_RevokeViaEmail_WrongTokenOnLapsedRowIsInvalid(t *testing.T) {
+	past := time.Now().Add(-time.Hour)
+	future := time.Now().Add(time.Hour)
+	longAgo := time.Now().Add(-2 * config.RevocationWindow)
+	hash := config.HashApprovalToken(tokenErrRawToken)
+	cases := []struct {
+		name      string
+		exec      config.PurchaseExecution
+		token     string
+		want      int
+		wantInMsg string
+	}{
+		{"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 past the revocation window", config.PurchaseExecution{Status: "completed", ApprovalToken: hash, ApprovalTokenExpiresAt: &future, CompletedAt: &longAgo}, tokenErrRawToken, 403, "revocation window has closed"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			exec := tc.exec
+			h := tokenErrHandler(t, &exec, nil)
+			resp, err := h.HandleRequest(context.Background(), tokenErrRequest("revoke", tc.token))
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, resp.StatusCode, resp.Body)
+			assert.Contains(t, resp.Body, tc.wantInMsg)
+			if tc.token != tokenErrRawToken {
+				assert.NotContains(t, resp.Body, "expired")
+				assert.NotContains(t, resp.Body, "window")
+			}
+		})
+	}
+}

internal/config/interfaces.go

@@ -295,6 +295,14 @@ type StoreInterface interface {
 	// inside a WithTx block so the suppression cleanup and the status flip
 	// commit atomically.
 	CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (canceled bool, currentStatus string, err error)
+	// CancelExecutionAtomicTokenPinned is CancelExecutionAtomic that also
+	// requires the row's stored approval token hash to match rawToken and to
+	// be unexpired, in the same statement as the status change, so a token
+	// rotated or expired after the email-link cancel validated it cannot
+	// cancel the row (issue #1190). A lost token pin on a row still
+	// pending/notified returns an error wrapping ErrApprovalTokenChanged; a
+	// lost status CAS returns (false, currentStatus, nil) as usual.
+	CancelExecutionAtomicTokenPinned(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string, rawToken string) (canceled bool, currentStatus string, err error)
 	// CancelScheduledExecutionAtomic atomically flips status from 'scheduled' to
 	// 'canceled' (canonical US spelling), setting canceled_by. Used by the
 	// Gmail-style pre-fire delay revoke path (issue #291 wave-2) to cancel a

internal/config/store_postgres.go

@@ -1414,6 +1414,30 @@ func (s *PostgresStore) SetCancelledBy(ctx context.Context, executionID, cancell
 // status guard is inside the UPDATE rather than checked optimistically
 // before entering the tx.
 func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string) (canceled bool, currentStatus string, err error) {
+	return s.cancelExecutionAtomic(ctx, tx, executionID, cancelledBy, nil)
+}
+
+// CancelExecutionAtomicTokenPinned is CancelExecutionAtomic that also requires
+// approval_token_hash to equal the hash of rawToken and the token to be
+// unexpired, checked in the same UPDATE as the status change. The email-link
+// cancel validated the token against a snapshot; a re-notification that
+// rotates the token between that read and this write would otherwise still
+// let the old, revoked token cancel the row (issue #1190, the cancel side of
+// #590). A lost token pin on a row still pending/notified returns an error
+// wrapping ErrApprovalTokenChanged; a lost status CAS returns (false,
+// currentStatus, nil) as CancelExecutionAtomic does. An empty rawToken never
+// matches.
+func (s *PostgresStore) CancelExecutionAtomicTokenPinned(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy *string, rawToken string) (canceled bool, currentStatus string, err error) {
+	if rawToken == "" {
+		return false, "", fmt.Errorf("%w: execution %s: no approval token supplied", ErrApprovalTokenChanged, executionID)
+	}
+	tokenHash := HashApprovalToken(rawToken)
+	return s.cancelExecutionAtomic(ctx, tx, executionID, cancelledBy, &tokenHash)
+}
+
+// cancelExecutionAtomic is the shared body of CancelExecutionAtomic and
+// CancelExecutionAtomicTokenPinned; a nil tokenHash skips the token pin.
+func (s *PostgresStore) cancelExecutionAtomic(ctx context.Context, tx pgx.Tx, executionID string, cancelledBy, tokenHash *string) (canceled bool, currentStatus string, err error) {
 	// StatusCanceled is the canonical US-spelling value; see types.go.
 	q := `
 		UPDATE purchase_executions
@@ -1422,9 +1446,11 @@ func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, ex
 		       updated_at   = NOW()
 		 WHERE execution_id = $1
 		   AND status IN ('pending', 'notified')
+		   AND ($4::text IS NULL OR (approval_token_hash = $4
+		        AND (approval_token_expires_at IS NULL OR approval_token_expires_at > NOW())))
 		RETURNING status
 	`
-	rows, err := tx.Query(ctx, q, executionID, cancelledBy, StatusCanceled)
+	rows, err := tx.Query(ctx, q, executionID, cancelledBy, StatusCanceled, tokenHash)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to cancel execution: %w", err)
 	}
@@ -1444,14 +1470,27 @@ func (s *PostgresStore) CancelExecutionAtomic(ctx context.Context, tx pgx.Tx, ex
 		return false, "", fmt.Errorf("failed to iterate cancel result: %w", rowsErr)
 	}
 
-	// Zero rows affected: execution either does not exist or has already
-	// transitioned out of pending/notified. Surface the current status so
-	// callers can return a meaningful 409 body.
+	// Zero rows affected: execution either does not exist, has already
+	// transitioned out of pending/notified, or (pinned) no longer carries the
+	// caller's token. Surface the current status so callers can return a
+	// meaningful 409 body.
+	currentStatus, err = s.cancelRejection(ctx, executionID, tokenHash != nil)
+	return false, currentStatus, err
+}
+
+// cancelRejection explains a zero-row cancelExecutionAtomic by probing the
+// row's current status. A row still pending/notified under a token pin lost
+// the pin, not the status race, so that case returns ErrApprovalTokenChanged.
+// Extracted to keep the caller under the gocyclo threshold.
+func (s *PostgresStore) cancelRejection(ctx context.Context, executionID string, tokenPinned bool) (currentStatus string, err error) {
 	existing, existErr := s.GetExecutionByID(ctx, executionID)
 	if existErr != nil {
-		return false, "", fmt.Errorf("execution not found or db error: %w", existErr)
+		return "", fmt.Errorf("execution not found or db error: %w", existErr)
 	}
-	return false, existing.Status, nil
+	if tokenPinned && (existing.Status == "pending" || existing.Status == "notified") {
+		return existing.Status, fmt.Errorf("%w: execution %s", ErrApprovalTokenChanged, executionID)
+	}
+	return existing.Status, nil
 }
 
 // CancelScheduledExecutionAtomic atomically transitions an execution from

internal/config/store_postgres_pgxmock_test.go

@@ -3088,12 +3088,13 @@ func TestPGXMock_CancelExecutionAtomic_WritesCanonicalStatus(t *testing.T) {
 
 	// The mock uses regexp matching. We pin on:
 	//   - "canceled_by" to confirm the new column is targeted (not cancelled_by)
-	//   - WithArgs: $1=execID, $2=&actor, $3=StatusCanceled
+	//   - WithArgs: $1=execID, $2=&actor, $3=StatusCanceled, $4=nil (no
+	//     token pin on the unpinned cancel, issue #1190)
 	// The RETURNING row also carries "canceled" so the caller's returned
 	// status is the canonical spelling end-to-end.
 	rows := pgxmock.NewRows([]string{"status"}).AddRow(StatusCanceled)
 	mock.ExpectQuery("canceled_by").
-		WithArgs("exec-1", &actor, StatusCanceled).
+		WithArgs("exec-1", &actor, StatusCanceled, (*string)(nil)).
 		WillReturnRows(rows)
 
 	// Pass mock directly as pgx.Tx (PgxPoolIface embeds pgx.Tx).