Skip to content

sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first - #1242

Draft
IAmJSD wants to merge 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1190
Draft

IAmJSD wants to merge 3 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1190

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 22, 2026

Copy link
Copy Markdown
Collaborator

Root cause

  1. Token approve/cancel race on rotation. The immediate email-link/SQS approve (ApproveExecution) and the email-link/SQS cancel (CancelExecution) checked the approval token against a snapshot (ValidateApprovalToken), then moved the row with a plain status CAS (TransitionExecutionStatus to running, and CancelExecutionAtomic). If a re-notification rotated the token (RotatePendingApprovalToken) between the read and the CAS, the old, revoked token still won: it claimed the row and bought, or canceled it. The delayed approve was already pinned by sec(api): delayed token approval validates the execution snapshot, not the token hash at the status change #590 (TransitionExecutionStatusTokenPinned); these two paths were not.
  2. validateRevokeToken revealed expiry to a wrong token. It checked the link's expiry and the revocation window before comparing the hash. A wrong token on a lapsed row got "revocation link has expired" (409) or "revocation window has closed" (403) instead of "invalid revocation token".

Fix

  • config: new CancelExecutionAtomicTokenPinned. It is CancelExecutionAtomic with approval_token_hash = hash(token) AND (expires_at IS NULL OR expires_at > NOW()) in the same UPDATE as the status change. A lost pin on a row that is still pending/notified returns ErrApprovalTokenChanged. A lost status race still returns (false, currentStatus, nil). The unpinned CancelExecutionAtomic (used by the session cancel and the revoke paths) is unchanged: the new method shares its body and adds a NULL-able token parameter.
  • purchase: ApproveExecution now claims through TransitionExecutionStatusTokenPinned(..., "running", ..., token) via claimRunningForApprove. CancelExecution now cancels through CancelExecutionAtomicTokenPinned. Session approve, direct execute and run-now pass no token and are unchanged.
  • api: tokenActionError maps config.ErrApprovalTokenChanged to 403 "invalid approval token" (it was a 500), matching what approveWithDelay already returns. validateRevokeToken now compares the hash first, then checks expiry, then the window.

Concurrency: under READ COMMITTED, a pinned UPDATE that waits behind an uncommitted rotation re-evaluates its WHERE against the rotated row and matches nothing. The rotation's own WHERE status IN ('pending','notified') refuses a row the action has already moved. So exactly one of the two can win. Both orderings and the blocked-on-lock case are tested on real PostgreSQL (below).

No migration.

Closes #1190

Verification

All of this is local, on macOS, with Docker testcontainers PostgreSQL 16 (real DB) plus mock/fixture providers and email. Nothing calls a cloud API. 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 (on origin/main c8bdd46): I copied the new tests that only use APIs already on main into a detached worktree of main and ran them there.

  • go test -tags integration ./internal/purchase/ -run 'TestApproveExecution_TokenRotatedBeforeClaimIsRefused|TestCancelExecution_TokenRotatedBeforeCancelIsRefused|TestCancelExecution_RacesRotation':
    • approve: Expected error with "approval token no longer matches the execution" in chain but got nil. the revoked token must lose the claim. On main the revoked token claimed the row and ran it. The positive control passes on both.
    • cancel: ... but got nil. the revoked token must not cancel
    • race: round 1: exactly one of the cancel (err=<nil>) and the rotation (true) may win, meaning both succeeded.
  • go test ./internal/api/ -run TestHandleRequest_RevokeViaEmail_WrongTokenOnLapsedRowIsInvalid: wrong token on an expired link expected: 403 actual: 409. Wrong token past the window was also refused with "window has closed" instead of invalid. The right-token cases pass on both.

Pass after (this branch):

  • go test -tags integration ./internal/config/ -count=1: ok. This is the whole package, including the new TestPostgresStoreDB_CancelExecutionAtomicTokenPinned, TestPostgresStoreDB_TokenPinnedActionBlockedBehindRotationLoses (cancel and claim-to-running each block on the row lock behind an uncommitted rotation and must lose; the test detects the wait through pg_stat_activity) and TestPostgresStoreDB_TokenPinnedActionRacesRotation (40 unsynchronized rounds per action with staggered starts; the test asserts both orderings happened, e.g. cancel: action won 18, rotation won 22).
  • go test -tags integration ./internal/purchase/ -count=1: ok (195s, whole package).
  • go test ./internal/api/ ./internal/purchase/ ./internal/config/ ./internal/mocks/ ./internal/server/ ./internal/analytics/ -count=1: all ok.
  • Mutation probes on this branch, each reverted afterwards:
    • store pins ignored (token hash passed as NULL): the config pinned-cancel, blocked-on-lock and race tests fail.
    • claimRunningForApprove always unpinned and CancelExecution back on CancelExecutionAtomic: the purchase unit tests panic on the unregistered unpinned call, and the integration tests fail.
    • ErrApprovalTokenChanged mapping removed: TestHandleRequest_TokenActions_TokenRotatedBeforeCASIs403 fails (500 instead of 403).
  • go build ./..., go vet ./..., go vet -tags integration ./internal/... ./cmd/...: clean.
  • golangci-lint v2.10.1 --build-tags integration (and without the tag) on the touched packages: 0 issues. gocyclo -over 10 -ignore "_test\.go" on the touched packages: silent. scripts/gosec-hook.sh on the changed files: clean. gofmt -l: clean.
  • git-secrets 1.3.0 --register-aws, in a throwaway repo with an isolated HOME, with the changed files plus .gitallowed tracked: clean, and a planted AKIA key in the same repo was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.

Test changes worth a look

  • The existing mock tests of the token approve/cancel paths (approvals_test.go, coverage_extra_test.go, claimed_row_check_test.go, aws_no_token_redrive_test.go, messages_token_ttl_test.go, handler_purchases_token_errors_test.go, handler_purchases_cancel_constraints_test.go) now register the pinned store calls. The new CancelExecutionAtomicTokenPinned mock has no default and does not delegate, the same as the sec(api): delayed token approval validates the execution snapshot, not the token hash at the status change #590 TransitionExecutionStatusTokenPinned mock, so a revert to the unpinned call fails loudly. "Not called" guards on the token path now name the pinned method.
  • TestHandler_revokePurchase_ExpiredToken stored the raw token as ApprovalToken instead of its hash, so its "matching" token never matched. It passed only because expiry was checked first. The fixture now stores the hash.
  • TestPGXMock_CancelExecutionAtomic_WritesCanonicalStatus expects the new 4th SQL arg (NULL for the unpinned cancel).

Out of scope / notes

  • After a successful token cancel, clearApprovalToken still clears the token with a full-row SavePurchaseExecution of a re-read row. That is pre-existing and touches only an already-canceled row. Not changed here.
  • revokeViaSession moves completed to revocation_requested with a plain CAS after validateRevokeToken's snapshot check. Nothing rotates the token of a completed row today, so there is no rotation race to pin there. The issue does not name it.
  • On the token approve, a token that expires between the snapshot check and the CAS now gets 403 "invalid approval token" (lost pin) rather than 410. The delayed approve already behaves this way.
  • Known unrelated CI failure: govulncheck (GO-2026-6809) until fix(deps): bump Go to 1.26.10 for GO-2026-6809 #1236 merges.

No frontend changes.

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/security, triaged.

🤖 Generated with Claude Code

IAmJSD and others added 3 commits October 22, 2026 02:22
CancelExecutionAtomicTokenPinned is CancelExecutionAtomic with the approval
token checked in the same UPDATE as the status change: the stored hash must
still match the caller's token and be unexpired. A token rotated by a
re-notification after the caller's snapshot check now loses the cancel with
ErrApprovalTokenChanged instead of canceling the row (issue LeanerCloud#1190), the
cancel-side twin of the LeanerCloud#590 token-pinned transition.

Real-PostgreSQL tests cover the pinned cancel's outcomes, a pinned action
blocked on the row lock behind a committed rotation, and an unsynchronized
rotation race for both the pinned cancel and the pinned claim to running.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…token cancel

The immediate email-link/SQS approve (ApproveExecution) and the email-link/SQS
cancel (CancelExecution) validated the token against a snapshot, then moved
the row with a plain status CAS. A re-notification rotating the token between
the two let the old, revoked token still claim the row to running (and buy)
or cancel it (issue LeanerCloud#1190, the LeanerCloud#590 class the delayed approve already closed).

ApproveExecution now claims through TransitionExecutionStatusTokenPinned and
CancelExecution cancels through CancelExecutionAtomicTokenPinned, so the token
is re-checked in the same UPDATE as the status change. The session, direct
execute and run-now paths are unchanged (not token-gated). The HTTP layer maps
the lost pin (config.ErrApprovalTokenChanged) to 403 "invalid approval token"
instead of a 500, as the delayed approve already did.

Real-PostgreSQL tests rotate the token between ApproveExecution's/
CancelExecution's snapshot read and their CAS and require the old token to be
refused (both fail on main), plus a cancel-vs-rotation race. Existing mock
tests of the token paths now register the pinned store calls.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d window

validateRevokeToken checked the link's expiry and the revocation window
before comparing the token hash, so a wrong token on a lapsed row got
"revocation link has expired" (409) or "revocation window has closed" (403)
instead of "invalid revocation token", telling a token guesser the link
exists and has lapsed (issue LeanerCloud#1190). The hash is now compared first, as the
approve path's ValidateApprovalToken does.

TestHandler_revokePurchase_ExpiredToken stored the raw token instead of its
hash, so its "matching" token never matched; it passed only because expiry
was checked first. The fixture now stores the hash.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

This branch has not been deployed

No deployments
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/security Security finding urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec(purchase): token approve/cancel race on rotation, and revoke reveals expiry to a wrong token

1 participant