Skip to content

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

Merged
cristim merged 3 commits into
mainfrom
fix/approval-token-errors-4xx
Oct 6, 2026
Merged

cristim merged 3 commits into
mainfrom
fix/approval-token-errors-4xx

Conversation

@cristim

@cristim cristim commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Invalid approval token on email-link approve/cancel now answers 403 (same as the RI exchange approve path), an expired token 410, and a non-pending or concurrently transitioned execution in token mode 409. purchase exposes ErrInvalidApprovalToken and ErrApprovalTokenExpired; the cancel path reuses validateApprovalToken instead of a duplicate copy of the checks.
  • POST /api/purchases/revoke/{id} (email revoke) answers 404 for a missing execution instead of 500.
  • openapi.yaml: removed the "known defect fix(api): invalid or expired approval tokens and a missing revoke execution answer 500 instead of 4xx #564" text, documented 403/409/410/404 for these operations.

Scope: token mode only. Session-mode approve/direct-execute status mapping stays with #532.

Known limits, deliberately not changed here:

  • Token validation errors (403/410) apply on the immediate-approval path. The delayed-approval path (purchase delay configured, approveWithDelay) does not validate the token yet; tracked separately.
  • validateRevokeToken (403 invalid, 409 expired) and the RI exchange token path are unchanged, so expired-token codes are inconsistent for now (410 on approve/cancel, 409 on revoke). Alignment is a follow-up.

Test plan

  • New tests through HandleRequest with the real purchase.Manager; they fail on main (500) and pass here.
  • go vet ./..., go vet -tags integration ./internal/..., go test -race -short ./internal/api ./internal/purchase, integration-tagged cancel/approve/revoke tests with Docker.

Closes #564

Summary by CodeRabbit

  • Bug Fixes

    • Approval and cancellation requests now return clearer client errors: invalid tokens receive 403, expired tokens 410, and executions that cannot proceed or change concurrently 409.
    • Revoking a purchase by email now returns 404 when the execution does not exist; other storage errors remain server errors.
  • Documentation

    • Updated API documentation to reflect token validation, status conflicts, and missing-execution responses.

…cution

Invalid approval tokens now answer 403, expired tokens 410, and a non-pending execution in token mode 409 on approve and cancel. Revoke by email answers 404 for a missing execution. Closes #564.
@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged urgency/this-quarter Within the quarter impact/few Limited audience effort/s Hours type/bug Defect labels Oct 6, 2026
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: a899fcc5-aa43-4e52-a681-abf83f890548
📥 Commits

Reviewing files that changed from the base of the PR and between bad3422 and 9f2037e.

📒 Files selected for processing (4)
  • internal/api/handler_purchases.go
  • internal/api/handler_purchases_token_errors_test.go
  • internal/api/openapi.yaml
  • internal/purchase/approvals.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


📝 Walkthrough

Walkthrough

Purchase approval and cancellation handlers now map invalid tokens, expired tokens, and execution-status conflicts to client error responses. Email revocation returns 404 for a missing execution. Handler tests and OpenAPI response descriptions cover these outcomes.

Changes

Purchase token error responses

Layer / File(s) Summary
Classify token and execution errors
internal/purchase/approvals.go
Approval token validation returns sentinel errors for invalid and expired tokens. Cancellation uses the shared validator, and cancellation status errors wrap the expected-status sentinel.
Map purchase errors to HTTP responses
internal/api/handler_purchases.go, internal/api/handler_purchases_token_errors_test.go, internal/api/openapi.yaml
Approval and cancellation handlers map token and status errors to client responses. Email revocation returns 404 for missing executions. Tests check the response mappings, and OpenAPI descriptions document the response codes.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 9f203

The described token-mode response changes have no identified merge-blocking issue. Normal checks remain appropriate.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning [Issue #564] The PR maps invalid approval tokens to 403, expired tokens to 410, execution-status conflicts to 409, and missing email-revoke executions to 404. It adds handler tests and updates the Ope… Validate approval tokens on the delayed-approval path, map invalid and expired token errors to 403 and 410, and add tests for those outcomes.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes two central changes: 4xx responses for invalid approval tokens and 404 responses for missing revoke executions. It does not mention the 409 status-conflict handling, but th…
Out of Scope Changes check ✅ Passed The handler changes, purchase error sentinels, tests, and OpenAPI updates all implement or document the requirements in [Issue #564]. No unrelated changes appear in the reviewed diff.
Full details: Linked Issues check

Explanation

[Issue #564] The PR maps invalid approval tokens to 403, expired tokens to 410, execution-status conflicts to 409, and missing email-revoke executions to 404. It adds handler tests and updates the OpenAPI responses. However, the PR and OpenAPI state that the delayed-approval path (approveWithDelay) does not validate the approval token. An invalid or expired token on that path therefore does not receive the required 403 or 410 response.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Reword a comment the misspell linter flagged, add a test for the cancel CAS-loss mapping, and note in openapi that the delayed-approval path does not validate the token yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(api): invalid or expired approval tokens and a missing revoke execution answer 500 instead of 4xx

1 participant