From 8807f7b70f0907519f72e55ae6d66510693d6643 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 02:40:02 +0000 Subject: [PATCH 1/4] Auth: consume refresh tokens atomically to close a replay window (closes #147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- .../Sources/App/Auth/AuthController.swift | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/chess-server/Sources/App/Auth/AuthController.swift b/chess-server/Sources/App/Auth/AuthController.swift index 17a1f23..197ec00 100644 --- a/chess-server/Sources/App/Auth/AuthController.swift +++ b/chess-server/Sources/App/Auth/AuthController.swift @@ -57,18 +57,27 @@ struct AuthController: RouteCollection { let body = try req.content.decode(RefreshRequest.self, as: .json) let digest = RefreshToken.hash(body.refreshToken) - guard let stored = try await RefreshToken.query(on: req.db) - .filter(\.$tokenHash == digest) - .first() - else { - throw Abort(.unauthorized, reason: "invalid refresh token") + // Consume the token atomically: one DELETE … RETURNING, so two + // requests presenting the same token (a real client and a replayed + // stolen token) can never both mint a fresh pair — only the racer + // whose DELETE hits the row proceeds; the other gets 401. A + // SELECT-then-delete here was a replay window (#147). Expired tokens + // don't match, so they can't be exchanged either; cleanup reaps them. + // Mirrors `consumeNonce`; portable across Postgres and SQLite 3.35+. + guard let sql = req.db as? SQLDatabase else { + throw Abort(.internalServerError, reason: "refresh token store requires an SQL database") } + struct Consumed: Decodable { let user_id: String } + let consumed = try await sql.raw(""" + DELETE FROM refresh_tokens + WHERE token_hash = \(bind: digest) AND expires_at > \(bind: Date()) + RETURNING CAST(user_id AS text) AS user_id + """).first(decoding: Consumed.self) - try await stored.delete(on: req.db) - guard stored.expiresAt > Date() else { - throw Abort(.unauthorized, reason: "refresh token expired") + guard let consumed, let userID = UUID(uuidString: consumed.user_id) else { + throw Abort(.unauthorized, reason: "invalid or expired refresh token") } - guard let user = try await User.find(stored.$user.id, on: req.db) else { + guard let user = try await User.find(userID, on: req.db) else { throw Abort(.unauthorized, reason: "unknown user") } return try await issueTokens(for: user, on: req) From 75e7f9191f886f806797ce1c27c148e36bca8f45 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 11:37:51 +0000 Subject: [PATCH 2/4] Auth: add a concurrent-refresh regression for the replay race (#147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- chess-server/Tests/AppTests/AuthTests.swift | 42 +++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/chess-server/Tests/AppTests/AuthTests.swift b/chess-server/Tests/AppTests/AuthTests.swift index 800e324..934d6cc 100644 --- a/chess-server/Tests/AppTests/AuthTests.swift +++ b/chess-server/Tests/AppTests/AuthTests.swift @@ -80,6 +80,42 @@ final class AuthTests: XCTestCase { }) } + func testConcurrentRefreshRedeemsTheTokenExactlyOnce() async throws { + let auth = try await register() + // Application isn't Sendable; a box lets it cross into the concurrent + // task closures. Safe here — Vapor routes each test request + // independently, which is exactly the concurrency under test. + let box = SendableBox(value: app!) + let token = auth.refreshToken + + // Fire several simultaneous refreshes of the SAME token. Rotation is + // atomic (DELETE … RETURNING), so exactly one may redeem it — a + // replayed stolen token and the real client cannot both mint a session + // (#147). This is the concurrent race testRefreshRotatesToken can't + // reach: the old SELECT-then-delete let multiple racers both through. + let codes = try await withThrowingTaskGroup(of: UInt.self) { group in + for _ in 0 ..< 8 { + group.addTask { + var code: UInt = 0 + try await box.value.test(.POST, "auth/refresh", beforeRequest: { req in + try req.content.encode(RefreshRequest(refreshToken: token), as: .json) + }, afterResponse: { res in + code = res.status.code + }) + return code + } + } + var all: [UInt] = [] + for try await code in group { all.append(code) } + return all + } + + XCTAssertEqual(codes.filter { $0 == 200 }.count, 1, + "exactly one concurrent refresh may redeem the token") + XCTAssertEqual(codes.filter { $0 == 401 }.count, 7, + "every other concurrent refresh is rejected") + } + func testProtectedRoutesRejectAnonymous() async throws { try await app.test(.GET, "me", afterResponse: { res async in XCTAssertEqual(res.status, .unauthorized) @@ -401,3 +437,9 @@ final class AuthTests: XCTestCase { XCTAssertEqual(holders, 1) } } + +/// Carries a non-Sendable value into concurrent task closures for the +/// refresh-race test. +private struct SendableBox: @unchecked Sendable { + let value: T +} From cb1c9dbf53efdeac79877335e59d05fd1e62f710 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 11:38:52 +0000 Subject: [PATCH 3/4] Lint: no space around the range operator (--ranges no-space) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- chess-server/Tests/AppTests/AuthTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chess-server/Tests/AppTests/AuthTests.swift b/chess-server/Tests/AppTests/AuthTests.swift index 934d6cc..edb82aa 100644 --- a/chess-server/Tests/AppTests/AuthTests.swift +++ b/chess-server/Tests/AppTests/AuthTests.swift @@ -94,7 +94,7 @@ final class AuthTests: XCTestCase { // (#147). This is the concurrent race testRefreshRotatesToken can't // reach: the old SELECT-then-delete let multiple racers both through. let codes = try await withThrowingTaskGroup(of: UInt.self) { group in - for _ in 0 ..< 8 { + for _ in 0..<8 { group.addTask { var code: UInt = 0 try await box.value.test(.POST, "auth/refresh", beforeRequest: { req in From 5f8b8676615a4baa1903165563593c2f3a5c361f Mon Sep 17 00:00:00 2001 From: Yakiv Kovalskyi Date: Sun, 12 Jul 2026 13:57:55 +0200 Subject: [PATCH 4/4] Auth: fix concurrent-refresh regression test to compile under Swift 6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- chess-server/Tests/AppTests/AuthTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chess-server/Tests/AppTests/AuthTests.swift b/chess-server/Tests/AppTests/AuthTests.swift index edb82aa..f42ecba 100644 --- a/chess-server/Tests/AppTests/AuthTests.swift +++ b/chess-server/Tests/AppTests/AuthTests.swift @@ -99,7 +99,7 @@ final class AuthTests: XCTestCase { var code: UInt = 0 try await box.value.test(.POST, "auth/refresh", beforeRequest: { req in try req.content.encode(RefreshRequest(refreshToken: token), as: .json) - }, afterResponse: { res in + }, afterResponse: { res async in code = res.status.code }) return code