[IAmJSD]

Root cause

When a purchase delay is configured, approveViaToken checks the email-link token against the execution it loaded at the start of the request (purchase.ValidateApprovalToken, added for #586). Then approveWithDelay → scheduleApprovedExecution schedules the row with TransitionExecutionStatus, and that CAS filters on status only. If a re-notification (RotatePendingApprovalToken) rotated the token, or the token expired, between the load and the CAS, the old token could still schedule the purchase. The immediate path doesn't have this gap.

Fix

• New store method TransitionExecutionStatusTokenPinned (in config.StoreInterface, implemented in PostgresStore). It adds approval_token_hash = sha256(token) AND (approval_token_expires_at IS NULL OR approval_token_expires_at > NOW()) to the same UPDATE … WHERE status = ANY(...), so the token is checked against the row inside the transition.
• When that update matches no row and the row is still in an allowed status, the token pin is what rejected it. In that case the store returns the new sentinel config.ErrApprovalTokenChanged. A lost status CAS still returns ErrExecutionNotInExpectedStatus, and a probe DB error still propagates. The zero-row probe moved into transitionRejection to keep gocyclo under 10. An empty token never matches.
• scheduleApprovedExecution / approveWithDelay now take the raw token. The email-link path passes it and uses the pinned CAS. The session path passes "" and keeps the plain status CAS. ErrApprovalTokenChanged maps to 403 "invalid approval token", the same answer as a wrong token, and the row is neither scheduled nor stamped.
• The pre-check against the snapshot stays in place, so an expired token still gets 410. If the token expires in the few milliseconds between the pre-check and the CAS, the answer is 403.

Closes #590

Verification

All evidence comes from unit, mock and local-Postgres tests. Nothing calls a cloud API or makes a purchase. Unit runs used AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because the default AWS profile on this machine is invalid.

New and updated tests:

• Real Postgres (testcontainers): TestPostgresStoreDB_TransitionExecutionStatusTokenPinned in internal/config/store_postgres_db_test.go.
  • Rejected paths:
    • The caller's snapshot validates the old token, then RotatePendingApprovalToken rotates it, then the pinned CAS with the old token returns ErrApprovalTokenChanged and the row stays notified.
    • An expired token is rejected the same way.
    • A wrong token and an empty token are rejected.
    • A row with no stored token is rejected.
  • Accepted path: the new token schedules the row.
  • Replay: replaying the token on a row that is already scheduled returns ErrExecutionNotInExpectedStatus, not the token error.
• Handler (mock): in internal/api/handler_purchases_delay_token_test.go:
  • New TokenRotatedBeforeCASIs403: the snapshot holds a valid token and the pinned CAS reports a rotation. The handler answers 403 and never calls SavePurchaseExecution or the unpinned CAS.
  • ValidTokenSchedules now requires the pinned CAS to receive the supplied raw token, and the unpinned CAS must not be called.
  • The replay and invalid-token cases were updated to match.
• store_postgres_pgxmock_test.go: the arg count for the transition query went from 6 to 7.

Fail-before / pass-after, shown by reverting each part of the fix (mutation probes):

Mutation	Result
SQL token predicate replaced by TRUE (main's behavior)	DB test fails: rotated, expired, wrong/empty and no-stored-token subtests
Handler always uses the unpinned TransitionExecutionStatus (main's behavior)	ValidTokenSchedules fails (unmocked unpinned call)
ErrApprovalTokenChanged not mapped in approveWithDelay	TokenRotatedBeforeCASIs403 fails: expected 403, actual 500
Probe never reports the token change	DB test fails on all 4 rejected subtests

Commands (all pass with the fix):

go build ./... && go vet ./... && go vet -tags integration ./internal/config
go test ./... -count=1
go test -short -race ./internal/api/... ./internal/config/... ./internal/purchase/... ./internal/server/... ./internal/analytics/... ./internal/mocks/... -count=1
go test -tags integration ./internal/config/ -count=1          # full config integration suite, real Postgres, ok (597s)
golangci-lint v2.10.1 run ./internal/{config,api,mocks,server,analytics}/...   # 0 issues (also with --build-tags integration)
gocyclo -over 10 internal/api/handler_purchases.go internal/config/store_postgres.go internal/config/errors.go internal/config/interfaces.go internal/mocks/stores.go   # silent
scripts/gofmt-hook.sh, make tidy-check, scripts/gosec-hook.sh, git diff --check   # clean

Overlap with #935 (fix/issue-597)

I did a trial merge of this branch (rebased on current main, 2f41346) with fork/fix/issue-597 on a throwaway branch. handler_purchases.go and handler_purchases_test.go auto-merged with no conflicts. On the merged result, go build ./..., go vet ./..., go test ./... -count=1, the delayed-approval handler tests, the token-pinned real-Postgres test, golangci-lint and gocyclo all pass. This PR leaves #935's code alone: the CSRF check, the 404 concealment and the orphan check.

Out of scope / not done

• The optional step of clearing approval_token_hash at scheduling time is not done here. Replay of a scheduled row is already blocked by the status CAS, and clearing the hash would also need a review of the SavePurchaseExecution full-row write that follows the CAS.
• The real-Postgres coverage is at the store level (the SQL predicate that does the work). There is no end-to-end real-Postgres test of the HTTP route, which still uses mocks for the handler layer.
• The same snapshot-then-CAS pattern may exist on other token paths (cancel, revoke). I did not check them here.

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

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4fefa249-4365-44d7-9553-b8bc930f9514

📥 Commits

Reviewing files that changed from the base of the PR and between 2f41346 and d11b9c1.

📒 Files selected for processing: 11

• internal/analytics/collector_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_delay_token_test.go
• internal/api/handler_purchases_test.go
• internal/config/errors.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/config/store_postgres_db_test.go
• internal/config/store_postgres_pgxmock_test.go
• internal/mocks/stores.go
• internal/server/test_helpers_test.go

+-----------------------------------------------------+
| Code Wars Episode III: Revenge of the Shared State. |
+-----------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Independent review of d11b9c1: draft: blocks ready

No BLOCKING findings.

NON-BLOCKING

• internal/api/handler_purchases.go:1113: the pinned-vs-unpinned CAS is chosen by approvalToken != "". Today the token path cannot reach it with an empty token (purchase.ValidateApprovalToken, approvals.go:125, rejects an empty token first), so there is no fail-open now. But a future refactor that drops the pre-check would silently fall back to the unpinned CAS. Consider an explicit bool or two named call sites. Follow-up only.
• A token that expires in the milliseconds between the pre-check and the CAS now answers 403 instead of 410 (stated in the PR body). Harmless.

What I attacked

• Replay: after a successful pinned CAS the row is scheduled; replaying the same token passes the snapshot pre-check but the CAS fails on status and transitionRejection (store_postgres.go, new helper) returns ErrExecutionNotInExpectedStatus (409), not the token error. Covered by the PR's real-Postgres test.
• Rotated or expired token: the SQL predicate (approval_token_hash = $7 AND (expires IS NULL OR expires > NOW()), store_postgres.go:1246) is evaluated in the same UPDATE as the status change, so rotation between snapshot and CAS cannot win. A NULL stored hash makes the predicate NULL (no match), and an empty token is refused before SQL (store_postgres.go, TokenPinned). The hash is HashApprovalToken (sha256 hex), the same function used to store tokens (approval_token.go:20).
• Concurrent approve vs cancel: both go through status-CAS on status = ANY(pending, notified); the loser gets ErrExecutionNotInExpectedStatus (409). The probe cannot misreport a cancel as a token error because the token error needs the row to still be in an allowed status. Race window inside the probe: the status may change between UPDATE and probe; that yields at worst a 409 or 403, never a schedule.
• Post-CAS write: SavePurchaseExecution upserts the row returned by the CAS (fresh), so a rotation after the CAS cannot overwrite the new hash with a stale one. Session path passes "" and is unchanged (only two callers of approveWithDelay: handler_purchases.go:773 token, :862 session).
• Old tokens invalidated: the token is invalidated for approval by rotation (RotatePendingApprovalToken) and by the status leaving pending/notified; the hash is retained after scheduling, which is pre-existing and harmless since status gates it.
• Real Postgres (testcontainers, postgres:16-alpine): go test -tags integration ./internal/config -run TestPostgresStoreDB_TransitionExecutionStatusTokenPinned passes at the head (5 subtests). Unit: go test ./internal/api ./internal/config ./internal/purchase passes (4655 tests).
• Mutation probes on the SQL (real Postgres), reverted after each: (1) token predicate replaced by TRUE: 4 subtests FAIL (rotated, expired, wrong/empty, no stored token); (2) expiry clause removed: the expired subtest FAILs. This is fail-before behavior equal to main's status-only CAS.
• Scratch: testcontainers containers are removed by ryuk; I started no other databases.

Gate status (origin/main Review gate)

• Independent review: this comment, against the SHA above.
• CI on the SHA: all checks pass (Snyk skipped; pre-commit and CI - Build & Test success); mergeStateStatus CLEAN. CodeRabbit skipped (draft); no threads.
• Labels: match issue #590 (effort/s, impact/few, priority/p3, severity/low, triaged, type/bug, urgency/eventually).
• Local verification: real-Postgres integration test for the affected CAS plus handler mocks. No real email-link end-to-end run; no cloud calls.

Blocks ready

• Token pinned inside the same UPDATE as the status change; errors distinguish token loss from status loss.
• Fail-before/pass-after demonstrated on real Postgres by mutation of the SQL predicate.
• CI green; labels mirrored.

[cristim]

Gate record for d11b9c1 (origin/main CLAUDE.md "Review gate"):

• Independent review of d11b9c1: no blocking findings.
  • The approval token is now checked in the same SQL UPDATE as the delayed-approval status change, which closes the replay and race window.
  • Non-blocking: handler_purchases.go:1113 selects the pinned vs unpinned CAS with approvalToken != "". This is safe today because the pre-check rejects an empty token; an explicit flag would be clearer.
• CI on d11b9c1: CI Success and Run pre-commit hooks pass.
• Local verification (macOS, testcontainers Postgres):
  • The new test passes.
  • Mutating the SQL token predicate to TRUE fails 4 subtests.
  • Removing the expiry clause fails the expired-token subtest.
  • No real purchase.
• CR skipped: draft and quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(api): delayed token approval validates the execution snapshot, not the token hash at the status change #590.

Marking ready (owner-authorized) and merging at d11b9c1 with --match-head-commit.

[IAmJSD]

Addressed the first non-blocking note at d3b1d18, one commit on top of the reviewed d11b9c1. No rebase.

Pinned vs unpinned CAS chosen by approvalToken != "". approveWithDelay and scheduleApprovedExecution now take an explicit approvalGate, built by tokenApproval(token) or sessionApproval(), instead of a bare token string. The pinned CAS is selected by gate.tokenPinned, so an email-link approval that somehow carries an empty token still goes to TransitionExecutionStatusTokenPinned, which refuses it. It no longer falls back to the status-only CAS.

• Call sites: handler_purchases.go:773 (token path) uses tokenApproval(token) and :862 (session path) uses sessionApproval(). The three existing session-path tests were updated to match.
• New test: TestScheduleApprovedExecution_EmptyTokenOnTokenPathStaysPinned. Fail-before: with selection restored to gate.token != "", the unpinned CAS is called and the test fails (testify panics on the unexpected call). It passes at d3b1d18.

403 vs 410 on expiry in the race window: left as is, as stated in the PR body.

Checks on macOS, with AWS config and credentials set to /dev/null:

• go build ./... passed and go vet ./internal/api/ was clean.
• go test ./internal/api/ ./internal/server/ ./internal/config/ ./internal/purchase/ -count=1: ok.
• golangci-lint v2.10.1 reports 0 issues, gocyclo -over 10 is silent and git secrets --scan is clean.

These are unit tests with mocks. The SQL predicate is unchanged, so the real-Postgres integration evidence from the review still applies.

🤖 Generated with Claude Code