Repository navigation
fix(api): answer expired and lost-race token actions alike on revoke and RI exchange - #1172
Conversation
…and RI exchange Approve and cancel already answer an emailed-token action with 403 for an invalid token, 410 for an expired one and 409 for a wrong status. Revoke answered an expired link with 409, and read the expiry before matching the token. The RI exchange approve and reject links never checked expires_at: the transition CAS refused an expired or already-moved record with a plain error, so both came back as 500. - validateRevokeToken reuses purchase.ValidateApprovalToken: token matched first, then 410 for an expired link, then the revocation window (403). - checkExchangeToken gives RI approve and reject the same 403/410 order. - TransitionRIExchangeStatus wraps ErrRIExchangeExpired and ErrRIExchangeNotInExpectedStatus; riExchangeTransitionError maps them to 410 and 409 on the approve (token and session) and reject paths. - openapi.yaml documents the 410s and the 409 race. Closes LeanerCloud#587 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
VERDICT: APPROVE fea837d PR #1172 (Closes #587). One commit, 370 changed lines (cap 400: OK). Rebase check: Findings
Oracle / semantics check
Local results
Attacked (mutation probes, each restored)
|
|
Independent review 2 (claude-fleet, cold): APPROVE fea837d Findings (none blocking):
Against #587:
Attacked:
Verified locally, on a
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
From earlier reviews (confirmed, non-blocking):
Reviewer: rB24. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
Conflict in validateRevokeToken: LeanerCloud#1242 (on main) moved the token match ahead of the expiry and window checks but kept 409 for an expired link; this branch routes revoke through purchase.ValidateApprovalToken for the 403/410 mapping. Kept this branch's switch, which already matches the token (constant time, against the stored digest) before the expiry and the window, and merged the two doc comments. LeanerCloud#1242's test for that order (TestHandleRequest_RevokeViaEmail_WrongTokenOnLapsedRowIsInvalid) now expects 410 for the right token on an expired link, matching approve and cancel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e 409 text Review follow-ups on LeanerCloud#1172. riExchangeTransitionError turns the store's error, which names the exchange and its current status, into a bare ClientError, and handleRequestError never logs ClientErrors, so a lost approve/reject race or a mid-flight expiry left no server log line. Log the store error at info level in the 409 and 410 branches. Expiry is answered with 410 now, so the 409 conflict messages no longer list "expired" as a cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged current main (c8bb869) into the branch, in two merge commits: c1919cc, then c85ef50 for #1426. I also added one commit with review follow-ups. New head: Conflict:
cristim's review nits (new commit ff88e48):
Checks on the merged tree, with no cloud credentials:
|
Conflicts in internal/api/handler_purchases.go and its test come only from LeanerCloud#1172's commit, which this branch is stacked on; this branch does not touch those files. Took LeanerCloud#1172's resolution of the same merge (c1919cc): its 403/410 validateRevokeToken switch, which keeps LeanerCloud#1242's token-first order, and LeanerCloud#1242's order test expecting 410 for an expired link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bring in LeanerCloud#1172's new head (main merge plus its review follow-ups) so the two stacked PRs agree. The one conflict is the RI approve/reject status table: kept this branch's LeanerCloud#601 rows (missing record answers 403 like a wrong token in token mode) with LeanerCloud#1172's 409 text, which no longer lists "expired". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rC5. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rC6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on |
LeanerCloud#1172, which this branch carries, merged as squash 89e1140. For both conflicted files (internal/api/handler_ri_exchange.go and ri_exchange_approve_reject_status_test.go), main's content is byte-identical to LeanerCloud#1172's head c85ef50 and hasn't changed since, so the conflicts are only this branch's own additions on top: kept this branch's side. The diff against main is now only LeanerCloud#1175's own change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and reject (#1175) * fix(api): answer expired and lost-race token actions alike on revoke and RI exchange Approve and cancel already answer an emailed-token action with 403 for an invalid token, 410 for an expired one and 409 for a wrong status. Revoke answered an expired link with 409, and read the expiry before matching the token. The RI exchange approve and reject links never checked expires_at: the transition CAS refused an expired or already-moved record with a plain error, so both came back as 500. - validateRevokeToken reuses purchase.ValidateApprovalToken: token matched first, then 410 for an expired link, then the revocation window (403). - checkExchangeToken gives RI approve and reject the same 403/410 order. - TransitionRIExchangeStatus wraps ErrRIExchangeExpired and ErrRIExchangeNotInExpectedStatus; riExchangeTransitionError maps them to 410 and 409 on the approve (token and session) and reject paths. - openapi.yaml documents the 410s and the 409 race. Closes #587 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(api): answer a missing RI exchange like a wrong token on approve and reject The RI exchange approve and reject email links are open to callers with no session, since the token alone authorizes them. They answered a wrong token on an existing exchange with 403 but a missing exchange id with 404 (with the in-memory mocks) or 500 (on Postgres, whose GetRIExchangeRecord returned a bare error for no row), so anyone could probe which exchange ids exist. A 401 before the lookup, the purchase routes' fix (#435), does not fit: it would break the token-only email flow. Instead token mode now gives one answer to every caller who does not hold the token: - checkExchangeToken treats a missing record and a record without a token like a wrong one: 403 "invalid approval token" / "invalid rejection token". The #587 order is unchanged: the token match comes first, then 410 for an expired exchange, then the transition's 409/410. - PostgresStore.GetRIExchangeRecord wraps ErrNotFound for no row. getRIExchangeRecord maps it (or a nil record) to "missing", so the session approve path answers a missing exchange with the documented 404 instead of 500. Other store errors stay 500. - openapi.yaml: 404 is session mode only on approve and is gone from reject; the 403s say an unknown id answers like a wrong token. Closes #601 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(api): log lost RI exchange transitions and drop "expired" from the 409 text Review follow-ups on #1172. riExchangeTransitionError turns the store's error, which names the exchange and its current status, into a bare ClientError, and handleRequestError never logs ClientErrors, so a lost approve/reject race or a mid-flight expiry left no server log line. Log the store error at info level in the 409 and 410 branches. Expiry is answered with 410 now, so the 409 conflict messages no longer list "expired" as a cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Decision this PR proposes (owner call)
The issue asks for one mapping for every emailed-token action and leaves 409 vs 410 for an expired token to the owner. This PR picks 410, the code approve and cancel already answer since #584 (and the one the frontend's
approval-details.tsalready reads as "the approval link has expired"). That way only revoke and RI exchange change, not approve and cancel. If you'd rather have 409, it's a few constants and test expectations.Root cause
validateRevokeTokenhad its own copy of the checks. It answered an expired link with 409, and it read the expiry before matching the token, so a wrong token on an expired link also got the 409 "expired" answer.validateExchangeApprovalandrejectRIExchangenever readexpires_at. Expiry and status were enforced only by theTransitionRIExchangeStatusCAS. When the CAS matched no row,diagnoseTransitionFailurereturned a plainfmt.Errorf, which the handler wrapped as a 500. The handler'stransitioned == nil → 409branch only runs against mocks, because the Postgres store never returns(nil, nil). So on a real database, both an expired exchange and one a concurrent approve or reject had already moved answered 500.Fix
internal/api/handler_purchases.go:validateRevokeTokennow callspurchase.ValidateApprovalToken, the same check approve and cancel use. It answers 403invalid revocation token, or 410 when the token matches but has expired, then checks the 24-hour revocation window (still 403). This runs afterauthorizeApprovalActionand its fix(api): answer a hidden execution like a missing one on approve, cancel and revoke #935 concealment, as before. The order of the denial checks is unchanged.internal/config: new sentinelsErrRIExchangeExpiredandErrRIExchangeNotInExpectedStatus, whichdiagnoseTransitionFailurenow wraps. The message text is unchanged.internal/api/handler_ri_exchange.go:checkExchangeTokengives approve and reject the same order: no stored token or a wrong token is 403, and a matching token on an exchange pastexpires_atis 410. This check runs before any write, and on approve before the 4-eyes check.riExchangeTransitionErrormaps a lost CAS to 410 (expired in between) or 409 (another writer won). It covers the token approve, the session approve and the reject transition. Any other store error stays a 500.internal/api/openapi.yaml: adds 410 to revoke, RI approve and RI reject, and rewords their 409s.Concurrency
The only DB-side change is wrapping errors. No SQL changed. The race that matters is several approves and rejects on one pending exchange. Postgres serializes them on the row lock. The losers'
UPDATE ... WHERE status = 'pending'matches nothing, and the diagnoseSELECTsees the winner's committed status. A new integration test runs that race on real Postgres: 5 rounds of 8 concurrent callers, approve and reject mixed. In every round exactly one wins, every loser getsErrRIExchangeNotInExpectedStatus(409), none gets the expiry sentinel, and the row ends in the winner's status. The handler tests cover the other window: an exchange that expires between the token check and the CAS gets 410.Verification
Everything below is unit, mock and fixture evidence, plus a store-level integration test on a local Postgres testcontainer. No cloud APIs, purchases or deploys. I didn't drive the RI exchange handler against real Postgres end to end. The handler-to-status mapping is covered with the mock store returning the same wrapped sentinels the real store now returns, and the store side is covered by the integration test.
Tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.Fail-before / pass-after. I restored
main'shandler_ri_exchange.go,handler_purchases.goandstore_postgres.goand kept the new tests. The new sentinel declarations stayed so the tests compile; they change no behavior.TestHandleRequest_ApproveRIExchange_StatusCodes: 6 new subtests FAIL. They are: expired → 410, 410 before the 4-eyes check, expired mid-flight → 410, and lost race → 409, for both token and session. On main these came back 200 or 500.TestHandleRequest_RejectRIExchange_StatusCodes: 3 new subtests FAIL (expired, expired mid-flight, lost race).TestHandleRequest_TokenActions_TokenAndStatusErrorsAre4xx:revoke_expired_token(409) andrevoke_wrong_token_on_an_expired_link(409) FAIL.TestHandler_revokePurchase_ExpiredTokenFAILS (409).TestPGXMock_TransitionRIExchangeStatus_{WrongStatus,Expired}FAIL (no sentinel in the chain).TestPostgresStoreDB_TransitionRIExchangeStatus_*tests FAIL ("Target error should be in err chain").Fixture change.
TestHandler_revokePurchase_ExpiredTokenstored the raw token where the hash belongs. It only passed because expiry was checked before the match. It now storesconfig.HashApprovalToken(token)and expects 410.Behavior changes reviewers should weigh
POST /api/ri-exchange/approve/{id}from the dashboard (session mode,frontend/src/api/riexchange.ts) now gets 409 or 410 instead of 500 when an exchange was already handled or has expired. The frontend shows the error message generically and has no 500-specific handling. @cristim: no frontend change was made. If the RI exchange page should treat 410 differently (for example, refresh the row), that's a UI follow-up.authorizeApprovalActionandconcealExecutionDenialstill run before any token check, so only an authorized contact email ever sees 403, 410 or 409. The signed-in oracle test passes unchanged.Out of scope (noted, not fixed)
GetRIExchangeRecordon Postgres returns an error, not(nil, nil), for a missing id. So RI approve and reject answer a missing exchange with 500 in production, whileopenapi.yamland the mock-based tests say 404.approveViaToken's validation and the token-pinned CAS, the result isErrApprovalTokenChanged, which answers 403, not 410. The store can't tell rotated from expired there without a wider store change.openapi.yamlstill list RBAC and contact-email denials that fix(api): answer a hidden execution like a missing one on approve, cancel and revoke #935 now answers as 404.Closes #587
Maintainer: please mirror the issue's triage labels on this PR:
triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/s,type/bug.🤖 Generated with Claude Code