fix(api/purchases): enforce revoke-token expiry, atomic status, PII-safe log

Address CodeRabbit pass-1 findings on the one-click revoke path (#291):

- validateRevokeToken now rejects an expired ApprovalToken (409) before
  the constant-time match, mirroring ApproveExecution's expiry guard in
  internal/purchase/approvals.go. Legacy rows without
  ApprovalTokenExpiresAt (pre-migration 000051) still pass through.
- revokeViaSession replaces the read-modify-write SavePurchaseExecution
  with the existing atomic TransitionExecutionStatus conditional UPDATE
  (completed|partially_completed -> revocation_requested), closing a
  lost-update race; CAS rejection maps to 409 and a vanished row to 404.
  CancelledBy is stamped in a follow-up save on the freshly-returned row.
- The revocation log line now redacts the actor email via redactEmail
  instead of printing it raw, removing PII from logs.

Tests:
- Add session-auth revoke branch coverage (tryRevokeViaSession ->
  revokeViaSession): admin cancel-any, owner cancel-own, and a
  no-permission/no-token denial.
- Add regression tests for the expired-token rejection and the
  concurrent-transition 409.
- Switch MockEmailSender.SendPurchaseExecutedNotification to testify's
  m.Called recorder in internal/purchase and internal/scheduler so the
  method participates in expectation/assertion coverage.

Author: cristim

internal/api/handler_purchases.go

@@ -809,13 +809,20 @@ func checkRevokableStatus(execution *config.PurchaseExecution) error {
 }
 
 // validateRevokeToken checks that the execution carries a non-empty
-// ApprovalToken and that it matches the supplied token using constant-time
-// comparison (same guard as ApproveExecution in internal/purchase/approvals.go).
+// ApprovalToken, that the token has not expired, and that it matches the
+// supplied token using constant-time comparison (same guard as
+// ApproveExecution in internal/purchase/approvals.go). Pre-migration rows
+// without an ApprovalTokenExpiresAt (legacy executions created before
+// migration 000051) pass the expiry check, mirroring ApproveExecution.
 // Extracted from revokePurchase to keep that function under the cyclomatic limit.
 func validateRevokeToken(execution *config.PurchaseExecution, token string) error {
 	if execution.ApprovalToken == "" {
 		return NewClientError(403, "invalid revocation token")
 	}
+	if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) {
+		return NewClientError(409, fmt.Sprintf(
+			"execution %s cannot be revoked: the revocation link has expired", execution.ExecutionID))
+	}
 	if subtle.ConstantTimeCompare([]byte(execution.ApprovalToken), []byte(token)) != 1 {
 		return NewClientError(403, "invalid revocation token")
 	}
@@ -827,16 +834,35 @@ func validateRevokeToken(execution *config.PurchaseExecution, token string) erro
 // revocation (AWS support-case) is out of scope for issue #291 — it is
 // handled by the sibling "AWS RI/SP revocation via support case" issue.
 // This call records the intent so the History UI can surface it.
+//
+// The status flip uses TransitionExecutionStatus, which issues a conditional
+// UPDATE WHERE status IN ('completed','partially_completed'). This guards
+// against lost-update races: if the row changed between the GetExecutionByID
+// read and this write (e.g. a concurrent transition), zero rows are affected
+// and we return a 409 rather than blindly overwriting. CancelledBy is stamped
+// in a follow-up SavePurchaseExecution on the freshly-returned row.
 func (h *Handler) revokeViaSession(ctx context.Context, execution *config.PurchaseExecution, revokedBy string) (any, error) {
-	execution.Status = "revocation_requested"
+	updated, err := h.config.TransitionExecutionStatus(
+		ctx, execution.ExecutionID,
+		[]string{"completed", "partially_completed"}, "revocation_requested")
+	if err != nil {
+		if errors.Is(err, config.ErrExecutionNotInExpectedStatus) {
+			return nil, NewClientError(409, fmt.Sprintf(
+				"execution %s cannot be revoked: a concurrent operation changed its status", execution.ExecutionID))
+		}
+		if errors.Is(err, config.ErrNotFound) {
+			return nil, NewClientError(404, "execution not found")
+		}
+		return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err)
+	}
 	if revokedBy != "" {
 		rb := revokedBy
-		execution.CancelledBy = &rb
-	}
-	if err := h.config.SavePurchaseExecution(ctx, execution); err != nil {
-		return nil, fmt.Errorf("failed to record revocation request for execution %s: %w", execution.ExecutionID, err)
+		updated.CancelledBy = &rb
+		if err := h.config.SavePurchaseExecution(ctx, updated); err != nil {
+			return nil, fmt.Errorf("failed to record revocation requester for execution %s: %w", execution.ExecutionID, err)
+		}
 	}
-	logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, revokedBy)
+	logging.Infof("Revocation requested for execution %s by %s", execution.ExecutionID, redactEmail(revokedBy))
 	return map[string]string{
 		"status":  "revocation_requested",
 		"message": "Revocation request recorded. Contact AWS Support to complete the cancellation within the allowed window.",

internal/api/handler_purchases_test.go

@@ -3313,9 +3313,15 @@ func TestHandler_revokePurchase_ValidToken(t *testing.T) {
 	mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
 		return &config.CloudAccount{ID: id, ContactEmail: revokerEmail}, nil
 	}
-	// SavePurchaseExecution for the revocation_requested update.
+	// Atomic conditional transition completed/partially_completed ->
+	// revocation_requested, returning the updated row.
+	mockStore.On("TransitionExecutionStatus", ctx, execID,
+		[]string{"completed", "partially_completed"}, "revocation_requested").
+		Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil)
+	// SavePurchaseExecution stamps CancelledBy on the returned row.
 	mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool {
-		return e.ExecutionID == execID && e.Status == "revocation_requested"
+		return e.ExecutionID == execID && e.Status == "revocation_requested" &&
+			e.CancelledBy != nil && *e.CancelledBy == revokerEmail
 	})).Return(nil)
 
 	mockAuth := new(MockAuthService)
@@ -3425,6 +3431,190 @@ func TestHandler_revokePurchase_NotFound(t *testing.T) {
 	assert.Equal(t, 404, ce.code)
 }
 
+// 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.
+// Regression test for the revoke-token expiry CR finding (PR #889).
+func TestHandler_revokePurchase_ExpiredToken(t *testing.T) {
+	ctx := context.Background()
+	execID := "55555555-5555-5555-5555-555555555555"
+	token := "expiredbutmatching"
+	contactEmail := "contact@acct.example.com"
+	accountID := "acct-1"
+	past := time.Now().Add(-1 * time.Hour)
+
+	exec := &config.PurchaseExecution{
+		ExecutionID:            execID,
+		ApprovalToken:          token,
+		ApprovalTokenExpiresAt: &past,
+		Status:                 "completed",
+		Recommendations: []config.RecommendationRecord{
+			{ID: "r1", CloudAccountID: &accountID},
+		},
+	}
+
+	mockStore := new(MockConfigStore)
+	mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+	mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil)
+	mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil
+	}
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil)
+	mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe()
+	mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe()
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers:               map[string]string{"authorization": "Bearer sess-tok"},
+		QueryStringParameters: map[string]string{"token": token},
+	}
+	_, err := handler.revokePurchase(ctx, req, execID, token)
+	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.Contains(t, ce.message, "expired")
+}
+
+// TestHandler_revokePurchase_SessionAdminCancelAny exercises the tokenless
+// session-authorized revoke branch (tryRevokeViaSession -> revokeViaSession)
+// for an admin holding cancel-any on purchases. It asserts the atomic
+// TransitionExecutionStatus call and the revocation_requested result.
+// Covers the session-auth revoke branch CR finding (PR #889).
+func TestHandler_revokePurchase_SessionAdminCancelAny(t *testing.T) {
+	ctx := context.Background()
+	execID := "66666666-6666-6666-6666-666666666666"
+	adminEmail := "admin@example.com"
+	adminUserID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"
+
+	exec := buildCompletedExec(execID, "unused-token")
+
+	mockStore := new(MockConfigStore)
+	mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+	mockStore.On("TransitionExecutionStatus", ctx, execID,
+		[]string{"completed", "partially_completed"}, "revocation_requested").
+		Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil)
+	mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool {
+		return e.ExecutionID == execID && e.CancelledBy != nil && *e.CancelledBy == adminEmail
+	})).Return(nil)
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "admin-token").
+		Return(&Session{UserID: adminUserID, Email: adminEmail}, nil)
+	mockAuth.On("HasPermissionAPI", ctx, adminUserID, "cancel-any", "purchases").Return(true, nil)
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	// No token query param: forces the tokenless session-auth path.
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer admin-token"},
+	}
+	result, err := handler.revokePurchase(ctx, req, execID, "")
+	require.NoError(t, err, "session admin with cancel-any must succeed")
+	resultMap := result.(map[string]string)
+	assert.Equal(t, "revocation_requested", resultMap["status"])
+	mockStore.AssertExpectations(t)
+	mockAuth.AssertExpectations(t)
+}
+
+// TestHandler_revokePurchase_SessionOwnerCancelOwn exercises the tokenless
+// session-auth branch for a non-admin owner holding cancel-own on a purchase
+// they created. Covers the session-auth revoke branch CR finding (PR #889).
+func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) {
+	ctx := context.Background()
+	execID := "77777777-7777-7777-7777-777777777777"
+	ownerEmail := "owner@example.com"
+	ownerUserID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"
+
+	exec := buildCompletedExec(execID, "unused-token")
+	exec.CreatedByUserID = &ownerUserID
+
+	mockStore := new(MockConfigStore)
+	mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+	mockStore.On("TransitionExecutionStatus", ctx, execID,
+		[]string{"completed", "partially_completed"}, "revocation_requested").
+		Return(&config.PurchaseExecution{ExecutionID: execID, Status: "revocation_requested"}, nil)
+	mockStore.On("SavePurchaseExecution", ctx, mock.MatchedBy(func(e *config.PurchaseExecution) bool {
+		return e.ExecutionID == execID && e.CancelledBy != nil && *e.CancelledBy == ownerEmail
+	})).Return(nil)
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "owner-token").
+		Return(&Session{UserID: ownerUserID, Email: ownerEmail}, nil)
+	mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-any", "purchases").Return(false, nil)
+	mockAuth.On("HasPermissionAPI", ctx, ownerUserID, "cancel-own", "purchases").Return(true, nil)
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer owner-token"},
+	}
+	result, err := handler.revokePurchase(ctx, req, execID, "")
+	require.NoError(t, err, "session owner with cancel-own must succeed")
+	resultMap := result.(map[string]string)
+	assert.Equal(t, "revocation_requested", resultMap["status"])
+	mockStore.AssertExpectations(t)
+	mockAuth.AssertExpectations(t)
+}
+
+// TestHandler_revokePurchase_SessionNoPermissionNoToken verifies that a session
+// lacking both cancel-any and cancel-own, with no token supplied, is denied:
+// the session branch returns permission-denied and falls through to the
+// tokenless 401. Covers the session-auth revoke branch CR finding (PR #889).
+func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) {
+	ctx := context.Background()
+	execID := "88888888-8888-8888-8888-888888888888"
+	userEmail := "nobody@example.com"
+	userID := "cccccccc-cccc-cccc-cccc-cccccccccccc"
+
+	exec := buildCompletedExec(execID, "unused-token")
+
+	mockStore := new(MockConfigStore)
+	mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil)
+
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", ctx, "user-token").
+		Return(&Session{UserID: userID, Email: userEmail}, nil)
+	mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil)
+	mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(false, nil)
+
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{
+		Headers: map[string]string{"authorization": "Bearer user-token"},
+	}
+	_, err := handler.revokePurchase(ctx, req, execID, "")
+	require.Error(t, err, "no permission and no token must be denied")
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a client error")
+	assert.Equal(t, 401, ce.code)
+	mockStore.AssertExpectations(t)
+	mockAuth.AssertExpectations(t)
+}
+
+// TestHandler_revokeViaSession_ConcurrentTransition verifies that a lost-update
+// race (the row's status changed between read and the conditional UPDATE) is
+// surfaced as a 409 rather than silently overwriting. Regression test for the
+// atomic-status-transition CR finding (PR #889).
+func TestHandler_revokeViaSession_ConcurrentTransition(t *testing.T) {
+	ctx := context.Background()
+	execID := "99999999-9999-9999-9999-999999999999"
+
+	exec := buildCompletedExec(execID, "tok")
+
+	mockStore := new(MockConfigStore)
+	mockStore.On("TransitionExecutionStatus", ctx, execID,
+		[]string{"completed", "partially_completed"}, "revocation_requested").
+		Return(nil, fmt.Errorf("%w: execution %s", config.ErrExecutionNotInExpectedStatus, execID))
+
+	handler := &Handler{config: mockStore}
+	_, err := handler.revokeViaSession(ctx, exec, "someone@example.com")
+	require.Error(t, err, "a concurrent transition must surface an error")
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a client error")
+	assert.Equal(t, 409, ce.code)
+	mockStore.AssertExpectations(t)
+}
+
 // ---------------------------------------------------------------------------
 // resolveExecutedNotificationRecipients unit tests (issue #291)
 // ---------------------------------------------------------------------------

internal/purchase/mocks_test.go

@@ -659,8 +659,9 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data
 	args := m.Called(ctx, data)
 	return args.Error(0)
 }
-func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error {
-	return nil
+func (m *MockEmailSender) SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error {
+	args := m.Called(ctx, data)
+	return args.Error(0)
 }
 func (m *MockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error {
 	return nil

internal/scheduler/scheduler_test.go

@@ -539,8 +539,9 @@ func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data
 	args := m.Called(ctx, data)
 	return args.Error(0)
 }
-func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error {
-	return nil
+func (m *MockEmailSender) SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error {
+	args := m.Called(ctx, data)
+	return args.Error(0)
 }
 func (m *MockEmailSender) SendRegistrationReceivedNotification(_ context.Context, _ email.RegistrationNotificationData) error {
 	return nil