fix(api): pin the approval token inside the delayed-approval status CAS

internal/analytics/collector_test.go

@@ -311,6 +311,10 @@ func (m *mockConfigStore) TransitionExecutionStatusPinned(ctx context.Context, e
 	return nil, nil
 }
 
+func (m *mockConfigStore) TransitionExecutionStatusTokenPinned(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string, rawToken string) (*config.PurchaseExecution, error) {
+	return nil, nil
+}
+
 func (m *mockConfigStore) RescheduleAwaitingExecution(_ context.Context, _ string, _ time.Time) (bool, error) {
 	return false, nil
 }

internal/api/handler_purchases.go

@@ -763,11 +763,14 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio
 	}
 	if globalCfg.GetPurchaseDelay() > 0 {
 		// The delayed branch bypasses ApproveExecution, which is where the
-		// token is otherwise validated (issue #586).
+		// token is otherwise validated (issue #586). This check reads the
+		// snapshot loaded at the start of the request; approveWithDelay pins
+		// the token again inside the status CAS so a token rotated since
+		// cannot win the transition (issue #590).
 		if err := purchase.ValidateApprovalToken(execution, token); err != nil {
 			return nil, tokenActionError(err)
 		}
-		return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), actor, nil)
+		return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), actor, nil, token)
 	}
 	// ApproveExecution now runs the purchase synchronously inside the
 	// same call (issue #372). When it returns nil the AWS API call
@@ -856,7 +859,7 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam
 		return nil, fmt.Errorf("failed to read global config for purchase delay check: %w", cfgErr)
 	}
 	if globalCfg.GetPurchaseDelay() > 0 {
-		return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), session.Email, actor)
+		return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), session.Email, actor, "")
 	}
 
 	// revocationToken is the raw token ApproveAndExecute mints for this
@@ -1051,9 +1054,17 @@ func (h *Handler) requirePlanStepCap(ctx context.Context, execution *config.Purc
 // scheduled_execution_at <= NOW() and fires the actual SDK call.
 // Revoking a status=scheduled execution (via the revoke handler or the
 // History "Revoke" button) transitions it to "canceled" at zero cloud cost.
-func (h *Handler) approveWithDelay(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string, transitionedBy *string) (any, error) {
-	updated, err := h.scheduleApprovedExecution(ctx, execution, delay, actor, transitionedBy)
+//
+// approvalToken is the raw email-link token on the token path and "" on the
+// session path; see scheduleApprovedExecution.
+func (h *Handler) approveWithDelay(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string, transitionedBy *string, approvalToken string) (any, error) {
+	updated, err := h.scheduleApprovedExecution(ctx, execution, delay, actor, transitionedBy, approvalToken)
 	if err != nil {
+		if errors.Is(err, config.ErrApprovalTokenChanged) {
+			// The token was rotated or expired after approveViaToken validated
+			// it against the request's snapshot (issue #590).
+			return nil, NewClientError(403, "invalid approval token")
+		}
 		if errors.Is(err, config.ErrExecutionNotInExpectedStatus) {
 			// A concurrent Cancel beat the approve: the CAS rejected because the row
 			// is no longer in a schedulable status. Return 409 so the client knows to
@@ -1090,8 +1101,21 @@ func (h *Handler) approveWithDelay(ctx context.Context, execution *config.Purcha
 // "canceled" between the caller's SELECT and this write, TransitionExecutionStatus
 // returns ErrExecutionNotInExpectedStatus and we surface a 409 instead of
 // blindly overwriting the canceled state.
-func (h *Handler) scheduleApprovedExecution(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string, transitionedBy *string) (*config.PurchaseExecution, error) {
-	updated, err := h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, []string{"pending", "notified"}, "scheduled", transitionedBy)
+//
+// A non-empty approvalToken (the email-link path) is also pinned inside the
+// CAS: the row's stored token hash must still match it and be unexpired, so
+// a re-notification that rotated the token after the caller validated its
+// snapshot rejects the transition with config.ErrApprovalTokenChanged
+// (issue #590). The session path passes "" and is not token-gated.
+func (h *Handler) scheduleApprovedExecution(ctx context.Context, execution *config.PurchaseExecution, delay time.Duration, actor string, transitionedBy *string, approvalToken string) (*config.PurchaseExecution, error) {
+	from := []string{"pending", "notified"}
+	var updated *config.PurchaseExecution
+	var err error
+	if approvalToken != "" {
+		updated, err = h.config.TransitionExecutionStatusTokenPinned(ctx, execution.ExecutionID, from, "scheduled", transitionedBy, approvalToken)
+	} else {
+		updated, err = h.config.TransitionExecutionStatus(ctx, execution.ExecutionID, from, "scheduled", transitionedBy)
+	}
 	if err != nil {
 		return nil, fmt.Errorf("failed to transition execution %s to scheduled: %w", execution.ExecutionID, err)
 	}

internal/api/handler_purchases_delay_token_test.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -16,7 +17,7 @@ import (
 // token exactly like the immediate branch (issue #586), reusing the harness
 // from handler_purchases_token_errors_test.go.
 
-func delayTokenHandler(t *testing.T, exec *config.PurchaseExecution, delayHours int) *Handler {
+func delayTokenHandler(t *testing.T, exec *config.PurchaseExecution) *Handler {
 	t.Helper()
 	h := tokenErrHandler(t, exec, nil)
 	store := h.config.(*MockConfigStore)
@@ -29,7 +30,7 @@ func delayTokenHandler(t *testing.T, exec *config.PurchaseExecution, delayHours
 	store.ExpectedCalls = kept
 	approver := "admin@example.com"
 	store.On("GetGlobalConfig", mock.Anything).
-		Return(&config.GlobalConfig{NotificationEmail: &approver, PurchaseDelayHours: delayHours}, nil)
+		Return(&config.GlobalConfig{NotificationEmail: &approver, PurchaseDelayHours: 1}, nil)
 	return h
 }
 
@@ -51,43 +52,77 @@ func TestHandleRequest_ApproveViaToken_DelayConfigured_ValidatesToken(t *testing
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
 			exec := tc.exec
-			h := delayTokenHandler(t, &exec, 1)
+			h := delayTokenHandler(t, &exec)
 			store := h.config.(*MockConfigStore)
 			// Registered so a regression answers 200 instead of panicking on an unmocked call.
+			scheduled := &config.PurchaseExecution{ExecutionID: tokenErrExecID, Status: "scheduled"}
 			store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, mock.Anything, "scheduled", mock.Anything).
-				Return(&config.PurchaseExecution{ExecutionID: tokenErrExecID, Status: "scheduled"}, nil).Maybe()
+				Return(scheduled, nil).Maybe()
+			store.On("TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, mock.Anything, "scheduled", mock.Anything, mock.Anything).
+				Return(scheduled, nil).Maybe()
 			store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe()
 			resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tc.token))
 			require.NoError(t, err)
 			assert.Equal(t, tc.want, resp.StatusCode, resp.Body)
 			store.AssertNotCalled(t, "TransitionExecutionStatus",
 				mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+			store.AssertNotCalled(t, "TransitionExecutionStatusTokenPinned",
+				mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
 		})
 	}
 }
 
 func TestHandleRequest_ApproveViaToken_DelayConfigured_ValidTokenSchedules(t *testing.T) {
 	future := time.Now().Add(time.Hour)
 	exec := &config.PurchaseExecution{Status: "pending", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future}
-	h := delayTokenHandler(t, exec, 1)
+	h := delayTokenHandler(t, exec)
 	store := h.config.(*MockConfigStore)
 	scheduled := &config.PurchaseExecution{ExecutionID: tokenErrExecID, Status: "scheduled"}
-	store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything).
-		Return(scheduled, nil)
+	// The CAS must carry the supplied token so the store re-checks it against
+	// the row at transition time, not only against the request's snapshot
+	// (issue #590).
+	store.On("TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything, tokenErrRawToken).
+		Return(scheduled, nil).Once()
 	store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil)
 
 	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken))
 	require.NoError(t, err)
 	assert.Equal(t, 200, resp.StatusCode, resp.Body)
 	assert.Contains(t, resp.Body, "scheduled")
+	store.AssertExpectations(t)
+	store.AssertNotCalled(t, "TransitionExecutionStatus",
+		mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+// TestHandleRequest_ApproveViaToken_DelayConfigured_TokenRotatedBeforeCASIs403
+// covers issue #590: the request's snapshot still holds the hash of the
+// supplied token, so the pre-check passes, but a re-notification rotated the
+// stored token before the status CAS. The token-pinned CAS rejects it, and the
+// handler must answer 403 without scheduling or stamping the row.
+func TestHandleRequest_ApproveViaToken_DelayConfigured_TokenRotatedBeforeCASIs403(t *testing.T) {
+	future := time.Now().Add(time.Hour)
+	exec := &config.PurchaseExecution{Status: "notified", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future}
+	h := delayTokenHandler(t, exec)
+	store := h.config.(*MockConfigStore)
+	store.On("TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything, tokenErrRawToken).
+		Return(nil, fmt.Errorf("%w: execution %s", config.ErrApprovalTokenChanged, tokenErrExecID)).Once()
+
+	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken))
+	require.NoError(t, err)
+	assert.Equal(t, 403, resp.StatusCode, resp.Body)
+	assert.Contains(t, resp.Body, "invalid approval token")
+	store.AssertExpectations(t)
+	store.AssertNotCalled(t, "TransitionExecutionStatus",
+		mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+	store.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything)
 }
 
 func TestHandleRequest_ApproveViaToken_DelayConfigured_ReplayOfScheduledIs409(t *testing.T) {
 	future := time.Now().Add(time.Hour)
 	exec := &config.PurchaseExecution{Status: "scheduled", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future}
-	h := delayTokenHandler(t, exec, 1)
+	h := delayTokenHandler(t, exec)
 	store := h.config.(*MockConfigStore)
-	store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything).
+	store.On("TransitionExecutionStatusTokenPinned", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything, tokenErrRawToken).
 		Return(nil, config.ErrExecutionNotInExpectedStatus)
 
 	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken))

internal/api/handler_purchases_test.go

@@ -5614,7 +5614,7 @@ func TestHandler_scheduleApprovedExecution_CASGuardsConcurrentCancel(t *testing.
 
 	handler := &Handler{config: mockConfig}
 
-	_, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com", nil)
+	_, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com", nil, "")
 	require.Error(t, err, "concurrent cancel must surface as an error, not a silent overwrite")
 	// SavePurchaseExecution must NEVER be called: the canceled row is untouched.
 	mockConfig.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything)
@@ -5650,7 +5650,7 @@ func TestHandler_scheduleApprovedExecution_HappyPath(t *testing.T) {
 
 	handler := &Handler{config: mockConfig}
 
-	result, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com", nil)
+	result, err := handler.scheduleApprovedExecution(ctx, exec, 48*time.Hour, "actor@example.com", nil, "")
 	require.NoError(t, err)
 	assert.Equal(t, "scheduled", result.Status)
 	assert.NotNil(t, result.ScheduledExecutionAt, "ScheduledExecutionAt must be stamped")
@@ -5674,7 +5674,7 @@ func TestApproveWithDelay_CASLostMaps409(t *testing.T) {
 
 	handler := &Handler{config: mockConfig}
 
-	_, err := handler.approveWithDelay(ctx, exec, 48*time.Hour, "actor@example.com", nil)
+	_, err := handler.approveWithDelay(ctx, exec, 48*time.Hour, "actor@example.com", nil, "")
 	require.Error(t, err)
 	ce, ok := IsClientError(err)
 	require.True(t, ok, "CAS-lost error must be a ClientError")

internal/config/errors.go

@@ -23,6 +23,13 @@ var ErrPurchasePlanConflict = errors.New("purchase plan changed")
 // distinguish this legitimate race-loss from a hard DB error.
 var ErrExecutionNotInExpectedStatus = errors.New("execution not in expected status")
 
+// 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
+// caller supplied, or has expired: the token was rotated (re-notification) or
+// lapsed after the caller validated it and before the transition (issue #590).
+var ErrApprovalTokenChanged = errors.New("approval token no longer matches the execution")
+
 // ErrRampStepIncomplete is returned (wrapped) by CompletePlanStep when the
 // ramp step the completing execution belongs to has not been bought in full:
 // another cloud account on that step is still outstanding, or nothing on the

internal/config/interfaces.go

@@ -225,6 +225,14 @@ type StoreInterface interface {
 	// row whose recommendations were attached after the caller's gate check. A
 	// lost pin returns ErrExecutionNotInExpectedStatus like any lost CAS.
 	TransitionExecutionStatusPinned(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string, sawEmptyRecommendations bool) (*PurchaseExecution, error)
+	// TransitionExecutionStatusTokenPinned is TransitionExecutionStatus 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 caller validated it cannot win the
+	// transition (issue #590). A lost token pin on a row still in an allowed
+	// status returns ErrApprovalTokenChanged; a lost status CAS returns
+	// ErrExecutionNotInExpectedStatus as usual.
+	TransitionExecutionStatusTokenPinned(ctx context.Context, executionID string, fromStatuses []string, toStatus string, actor *string, rawToken string) (*PurchaseExecution, error)
 	// RescheduleAwaitingExecution sets scheduled_date on an execution that is
 	// still pending or notified and touches no other column. It returns false
 	// (and writes nothing) when the row has left those statuses, so a cancel or