fix(api): answer 4xx for bad approval tokens and a missing revoke execution

internal/api/handler_purchases.go

@@ -636,6 +636,23 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio
 	return h.approvePurchaseViaSession(ctx, req, execution)
 }
 
+// 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.
+func tokenActionError(err error) error {
+	switch {
+	case errors.Is(err, purchase.ErrInvalidApprovalToken):
+		return NewClientError(403, "invalid approval token")
+	case errors.Is(err, purchase.ErrApprovalTokenExpired):
+		return NewClientError(410, "approval token has expired")
+	case errors.Is(err, config.ErrExecutionNotInExpectedStatus):
+		return NewClientError(409, err.Error())
+	default:
+		return err
+	}
+}
+
 // approveViaToken handles the email-link approve branch of approvePurchase to keep
 // that function under the cyclomatic limit.
 func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, token string) (any, error) {
@@ -692,7 +709,7 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio
 	// the hash.
 	revocationToken, approveErr := h.purchase.ApproveExecution(ctx, execution.ExecutionID, token, actor)
 	if approveErr != nil {
-		return nil, approveErr
+		return nil, tokenActionError(approveErr)
 	}
 	// Re-fetch the execution to pick up the FINAL state (status,
 	// completed_at, per-rec purchase results) ApproveAndExecute wrote --
@@ -1243,7 +1260,7 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction
 			return nil, err
 		}
 		if err := h.purchase.CancelExecution(ctx, execID, token, actor); err != nil {
-			return nil, err
+			return nil, tokenActionError(err)
 		}
 		return map[string]string{"status": "canceled"}, nil
 	}
@@ -1402,12 +1419,9 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun
 		return renderRevokeConfirmPage(execID, token), nil
 	}
 
-	execution, err := h.config.GetExecutionByID(ctx, execID)
+	execution, err := h.getExecutionOr404(ctx, execID)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get execution: %w", err)
-	}
-	if execution == nil {
-		return nil, NewClientError(404, "execution not found")
+		return nil, err
 	}
 
 	// Three-mode dispatch — same shape as cancelPurchase.
@@ -1445,6 +1459,19 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun
 	return h.revokeViaSession(ctx, execution, actor)
 }
 
+// getExecutionOr404 loads an execution, answering 404 for a missing one
+// (ErrNotFound or a nil row) and leaving real store errors as 500s.
+func (h *Handler) getExecutionOr404(ctx context.Context, execID string) (*config.PurchaseExecution, error) {
+	execution, err := h.config.GetExecutionByID(ctx, execID)
+	if errors.Is(err, config.ErrNotFound) || (err == nil && execution == nil) {
+		return nil, NewClientError(404, "execution not found")
+	}
+	if err != nil {
+		return nil, fmt.Errorf("failed to get execution: %w", err)
+	}
+	return execution, nil
+}
+
 // renderRevokeConfirmPage returns a rawResponse containing the HTML
 // confirmation form for GET /revoke. The form POSTs the token so the actual
 // mutation only happens when the user explicitly clicks "Confirm Revoke".

internal/api/handler_purchases_token_errors_test.go

@@ -0,0 +1,132 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"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/purchase"
+)
+
+// 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).
+
+const tokenErrExecID = "12345678-1234-1234-1234-123456789abc"
+const tokenErrRawToken = "raw-secret-token"
+
+func tokenErrHandler(t *testing.T, exec *config.PurchaseExecution, getErr error) *Handler {
+	t.Helper()
+	approver := "admin@example.com"
+	accountID := "acct-1"
+	store := new(MockConfigStore)
+	store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) {
+		return &config.CloudAccount{ID: id, ContactEmail: approver}, nil
+	}
+	if getErr != nil {
+		store.On("GetExecutionByID", mock.Anything, tokenErrExecID).Return(nil, getErr)
+	} else {
+		exec.ExecutionID = tokenErrExecID
+		exec.Recommendations = []config.RecommendationRecord{{ID: "r1", CloudAccountID: &accountID}}
+		store.On("GetExecutionByID", mock.Anything, tokenErrExecID).Return(exec, nil)
+	}
+	store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{NotificationEmail: &approver}, nil)
+
+	auth := new(MockAuthService)
+	auth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe()
+	auth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil)
+	for _, perm := range []string{"approve-any", "approve-own", "cancel-any", "cancel-own", "revoke-any", "revoke-own"} {
+		auth.On("HasPermissionAPI", mock.Anything, "", perm, "purchases").Return(false, nil).Maybe()
+	}
+
+	return &Handler{
+		config:   store,
+		auth:     auth,
+		purchase: purchase.NewManager(purchase.ManagerConfig{ConfigStore: store}),
+	}
+}
+
+func tokenErrRequest(action, token string) *events.LambdaFunctionURLRequest {
+	return &events.LambdaFunctionURLRequest{
+		QueryStringParameters: map[string]string{"token": token},
+		Headers:               map[string]string{"authorization": "Bearer sess-tok"},
+		RequestContext: events.LambdaFunctionURLRequestContext{
+			HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{
+				Method: "POST",
+				Path:   "/api/purchases/" + action + "/" + tokenErrExecID,
+			},
+		},
+	}
+}
+
+func TestHandleRequest_TokenActions_TokenAndStatusErrorsAre4xx(t *testing.T) {
+	past := time.Now().Add(-time.Hour)
+	future := time.Now().Add(time.Hour)
+	hash := config.HashApprovalToken(tokenErrRawToken)
+
+	cases := []struct {
+		name   string
+		action string
+		exec   config.PurchaseExecution
+		token  string
+		want   int
+	}{
+		{"approve invalid token", "approve", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, "wrong", 403},
+		{"approve expired token", "approve", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410},
+		{"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},
+	}
+	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(tc.action, tc.token))
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, resp.StatusCode, resp.Body)
+			var body map[string]any
+			require.NoError(t, json.Unmarshal([]byte(resp.Body), &body))
+			assert.NotEmpty(t, body["error"])
+		})
+	}
+}
+
+func TestHandleRequest_ApproveViaToken_NonPendingExecutionIs409(t *testing.T) {
+	future := time.Now().Add(time.Hour)
+	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, "approved", mock.Anything).
+		Return(nil, config.ErrExecutionNotInExpectedStatus)
+
+	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken))
+	require.NoError(t, err)
+	assert.Equal(t, 409, resp.StatusCode, resp.Body)
+}
+
+func TestHandleRequest_RevokeViaEmail_MissingExecutionIs404(t *testing.T) {
+	h := tokenErrHandler(t, nil, config.ErrNotFound)
+	resp, err := h.HandleRequest(context.Background(), tokenErrRequest("revoke", tokenErrRawToken))
+	require.NoError(t, err)
+	assert.Equal(t, 404, resp.StatusCode, resp.Body)
+}
+
+func TestHandleRequest_CancelViaToken_LostCASIs409(t *testing.T) {
+	future := time.Now().Add(time.Hour)
+	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).
+		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)
+}

internal/api/openapi.yaml

@@ -558,7 +558,11 @@ paths:
             credentials), the session lacks approve permission or
             exceeds its Constraints, the caller is not the account's contact
             email, no contact email is configured on the execution's
-            accounts, or 4-eyes mode rejects the approver.
+            accounts, 4-eyes mode rejects the approver, or (token mode) the
+            approval token is invalid. Token validation applies on the
+            immediate-approval path only; the delayed-approval path (purchase
+            delay configured) does not validate the token yet (tracked
+            separately).
           content:
             application/json:
               schema:
@@ -574,20 +578,27 @@ paths:
         '409':
           description: >
             The execution cannot be approved: a non-AWS execution without a
-            resolvable cloud account, or (session mode) a status other than
-            pending or notified, the status changed concurrently, or the
+            resolvable cloud account, a status other than pending or notified
+            (in token mode the status check is the atomic transition, so a
+            concurrent change answers the same way), or (session mode) the
             purchase itself failed during execution.
           content:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+        '410':
+          description: >
+            Token mode only, the approval token has expired (immediate-approval
+            path only; the delayed-approval path does not validate the token yet,
+            tracked separately).
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
         '429':
           $ref: '#/components/responses/RateLimited'
         '500':
-          description: >
-            Server error. Currently also returned in token mode for an
-            invalid or expired approval token. Known defect tracked in #564;
-            this will become a 4xx, so do not rely on the 500.
+          description: Server error.
           content:
             application/json:
               schema:
@@ -652,8 +663,9 @@ paths:
           description: >
             CSRF validation failed (this includes a request with no
             credentials), the session lacks cancel permission, the caller is
-            not the account's contact email, or no contact email is
-            configured on the execution's accounts.
+            not the account's contact email, no contact email is
+            configured on the execution's accounts, or (token mode) the
+            approval token is invalid.
           content:
             application/json:
               schema:
@@ -668,21 +680,24 @@ paths:
                 $ref: '#/components/schemas/Error'
         '409':
           description: >
-            Session mode only: the execution is not cancelable (scheduled
+            The execution is not cancelable (in session mode, scheduled
             executions must use the revoke endpoint), or its status changed
             concurrently.
           content:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
+        '410':
+          description: >
+            Token mode only, the approval token has expired.
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
         '429':
           $ref: '#/components/responses/RateLimited'
         '500':
-          description: >
-            Server error. Currently also returned in token mode for an
-            invalid or expired token, or an execution that is no longer
-            pending. Known defect tracked in #564; this will become a 4xx,
-            so do not rely on the 500.
+          description: Server error.
           content:
             application/json:
               schema:
@@ -761,7 +776,8 @@ paths:
               schema:
                 $ref: '#/components/schemas/Error'
         '404':
-          description: Execution outside the session's account scope.
+          description: >
+            Execution not found, or outside the session's account scope.
           content:
             application/json:
               schema:
@@ -777,10 +793,7 @@ paths:
         '429':
           $ref: '#/components/responses/RateLimited'
         '500':
-          description: >
-            Server error. Currently also returned for a non-existent
-            execution id. Known defect tracked in #564; this will become a
-            404, so do not rely on the 500.
+          description: Server error.
           content:
             application/json:
               schema:

internal/purchase/approvals.go

@@ -17,6 +17,13 @@ import (
 // opposed to a failure to evaluate it (config or store errors).
 var ErrFourEyesDenied = errors.New("approval declined")
 
+// ErrInvalidApprovalToken and ErrApprovalTokenExpired let the HTTP layer map a
+// bad email-link token to a 4xx instead of a 500.
+var (
+	ErrInvalidApprovalToken = errors.New("invalid approval token")
+	ErrApprovalTokenExpired = errors.New("approval token has expired")
+)
+
 // runNowSkipReason is stamped onto PreApprovalSkipReason when an operator's
 // "Run now" stands in for the approval and the scheduled wait.
 const runNowSkipReason = "run-now"
@@ -110,13 +117,13 @@ func maskActor(actor string) string {
 // supplied token and compares digests in constant time.
 func validateApprovalToken(execution *config.PurchaseExecution, token string) error {
 	if execution.ApprovalToken == "" || token == "" {
-		return fmt.Errorf("invalid approval token")
+		return ErrInvalidApprovalToken
 	}
 	if !config.ApprovalTokenMatches(execution.ApprovalToken, token) {
-		return fmt.Errorf("invalid approval token")
+		return ErrInvalidApprovalToken
 	}
 	if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) {
-		return fmt.Errorf("approval token has expired")
+		return ErrApprovalTokenExpired
 	}
 	return nil
 }
@@ -465,7 +472,7 @@ func (m *Manager) CancelExecution(ctx context.Context, executionID, token, actor
 	}
 
 	if !canceled {
-		return fmt.Errorf("execution %s cannot be canceled: concurrent operation already transitioned it to %q", executionID, currentStatus)
+		return fmt.Errorf("%w: execution %s cannot be canceled: concurrent operation already transitioned it to %q", config.ErrExecutionNotInExpectedStatus, executionID, currentStatus)
 	}
 
 	logging.Infof("Execution %s canceled", executionID)
@@ -555,20 +562,12 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke
 	if err != nil {
 		return nil, fmt.Errorf("failed to get execution: %w", err)
 	}
-	if execution.ApprovalToken == "" || token == "" {
-		return nil, fmt.Errorf("invalid approval token")
-	}
 	// execution.ApprovalToken is the SHA-256 hex digest stored at rest
-	// (issue #103); config.ApprovalTokenMatches hashes the supplied token
-	// and compares digests in constant time.
-	if !config.ApprovalTokenMatches(execution.ApprovalToken, token) {
-		return nil, fmt.Errorf("invalid approval token")
-	}
-
-	// Enforce token TTL (issue #397). Same backward-compat nil-guard as
-	// ApproveExecution: legacy rows without ApprovalTokenExpiresAt pass through.
-	if execution.ApprovalTokenExpiresAt != nil && time.Now().After(*execution.ApprovalTokenExpiresAt) {
-		return nil, fmt.Errorf("approval token has expired")
+	// (issue #103); validateApprovalToken hashes the supplied token and
+	// compares digests in constant time, and enforces the TTL (issue #397;
+	// legacy rows without ApprovalTokenExpiresAt pass).
+	if err := validateApprovalToken(execution, token); err != nil {
+		return nil, err
 	}
 
 	// Only pre-purchase rows (pending/notified/scheduled) are cancelable --
@@ -583,7 +582,7 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke
 	// mid-execution (the AWS commitment is being or has been created), so
 	// canceling them would leave the DB and the cloud out of sync.
 	if !execution.IsCancelable() {
-		return nil, fmt.Errorf("execution cannot be canceled, current status: %s", execution.Status)
+		return nil, fmt.Errorf("%w: execution cannot be canceled, current status: %s", config.ErrExecutionNotInExpectedStatus, execution.Status)
 	}
 	return execution, nil
 }