From ca77f7a0561525ae0668034b4efc289274748f41 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 14:04:38 +0000 Subject: [PATCH 1/4] Server: move the auth rate limiter to a shared sliding-window store (closes #79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /auth/* throttle added for #32 counted requests in process memory, so a multi-instance deployment would multiply the effective limit by the instance count and every restart reset all windows. Counters now live in a new auth_rate_windows table in the application database — the store all instances already share — via an atomic upsert (INSERT ... ON CONFLICT ... DO UPDATE ... RETURNING), which works on both Postgres and SQLite. The window also slides now: a verdict weighs the current bucket plus the previous one decayed linearly, closing the 2x burst that fixed windows admit across a window boundary. Refused requests count too, so hammering past the limit keeps the client limited. Stale buckets are swept at most once per window to keep the table at ~two rows per active key. Same knobs as before (AUTH_RATE_LIMIT, AUTH_RATE_LIMIT_WINDOW); tests now cover cross-instance sharing, boundary smoothing, and the sweep. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- .../Sources/App/Auth/AuthRateLimiter.swift | 116 +++++++++++++----- .../Sources/App/Migrations/Migrations.swift | 18 +++ chess-server/Sources/App/configure.swift | 10 +- .../Tests/AppTests/AuthAbuseTests.swift | 104 +++++++++++++--- 4 files changed, 194 insertions(+), 54 deletions(-) diff --git a/chess-server/Sources/App/Auth/AuthRateLimiter.swift b/chess-server/Sources/App/Auth/AuthRateLimiter.swift index a4beea5..0a8354a 100644 --- a/chess-server/Sources/App/Auth/AuthRateLimiter.swift +++ b/chess-server/Sources/App/Auth/AuthRateLimiter.swift @@ -1,49 +1,102 @@ import Vapor +import Fluent +import SQLKit -/// Fixed-window request counter keyed by client, used to throttle the -/// unauthenticated auth endpoints. A window opens on a key's first request -/// and admits `limit` requests until `window` seconds have passed; later -/// requests are refused with the seconds remaining until the window resets. -actor FixedWindowRateLimiter { +/// Sliding-window request counter shared through the application database, +/// used to throttle the unauthenticated auth endpoints. Counters live in the +/// `auth_rate_windows` table rather than process memory, so every server +/// instance draws from the same budget and restarts don't reset windows (#79). +/// +/// The window slides: requests land in fixed buckets of `window` seconds, and +/// a verdict weighs the current bucket plus the previous one decayed linearly +/// by how far the current bucket has progressed. A burst straddling a bucket +/// boundary therefore can't get 2× the limit the way plain fixed windows +/// allow. Refused requests are counted too, so a client hammering past the +/// limit keeps itself limited. +actor SlidingWindowRateLimiter { enum Verdict: Equatable { case allowed case limited(retryAfter: TimeInterval) } - private struct Window { - var start: Date - var count: Int - } - private let limit: Int private let window: TimeInterval - private var windows: [String: Window] = [:] - - /// Entry count above which expired windows are swept out, so an attacker - /// rotating source addresses can't grow the dictionary without bound. - private let pruneThreshold = 4096 + private var lastSweep: Date = .distantPast init(limit: Int, window: TimeInterval = 60) { self.limit = max(1, limit) - self.window = window + self.window = max(1, window) } - func check(_ key: String, now: Date = Date()) -> Verdict { - if windows.count > pruneThreshold { - windows = windows.filter { now.timeIntervalSince($0.value.start) < window } + private struct CountRow: Decodable { + var count: Int + } + + func check(_ key: String, on database: any Database, now: Date = Date()) async throws -> Verdict { + guard let sql = database as? any SQLDatabase else { + // Both supported drivers (Postgres, SQLite) are SQL databases; + // reaching this means a misconfigured fixture, not a client at + // fault — and a limiter that can't count must not admit. + throw Abort(.internalServerError, reason: "rate limiter requires a SQL database") } - if var current = windows[key], now.timeIntervalSince(current.start) < window { - guard current.count < limit else { - return .limited(retryAfter: window - now.timeIntervalSince(current.start)) + let bucket = Int(now.timeIntervalSince1970 / window) + let fraction = (now.timeIntervalSince1970 - Double(bucket) * window) / window + + // One atomic statement counts this request and reads the result, so + // concurrent requests across instances can never both see the last + // free slot. + let current = try await sql.raw(""" + INSERT INTO "auth_rate_windows" ("key", "bucket", "count") + VALUES (\(bind: key), \(bind: bucket), 1) + ON CONFLICT ("key", "bucket") + DO UPDATE SET "count" = "auth_rate_windows"."count" + 1 + RETURNING "count" + """).first(decoding: CountRow.self)?.count ?? 1 + + let previous = try await sql.raw(""" + SELECT "count" FROM "auth_rate_windows" + WHERE "key" = \(bind: key) AND "bucket" = \(bind: bucket - 1) + """).first(decoding: CountRow.self)?.count ?? 0 + + try await sweepIfDue(sql, currentBucket: bucket, now: now) + + let weighted = Double(previous) * (1 - fraction) + Double(current) + guard weighted > Double(limit) else { return .allowed } + return .limited(retryAfter: retryAfter(previous: previous, current: current, fraction: fraction)) + } + + /// Seconds until a retry would be admitted, assuming the client goes + /// quiet meanwhile (further requests push this out — they count too). + private func retryAfter(previous: Int, current: Int, fraction: Double) -> TimeInterval { + let limit = Double(self.limit) + + // Still within the current bucket, the previous bucket's weight + // decays: a retry at fraction g adds one to `current` and is admitted + // once previous × (1 − g) + current + 1 ≤ limit. + if previous > 0 { + let g = 1 - (limit - Double(current) - 1) / Double(previous) + if g <= 1 { + return max(1, (g - fraction) * window) } - current.count += 1 - windows[key] = current - return .allowed } - windows[key] = Window(start: now, count: 1) - return .allowed + // Otherwise wait for rollover, when this bucket becomes the previous + // one and decays in turn: current × (1 − g) + 1 ≤ limit. + let g = max(0, 1 - (limit - 1) / Double(current)) + return max(1, (1 - fraction + g) * window) + } + + /// Buckets older than the previous window no longer influence any + /// verdict. Sweeping at most once per window keeps the table at roughly + /// two rows per recently active key, so an attacker rotating source + /// addresses can't grow it without bound. + private func sweepIfDue(_ sql: any SQLDatabase, currentBucket: Int, now: Date) async throws { + guard now.timeIntervalSince(lastSweep) >= window else { return } + lastSweep = now + try await sql.raw(""" + DELETE FROM "auth_rate_windows" WHERE "bucket" < \(bind: currentBucket - 1) + """).run() } } @@ -51,7 +104,8 @@ actor FixedWindowRateLimiter { /// IP. Refusals are 429 with a Retry-After header. struct AuthRateLimitMiddleware: AsyncMiddleware { func respond(to request: Request, chainingTo next: AsyncResponder) async throws -> Response { - let verdict = await request.application.authRateLimiter.check(clientKey(for: request)) + let verdict = try await request.application.authRateLimiter + .check(clientKey(for: request), on: request.db) if case .limited(let retryAfter) = verdict { let seconds = max(1, Int(retryAfter.rounded(.up))) throw Abort(.tooManyRequests, @@ -96,13 +150,13 @@ extension Environment { extension Application { private struct AuthRateLimiterKey: StorageKey { - typealias Value = FixedWindowRateLimiter + typealias Value = SlidingWindowRateLimiter } /// Shared limiter for the auth endpoints. Configured in `configure(_:)`; /// tests install a tighter one to exercise the 429 path. - var authRateLimiter: FixedWindowRateLimiter { - get { storage[AuthRateLimiterKey.self] ?? FixedWindowRateLimiter(limit: .max) } + var authRateLimiter: SlidingWindowRateLimiter { + get { storage[AuthRateLimiterKey.self] ?? SlidingWindowRateLimiter(limit: .max) } set { storage[AuthRateLimiterKey.self] = newValue } } } diff --git a/chess-server/Sources/App/Migrations/Migrations.swift b/chess-server/Sources/App/Migrations/Migrations.swift index a97f823..5bb9489 100644 --- a/chess-server/Sources/App/Migrations/Migrations.swift +++ b/chess-server/Sources/App/Migrations/Migrations.swift @@ -100,6 +100,24 @@ struct CreateAppleNonce: AsyncMigration { } } +struct CreateAuthRateWindow: AsyncMigration { + // Shared store for the auth rate limiter (#79). No `.id()`: the limiter + // upserts on ("key", "bucket") and never supplies one; the composite + // unique constraint is also what its ON CONFLICT target resolves to. + func prepare(on database: Database) async throws { + try await database.schema("auth_rate_windows") + .field("key", .string, .required) + .field("bucket", .int64, .required) + .field("count", .int, .required) + .unique(on: "key", "bucket") + .create() + } + + func revert(on database: Database) async throws { + try await database.schema("auth_rate_windows").delete() + } +} + struct CreateGameRecord: AsyncMigration { func prepare(on database: Database) async throws { try await database.schema(GameRecord.schema) diff --git a/chess-server/Sources/App/configure.swift b/chess-server/Sources/App/configure.swift index e647f9a..5cd4d0c 100644 --- a/chess-server/Sources/App/configure.swift +++ b/chess-server/Sources/App/configure.swift @@ -33,6 +33,7 @@ public func configure(_ app: Application) async throws { app.migrations.add(AddUserAppleID()) app.migrations.add(AddGameRecordTimeControl()) app.migrations.add(CreateAppleNonce()) + app.migrations.add(CreateAuthRateWindow()) try await app.autoMigrate() // MARK: JWT signing key @@ -59,13 +60,14 @@ public func configure(_ app: Application) async throws { app.jwt.apple.applicationIdentifier = appleAppID } - // MARK: Auth abuse protection (#32) + // MARK: Auth abuse protection (#32, #79) - // Per-IP fixed-window throttle on /auth/*. Effectively off under test so - // the suite's rapid registrations pass; rate-limit tests install a tight + // Per-IP sliding-window throttle on /auth/*, counted in the database so + // all instances share one budget. Effectively off under test so the + // suite's rapid registrations pass; rate-limit tests install a tight // limiter themselves. let defaultLimit = app.environment == .testing ? Int.max : 10 - app.authRateLimiter = FixedWindowRateLimiter( + app.authRateLimiter = SlidingWindowRateLimiter( limit: Environment.get("AUTH_RATE_LIMIT").flatMap(Int.init) ?? defaultLimit, window: Environment.get("AUTH_RATE_LIMIT_WINDOW").flatMap(Double.init) ?? 60 ) diff --git a/chess-server/Tests/AppTests/AuthAbuseTests.swift b/chess-server/Tests/AppTests/AuthAbuseTests.swift index 6250f72..6b452f6 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/chess-server/Tests/AppTests/AuthAbuseTests.swift @@ -1,10 +1,12 @@ @testable import App import XCTVapor import Fluent +import SQLKit import ChessOnline -/// Abuse protection for the auth surface (#32): the per-IP rate limit on -/// /auth/* and the abandoned-guest cleanup job. +/// Abuse protection for the auth surface (#32, #79): the per-IP rate limit +/// on /auth/* — counted in the shared database so every instance draws from +/// one budget — and the abandoned-guest cleanup job. final class AuthAbuseTests: XCTestCase { var app: Application! @@ -20,41 +22,105 @@ final class AuthAbuseTests: XCTestCase { // MARK: - Limiter semantics - func testLimiterAdmitsUpToLimitThenRefusesUntilWindowResets() async { - let limiter = FixedWindowRateLimiter(limit: 3, window: 60) - let start = Date() + /// A fixed instant on a bucket boundary (divisible by the 60s window), + /// so tests control exactly where in a bucket each request lands. + private let bucketStart = Date(timeIntervalSince1970: 60_000_000) + + func testLimiterAdmitsUpToLimitThenRefusesUntilWindowPasses() async throws { + let limiter = SlidingWindowRateLimiter(limit: 3, window: 60) for i in 0..<3 { - let verdict = await limiter.check("ip", now: start.addingTimeInterval(Double(i))) + let verdict = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(Double(i))) XCTAssertEqual(verdict, .allowed, "request \(i + 1) is within the limit") } - let refused = await limiter.check("ip", now: start.addingTimeInterval(10)) - XCTAssertEqual(refused, .limited(retryAfter: 50), "window opened at t=0, so t=10 waits 50s") + guard case .limited(let retryAfter) = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(10)) else { + return XCTFail("request past the limit must be refused") + } + XCTAssertGreaterThanOrEqual(retryAfter, 1, "retry guidance is at least a second") - // A fresh window admits again. - let afterReset = await limiter.check("ip", now: start.addingTimeInterval(61)) + // Two windows later both buckets have aged out; the key is admitted again. + let afterReset = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(121)) XCTAssertEqual(afterReset, .allowed) } - func testLimiterKeysAreIndependent() async { - let limiter = FixedWindowRateLimiter(limit: 1, window: 60) - let now = Date() + func testWindowBoundaryBurstIsSmoothed() async throws { + // A fixed window admits `limit` at the end of one window and `limit` + // more right after the boundary — 2× the limit in seconds. The + // sliding window weighs the previous bucket in, decaying as the new + // bucket progresses. + let limiter = SlidingWindowRateLimiter(limit: 4, window: 60) + + for i in 0..<4 { + let lateBurst = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(55 + Double(i))) + XCTAssertEqual(lateBurst, .allowed, "the first 4 requests fit the budget") + } + if case .allowed = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(61)) { + XCTFail("just past the boundary the previous bucket still weighs ~full; a fresh burst must be refused") + } + // Halfway into the new bucket the old one has decayed enough to admit. + let afterDecay = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(91)) + XCTAssertEqual(afterDecay, .allowed) + } + + func testLimiterKeysAreIndependent() async throws { + let limiter = SlidingWindowRateLimiter(limit: 1, window: 60) - let first = await limiter.check("ip-a", now: now) + let first = try await limiter.check("ip-a", on: app.db, now: bucketStart) XCTAssertEqual(first, .allowed) - if case .allowed = await limiter.check("ip-a", now: now) { + if case .allowed = try await limiter.check("ip-a", on: app.db, now: bucketStart) { XCTFail("second request from the same key must be limited") } - let otherKey = await limiter.check("ip-b", now: now) + let otherKey = try await limiter.check("ip-b", on: app.db, now: bucketStart) XCTAssertEqual(otherKey, .allowed, "another key has its own window") } + func testLimitIsSharedAcrossLimiterInstances() async throws { + // Two limiters over one database stand in for two server instances: + // the budget must be shared, not `limit × instances` (#79). + let one = SlidingWindowRateLimiter(limit: 2, window: 60) + let two = SlidingWindowRateLimiter(limit: 2, window: 60) + + let first = try await one.check("ip", on: app.db, now: bucketStart) + XCTAssertEqual(first, .allowed) + let second = try await two.check("ip", on: app.db, now: bucketStart.addingTimeInterval(1)) + XCTAssertEqual(second, .allowed) + if case .allowed = try await two.check("ip", on: app.db, now: bucketStart.addingTimeInterval(2)) { + XCTFail("an instance must see requests counted by its peers") + } + if case .allowed = try await one.check("ip", on: app.db, now: bucketStart.addingTimeInterval(3)) { + XCTFail("the refusal holds on every instance") + } + } + + func testStaleBucketsAreSweptFromTheStore() async throws { + let limiter = SlidingWindowRateLimiter(limit: 5, window: 60) + + _ = try await limiter.check("old-ip", on: app.db, now: bucketStart) + // Two windows later any check sweeps buckets that no longer affect + // verdicts, so rotating source addresses can't grow the table. + _ = try await limiter.check("new-ip", on: app.db, now: bucketStart.addingTimeInterval(180)) + + let oldRows = try await windowRows(key: "old-ip") + XCTAssertEqual(oldRows, 0, "aged-out buckets are deleted") + let newRows = try await windowRows(key: "new-ip") + XCTAssertEqual(newRows, 1, "the live bucket stays") + } + + private func windowRows(key: String) async throws -> Int { + struct Row: Decodable { var count: Int } + let sql = try XCTUnwrap(app.db as? any SQLDatabase) + let row = try await sql.raw(""" + SELECT COUNT(*) AS "count" FROM "auth_rate_windows" WHERE "key" = \(bind: key) + """).first(decoding: Row.self) + return try XCTUnwrap(row).count + } + // MARK: - HTTP surface func testAuthEndpointsReturn429PastTheLimit() async throws { // Test requests carry no socket address, so they all share one // bucket — which is exactly what this test needs. - app.authRateLimiter = FixedWindowRateLimiter(limit: 2, window: 60) + app.authRateLimiter = SlidingWindowRateLimiter(limit: 2, window: 60) for _ in 0..<2 { try await app.test(.POST, "auth/register", beforeRequest: { req in @@ -90,7 +156,7 @@ final class AuthAbuseTests: XCTestCase { // would fold every client into one shared bucket. setenv("TRUST_PROXY_HEADERS", "1", 1) defer { unsetenv("TRUST_PROXY_HEADERS") } - app.authRateLimiter = FixedWindowRateLimiter(limit: 1, window: 60) + app.authRateLimiter = SlidingWindowRateLimiter(limit: 1, window: 60) // Two clients sharing the Fly-style XFF tail each get their own window… for client in ["203.0.113.7", "203.0.113.8"] { @@ -118,7 +184,7 @@ final class AuthAbuseTests: XCTestCase { // Client-controlled earlier entries must not affect the key. setenv("TRUST_PROXY_HEADERS", "1", 1) defer { unsetenv("TRUST_PROXY_HEADERS") } - app.authRateLimiter = FixedWindowRateLimiter(limit: 1, window: 60) + app.authRateLimiter = SlidingWindowRateLimiter(limit: 1, window: 60) try await app.test(.POST, "auth/register", beforeRequest: { req in req.headers.replaceOrAdd(name: "X-Forwarded-For", value: "10.9.9.9, 198.51.100.4") From 560e1b4bcc42225505d816059bcb8a9cfeb22a2b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 14:06:28 +0000 Subject: [PATCH 2/4] Lint: dedent multiline SQL strings to scope level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SwiftFormat's indent rule wants multiline string contents at the statement's own indentation (the existing Migrations.swift style), not one level deeper. String contents are unchanged — Swift strips indentation relative to the closing delimiter. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- .../Sources/App/Auth/AuthRateLimiter.swift | 22 +++++++++---------- .../Tests/AppTests/AuthAbuseTests.swift | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/chess-server/Sources/App/Auth/AuthRateLimiter.swift b/chess-server/Sources/App/Auth/AuthRateLimiter.swift index 0a8354a..93e579e 100644 --- a/chess-server/Sources/App/Auth/AuthRateLimiter.swift +++ b/chess-server/Sources/App/Auth/AuthRateLimiter.swift @@ -47,17 +47,17 @@ actor SlidingWindowRateLimiter { // concurrent requests across instances can never both see the last // free slot. let current = try await sql.raw(""" - INSERT INTO "auth_rate_windows" ("key", "bucket", "count") - VALUES (\(bind: key), \(bind: bucket), 1) - ON CONFLICT ("key", "bucket") - DO UPDATE SET "count" = "auth_rate_windows"."count" + 1 - RETURNING "count" - """).first(decoding: CountRow.self)?.count ?? 1 + INSERT INTO "auth_rate_windows" ("key", "bucket", "count") + VALUES (\(bind: key), \(bind: bucket), 1) + ON CONFLICT ("key", "bucket") + DO UPDATE SET "count" = "auth_rate_windows"."count" + 1 + RETURNING "count" + """).first(decoding: CountRow.self)?.count ?? 1 let previous = try await sql.raw(""" - SELECT "count" FROM "auth_rate_windows" - WHERE "key" = \(bind: key) AND "bucket" = \(bind: bucket - 1) - """).first(decoding: CountRow.self)?.count ?? 0 + SELECT "count" FROM "auth_rate_windows" + WHERE "key" = \(bind: key) AND "bucket" = \(bind: bucket - 1) + """).first(decoding: CountRow.self)?.count ?? 0 try await sweepIfDue(sql, currentBucket: bucket, now: now) @@ -95,8 +95,8 @@ actor SlidingWindowRateLimiter { guard now.timeIntervalSince(lastSweep) >= window else { return } lastSweep = now try await sql.raw(""" - DELETE FROM "auth_rate_windows" WHERE "bucket" < \(bind: currentBucket - 1) - """).run() + DELETE FROM "auth_rate_windows" WHERE "bucket" < \(bind: currentBucket - 1) + """).run() } } diff --git a/chess-server/Tests/AppTests/AuthAbuseTests.swift b/chess-server/Tests/AppTests/AuthAbuseTests.swift index 6b452f6..9a5ca80 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/chess-server/Tests/AppTests/AuthAbuseTests.swift @@ -110,8 +110,8 @@ final class AuthAbuseTests: XCTestCase { struct Row: Decodable { var count: Int } let sql = try XCTUnwrap(app.db as? any SQLDatabase) let row = try await sql.raw(""" - SELECT COUNT(*) AS "count" FROM "auth_rate_windows" WHERE "key" = \(bind: key) - """).first(decoding: Row.self) + SELECT COUNT(*) AS "count" FROM "auth_rate_windows" WHERE "key" = \(bind: key) + """).first(decoding: Row.self) return try XCTUnwrap(row).count } From 6aa7b26af30c9f9f9244f7ba0aa41278262b9ede Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 17:06:38 +0000 Subject: [PATCH 3/4] Rate limiter: harden against self-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From an adversarial review of the PR diff: - The stale-bucket sweep now runs after the verdict is decided and logs failures instead of throwing — housekeeping could previously turn an already-decided request into a 500. - The sweep reclaims any bucket outside current±1, so changing AUTH_RATE_LIMIT_WINDOW (which renumbers buckets) can't strand rows, and a clock-ahead peer's current bucket survives. - Keys are clamped to 64 chars: real keys are IPs, and a hostile proxy-supplied header must not blow index row-size limits and turn the upsert into an error path. - Refusal counting saturates at limit+1, restoring the fixed window's recovery-after-rollover for hammered keys and keeping Retry-After honest; tests pin the exact retry value and the saturation cap. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T --- .../Sources/App/Auth/AuthRateLimiter.swift | 55 +++++++++++++------ .../Tests/AppTests/AuthAbuseTests.swift | 21 ++++++- 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/chess-server/Sources/App/Auth/AuthRateLimiter.swift b/chess-server/Sources/App/Auth/AuthRateLimiter.swift index 93e579e..560189e 100644 --- a/chess-server/Sources/App/Auth/AuthRateLimiter.swift +++ b/chess-server/Sources/App/Auth/AuthRateLimiter.swift @@ -11,8 +11,9 @@ import SQLKit /// a verdict weighs the current bucket plus the previous one decayed linearly /// by how far the current bucket has progressed. A burst straddling a bucket /// boundary therefore can't get 2× the limit the way plain fixed windows -/// allow. Refused requests are counted too, so a client hammering past the -/// limit keeps itself limited. +/// allow. Refused requests count too, but only up to one past the limit — +/// hammering can't wedge the counter, so once the traffic stops the key is +/// admitted again after rollover, the same recovery the fixed window gave. actor SlidingWindowRateLimiter { enum Verdict: Equatable { case allowed @@ -40,17 +41,26 @@ actor SlidingWindowRateLimiter { throw Abort(.internalServerError, reason: "rate limiter requires a SQL database") } + // Real keys are IP addresses (≤45 chars); a hostile proxy-supplied + // header must not be able to blow past index row-size limits and + // turn the counter upsert into an error path. + let key = String(key.prefix(64)) + let bucket = Int(now.timeIntervalSince1970 / window) let fraction = (now.timeIntervalSince1970 - Double(bucket) * window) / window // One atomic statement counts this request and reads the result, so // concurrent requests across instances can never both see the last - // free slot. + // free slot. The CASE saturates the count one past the limit (see + // the type comment). let current = try await sql.raw(""" INSERT INTO "auth_rate_windows" ("key", "bucket", "count") VALUES (\(bind: key), \(bind: bucket), 1) ON CONFLICT ("key", "bucket") - DO UPDATE SET "count" = "auth_rate_windows"."count" + 1 + DO UPDATE SET "count" = CASE + WHEN "auth_rate_windows"."count" > \(bind: limit) THEN "auth_rate_windows"."count" + ELSE "auth_rate_windows"."count" + 1 + END RETURNING "count" """).first(decoding: CountRow.self)?.count ?? 1 @@ -59,11 +69,15 @@ actor SlidingWindowRateLimiter { WHERE "key" = \(bind: key) AND "bucket" = \(bind: bucket - 1) """).first(decoding: CountRow.self)?.count ?? 0 - try await sweepIfDue(sql, currentBucket: bucket, now: now) - let weighted = Double(previous) * (1 - fraction) + Double(current) - guard weighted > Double(limit) else { return .allowed } - return .limited(retryAfter: retryAfter(previous: previous, current: current, fraction: fraction)) + let verdict: Verdict = weighted <= Double(limit) + ? .allowed + : .limited(retryAfter: retryAfter(previous: previous, current: current, fraction: fraction)) + + // Housekeeping runs after the verdict is decided and never fails the + // request. + await sweepIfDue(sql, currentBucket: bucket, now: now) + return verdict } /// Seconds until a retry would be admitted, assuming the client goes @@ -87,16 +101,25 @@ actor SlidingWindowRateLimiter { return max(1, (1 - fraction + g) * window) } - /// Buckets older than the previous window no longer influence any - /// verdict. Sweeping at most once per window keeps the table at roughly - /// two rows per recently active key, so an attacker rotating source - /// addresses can't grow it without bound. - private func sweepIfDue(_ sql: any SQLDatabase, currentBucket: Int, now: Date) async throws { + /// Buckets outside current±1 no longer influence any verdict: previous + /// feeds the decay, current+1 tolerates a clock-ahead peer instance, and + /// everything else — including rows stranded by an + /// AUTH_RATE_LIMIT_WINDOW change, which renumbers buckets — is + /// reclaimed. Sweeping at most once per window keeps the table at a few + /// rows per recently active key, so an attacker rotating source + /// addresses can't grow it without bound. Errors are logged, not + /// thrown: housekeeping must never fail an already-decided request. + private func sweepIfDue(_ sql: any SQLDatabase, currentBucket: Int, now: Date) async { guard now.timeIntervalSince(lastSweep) >= window else { return } lastSweep = now - try await sql.raw(""" - DELETE FROM "auth_rate_windows" WHERE "bucket" < \(bind: currentBucket - 1) - """).run() + do { + try await sql.raw(""" + DELETE FROM "auth_rate_windows" + WHERE "bucket" < \(bind: currentBucket - 1) OR "bucket" > \(bind: currentBucket + 1) + """).run() + } catch { + sql.logger.warning("auth rate limiter sweep failed: \(String(reflecting: error))") + } } } diff --git a/chess-server/Tests/AppTests/AuthAbuseTests.swift b/chess-server/Tests/AppTests/AuthAbuseTests.swift index 9a5ca80..740ded7 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/chess-server/Tests/AppTests/AuthAbuseTests.swift @@ -36,7 +36,10 @@ final class AuthAbuseTests: XCTestCase { guard case .limited(let retryAfter) = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(10)) else { return XCTFail("request past the limit must be refused") } - XCTAssertGreaterThanOrEqual(retryAfter, 1, "retry guidance is at least a second") + // With no previous bucket and current saturated at 4, a quiet client + // is admitted at fraction g = 1 − (limit−1)/current = 0.5 of the + // next bucket: (1 − 10/60 + 0.5) × 60 = 80s. + XCTAssertEqual(retryAfter, 80, accuracy: 0.001, "retry guidance matches the decay math") // Two windows later both buckets have aged out; the key is admitted again. let afterReset = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(121)) @@ -92,6 +95,22 @@ final class AuthAbuseTests: XCTestCase { } } + func testRefusalsSaturateOnePastTheLimit() async throws { + // Hammering must not grow the counter without bound: the count caps + // at limit + 1, so once traffic stops the key recovers after one + // rollover (old fixed-window behavior) and Retry-After stays honest. + let limiter = SlidingWindowRateLimiter(limit: 2, window: 60) + for i in 0..<8 { + _ = try await limiter.check("ip", on: app.db, now: bucketStart.addingTimeInterval(Double(i))) + } + struct Row: Decodable { var count: Int } + let sql = try XCTUnwrap(app.db as? any SQLDatabase) + let row = try await sql.raw(""" + SELECT "count" FROM "auth_rate_windows" WHERE "key" = \(bind: "ip") + """).first(decoding: Row.self) + XCTAssertEqual(try XCTUnwrap(row).count, 3, "counter saturates at limit + 1") + } + func testStaleBucketsAreSweptFromTheStore() async throws { let limiter = SlidingWindowRateLimiter(limit: 5, window: 60) From 511a5e261da451dd37352b6a46a90e3fe64ca8fd Mon Sep 17 00:00:00 2001 From: testtest126 <44771568+testtest126@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:02:13 +0200 Subject: [PATCH 4/4] Rate limiter: close the BLOCK review's three gaps for revival MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior BLOCK @ 1b466b4 (independent second review of the closed PR #136) found the earlier APPROVE unsound and flagged three concrete gaps before this branch could be revived for #182: - Missing DB-error fail-closed coverage: add a limiter-level test (the check throws when the store is unqueryable) and a middleware/HTTP-level test (a broken store yields a non-2xx response and creates no account). - Missing genuine concurrency coverage: sequential awaits can't catch a check-then-act race. Add a test that fires real concurrent requests at one shared connection via withThrowingTaskGroup and asserts the atomic upsert holds the budget to exactly the configured limit. - CI only ever validated the raw SQL against SQLite. Add a non-required CI job (server-postgres) that runs the rate-limiter test group against a real local Postgres, so the actual ON CONFLICT … DO UPDATE … RETURNING syntax is exercised before deploy, not just SQLite's dialect-compatible mode. Scoped to the rate-limiter tests rather than the whole suite: unrelated tests assert global table counts that only hold under SQLite's per-test fresh in-memory database, not a shared persistent Postgres. Verified locally: all 96 chess-server tests pass against SQLite; the 12 rate-limiter/HTTP-surface tests pass against a real local Postgres 16. --- .github/workflows/ci.yml | 39 ++++++++++ .../Tests/AppTests/AuthAbuseTests.swift | 75 +++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 025724d..f48277c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,45 @@ jobs: - name: Run server tests run: swift test --package-path chess-server + # Validates the auth rate limiter's raw SQL (INSERT … ON CONFLICT … DO + # UPDATE … RETURNING, #182) against a real Postgres, not just SQLite — + # the `server` job above always runs against in-memory SQLite since + # DATABASE_URL is unset there, and dialect differences (e.g. UPSERT + # semantics) only surface against the real driver. + # + # NOT a required check: same pattern as `lint` and the `iOS` workflow — + # informational, so a flaky or slow local-Postgres setup here can't + # destabilize the required `Server tests` gate (flipping it to required + # is the orchestrator's call). Filtered to the rate-limiter test group + # rather than the whole suite: unrelated tests elsewhere in this file + # assert global table counts, which only hold under SQLite's per-test + # fresh in-memory database — a real Postgres instance is shared and + # persistent across the whole job, so those assertions don't transfer. + server-postgres: + name: Server tests (Postgres) + runs-on: macos-15 + timeout-minutes: 30 + steps: + - uses: actions/checkout@v7 + - name: Install and start Postgres + run: | + brew install postgresql@16 + LC_ALL="en_US.UTF-8" "$(brew --prefix postgresql@16)/bin/pg_ctl" \ + -D "$(brew --prefix)/var/postgresql@16" -l /tmp/postgres.log start + "$(brew --prefix postgresql@16)/bin/createdb" chess_ci + - name: Cache SwiftPM build + uses: actions/cache@v6 + with: + path: chess-server/.build + key: server-pg-spm-${{ runner.os }}-${{ hashFiles('chess-server/Package.swift', 'chess-server/Package.resolved') }} + restore-keys: server-pg-spm-${{ runner.os }}- + - name: Run rate-limiter tests against Postgres + env: + DATABASE_URL: postgres://runner@localhost:5432/chess_ci + run: > + swift test --package-path chess-server + --filter 'AuthAbuseTests/test(Limiter|WindowBoundaryBurst|LimitIsSharedAcrossLimiterInstances|Refusals|StaleBuckets|Concurrent|AuthEndpointsReturn429|ProxyHeaderKeys|MiddlewareFailsClosed)' + # Fast, cheap style gate (issue #100). Runs on ubuntu with pinned prebuilt # binaries — no toolchain build, finishes in well under two minutes. # NOT a required check: flipping it to enforcing is the orchestrator's call diff --git a/chess-server/Tests/AppTests/AuthAbuseTests.swift b/chess-server/Tests/AppTests/AuthAbuseTests.swift index 740ded7..a09fbcc 100644 --- a/chess-server/Tests/AppTests/AuthAbuseTests.swift +++ b/chess-server/Tests/AppTests/AuthAbuseTests.swift @@ -111,6 +111,81 @@ final class AuthAbuseTests: XCTestCase { XCTAssertEqual(try XCTUnwrap(row).count, 3, "counter saturates at limit + 1") } + func testLimiterFailsClosedWhenStoreIsUnavailable() async throws { + // A store that can't be queried must refuse to vouch for a verdict + // rather than silently admitting — the check throws instead of + // returning `.allowed`. + let limiter = SlidingWindowRateLimiter(limit: 5, window: 60) + let sql = try XCTUnwrap(app.db as? any SQLDatabase) + try await sql.raw(#"DROP TABLE "auth_rate_windows""#).run() + + do { + _ = try await limiter.check("ip", on: app.db, now: bucketStart) + XCTFail("a query against a missing table must throw, not admit") + } catch { + // Any thrown error is fail-closed; the specific type is the + // driver's own "no such table" / "relation does not exist". + } + + // A shared, persistent database (unlike per-test in-memory SQLite) + // must not stay broken for whichever test runs next. + try await CreateAuthRateWindow().prepare(on: app.db) + } + + func testMiddlewareFailsClosedWhenStoreIsUnavailable() async throws { + // The HTTP-facing half of the same guarantee: a broken store must + // turn into a non-2xx response before the route handler runs, not + // a quietly-admitted request. + app.authRateLimiter = SlidingWindowRateLimiter(limit: 5, window: 60) + let sql = try XCTUnwrap(app.db as? any SQLDatabase) + try await sql.raw(#"DROP TABLE "auth_rate_windows""#).run() + + let usersBefore = try await User.query(on: app.db).count() + try await app.test(.POST, "auth/register", beforeRequest: { req in + try req.content.encode(RegisterRequest(displayName: nil), as: .json) + }, afterResponse: { res async in + XCTAssertFalse((200...299).contains(Int(res.status.code)), + "a limiter that can't count must not let the request through") + }) + let usersAfter = try await User.query(on: app.db).count() + XCTAssertEqual(usersBefore, usersAfter, "no account is created when the store is down") + + // A shared, persistent database (unlike per-test in-memory SQLite) + // must not stay broken for whichever test runs next. + try await CreateAuthRateWindow().prepare(on: app.db) + } + + func testConcurrentRequestsShareOneAtomicBudget() async throws { + // Sequential awaits (the tests above) can't catch a check-then-act + // race: a non-atomic "SELECT count, then INSERT/UPDATE" store would + // let concurrent requests all read the same stale count and all + // proceed. Firing real concurrent requests at one shared connection + // pool is the only way to exercise that race, and the single atomic + // upsert (INSERT … ON CONFLICT … DO UPDATE … RETURNING) must hold + // the budget to exactly `limit` admissions regardless of ordering. + let limiter = SlidingWindowRateLimiter(limit: 10, window: 60) + let attempts = 40 + let db = app.db + let now = bucketStart + + let verdicts = try await withThrowingTaskGroup(of: SlidingWindowRateLimiter.Verdict.self) { group in + for _ in 0..