diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 4f34b3cc8..d62232a6d 100644 --- a/internal/api/handler_purchases.go +++ b/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". diff --git a/internal/api/handler_purchases_token_errors_test.go b/internal/api/handler_purchases_token_errors_test.go new file mode 100644 index 000000000..7d9bd9efe --- /dev/null +++ b/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) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 7adc52506..2095152d2 100644 --- a/internal/api/openapi.yaml +++ b/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: diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index dbd04a31c..ace1206fc 100644 --- a/internal/purchase/approvals.go +++ b/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 }