Auth: consume refresh tokens atomically to close a replay window (closes #147) by testtest126 · Pull Request #148 · testtest126/chess

testtest126 · 2026-07-12T02:40:20Z

Summary

Closes #147 (audit finding M2). Rule-5 security-sensitive (session/token management) — draft pending a security verdict.

refresh() rotated the token with a read-then-delete:

guard let stored = …filter(tokenHash == digest).first() else { 401 }
try await stored.delete(on: req.db)   // deletes by id; 0 rows on the loser, no error
… issueTokens(for: user)

Two requests with the same token (a real client and a replayed stolen token) can both read the row before either deletes, so both mint a valid access+refresh pair — rotation fails to neutralize a captured token.

Fix: a single atomic consume, mirroring the consumeNonce helper already in this file (which solved the identical TOCTOU for Apple nonces):

DELETE FROM refresh_tokens
WHERE token_hash = ? AND expires_at > ?
RETURNING CAST(user_id AS text) AS user_id

Only the racer whose DELETE hits the row gets a user_id and proceeds; the other gets 401. Expired tokens don't match (can't be exchanged; cleanup reaps them). Portable across Postgres and SQLite 3.35+.

Behavior change worth noting for review: an expired token now returns a generic "invalid or expired" 401 instead of a distinct "expired" message, and is left for cleanup rather than deleted on the failed exchange — no test depended on the old distinction.

Test plan

swift test in ChessKit/ passes (untouched)
swift test in chess-server/ passes — server lane is the verifier (this session can't build Swift locally)
App builds (no app code changed)

testRefreshRotatesToken (rotation + sequential-reuse rejection) still holds. The concurrency guarantee is provided by the atomic DELETE … RETURNING — the same mechanism already reviewed and tested for consumeNonce — rather than a new concurrency test, since the existing test harness runs requests sequentially.

Security review

Rule 5 verdict requested from the root session at head — this stays draft until Security review: APPROVE @ <head>.

🤖 Generated with Claude Code

https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T

Generated by Claude Code

#147) refresh() read the token row, then deleted it, then issued new tokens. Two requests with the same token — a real client and a replayed stolen one — could both read the row before either delete (the second delete hits 0 rows without erroring), so both minted a fresh access+refresh pair. Rotation therefore failed to neutralize a captured token. Replaced the SELECT-then-delete with a single atomic DELETE ... WHERE token_hash = ? AND expires_at > ? RETURNING user_id, so only the racer whose delete hits the row proceeds and the other gets 401 — mirroring the consumeNonce pattern already used in this file for the same class of TOCTOU replay. Portable across Postgres and SQLite. The existing testRefreshRotatesToken (rotation + sequential-reuse rejection) still holds. Rule 5 (session management): opens as draft pending a security verdict. Found by the 2026-07-11 full-repo audit (M2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T

testtest126 · 2026-07-12T11:33:32Z

Security review (rule 5) — BLOCK @ 8807f7b7

Classification: security-sensitive — refresh-token rotation / session management.

Fix: correct. The atomic DELETE … RETURNING consume closes the concurrent replay window in #147: two requests presenting the same token can no longer both SELECT-then-issue, because only the transaction whose DELETE hits the row gets a RETURNING result — the loser matches zero rows and gets 401. Folding expires_at > now into the WHERE is right, and mirroring consumeNonce is the correct precedent. No correctness objections.

Blocker — missing regression for the bug class. AuthTests.testRefreshRotatesToken already asserts sequential single-use (replaying the old token after rotation → 401), but the old SELECT-then-delete code passes that test too — it never exercised the concurrent race that #147 is about. Per rule 5, the test must be able to catch the class of bug under review, and this one can't.

Required before APPROVE: a concurrency regression that fires ≥2 simultaneous POST /auth/refresh with the same token (task group / async let) and asserts exactly one 200 and the rest 401, with only one new token pair minted. That test fails against the pre-fix code and passes against this diff.

Staying draft; no Security review: APPROVE posted, no orchestrator-approval status. Head reviewed: 8807f7b7 — a new push voids this verdict.

— orchestrator/reviewer, 2026-07-12

Security review (rule 5) noted testRefreshRotatesToken only covers sequential reuse — the old SELECT-then-delete code passes it too — so it can't catch the concurrent replay that #147 fixes. Added a test that fires 8 simultaneous POST /auth/refresh with the same token and asserts exactly one 200 and the rest 401. Atomic DELETE … RETURNING makes that deterministic here; the pre-fix code let multiple racers mint sessions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T

testtest126 · 2026-07-12T11:38:13Z

Added the required concurrency regression at 75e7f91: testConcurrentRefreshRedeemsTheTokenExactlyOnce fires 8 simultaneous POST /auth/refresh with the same token via a task group and asserts exactly one 200 and seven 401. The atomic DELETE … RETURNING makes "exactly one winner" deterministic here; the pre-fix SELECT-then-delete let multiple racers through. (Application isn't Sendable, so it crosses into the task closures via a small @unchecked Sendable box — Vapor routes each test request independently, which is the concurrency under test.) Ready for re-review; CI is the build/run verifier for this session.

Generated by Claude Code

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T

The concurrency regression from 75e7f91 didn't build: its afterResponse closure was synchronous, so overload resolution picked Application.test's sync variant, which is unavailable from an async context (the enclosing task-group closure). Mark the closure `{ res async in }` — the same disambiguation every other async test in this file uses — so the async overload is selected. Verified the regression now does its job: against the pre-fix SELECT-then-delete code all 8 concurrent refreshes return 200 (0×401), and it fails reliably (5/5 runs); against the atomic DELETE … RETURNING fix exactly one returns 200 and seven return 401 (5/5). Full server suite: 86 tests, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

testtest126 · 2026-07-12T11:59:59Z

Re-review requested @ 5f8b867 — the concurrency regression now builds and bites.

Flagging the head the verdict must land on, because the regression added at 75e7f91 did not compile under the server toolchain (Swift 6.3.3): its afterResponse closure was synchronous, so overload resolution selected Application.test's sync variant — unavailable from asynchronous contexts inside the task-group closure. CI would have gone red; the earlier "ready for re-review" was never build-verified locally (author noted CI was the verifier).

Fixed in 5f8b867 with the one-line disambiguation every other async test in this file already uses:

-                    }, afterResponse: { res in
+                    }, afterResponse: { res async in

Verified empirically that the regression catches the bug class (the rule-5 blocker):

Pre-fix SELECT-then-delete (reverted AuthController.swift to cf20ef7 locally): fails 5/5 — all 8 concurrent refreshes return 200, 0×401. Replay window fully open; every racer mints a pair.
Atomic DELETE … RETURNING fix: passes 5/5 — exactly 1×200, 7×401. A pair is minted only on the 200 path, so "exactly one 200" is "exactly one new token pair minted".
Full server suite at this head: swift test → 86 tests, 0 failures.

Scope note: production code is unchanged since your BLOCK @ 8807f7b7 — git diff 8807f7b 5f8b867 -- …/AuthController.swift is empty (byte-identical). Every commit since the fix is test-only (regression add + lint + this compile fix). The consume logic you already assessed as "correct" hasn't moved.

Staying draft; not self-approving (rule 5). Requesting the rule-5 verdict re-issued at 5f8b867 — Security review: APPROVE @ 5f8b867.

testtest126 · 2026-07-12T12:01:52Z

Thanks for the compile catch and the empirical pre/post verification — the sync afterResponse overload selection was a real gap I couldn't see without building (this session has no Swift toolchain; proxy blocks the download). Author-side confirmation: I'm freezing this branch at 5f8b867 — no further pushes from me, so the head stays stable for the rule-5 verdict. Production code is unchanged since your BLOCK @ 8807f7b7 as you noted. Nothing further needed from my side.

Generated by Claude Code

testtest126 · 2026-07-12T17:39:25Z

Security review: APPROVE @ 5f8b867

Root-agent review (CLAUDE.md rule 5). The atomic DELETE ... RETURNING correctly closes the SELECT-then-delete replay window; parameterized binds (no injection); mirrors the established consumeNonce helper. Verified empirically, not just against CI: built at this head, then reverted the fix while keeping the test and confirmed testConcurrentRefreshRedeemsTheTokenExactlyOnce fails on the old code (8 of 8 concurrent refreshes mint a session) and passes on the fix -- a genuine regression test, not a self-consistent tautology. All lanes green at this head.

claude and others added 2 commits July 12, 2026 11:38

Lint: no space around the range operator (--ranges no-space)

cb1c9db

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T

testtest126 marked this pull request as ready for review July 12, 2026 17:39

testtest126 merged commit e1b441f into main Jul 12, 2026
9 checks passed

testtest126 deleted the claude/smiley-face-1xzfkz-refresh-atomic branch July 12, 2026 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth: consume refresh tokens atomically to close a replay window (closes #147) - #148

Auth: consume refresh tokens atomically to close a replay window (closes #147)#148
testtest126 merged 4 commits into
mainfrom
claude/smiley-face-1xzfkz-refresh-atomic

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

testtest126 commented Jul 12, 2026

Summary

Test plan

Security review

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

testtest126 commented Jul 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants