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/Sources/App/Auth/AuthRateLimiter.swift b/chess-server/Sources/App/Auth/AuthRateLimiter.swift index a4beea5..560189e 100644 --- a/chess-server/Sources/App/Auth/AuthRateLimiter.swift +++ b/chess-server/Sources/App/Auth/AuthRateLimiter.swift @@ -1,49 +1,125 @@ 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 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 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)) + // 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. 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" = 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 + + 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 + + let weighted = Double(previous) * (1 - fraction) + Double(current) + 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 + /// 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 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 + 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))") + } } } @@ -51,7 +127,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 +173,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..a09fbcc 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,199 @@ 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") + } + // 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") - // 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 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 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.. 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 +250,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 +278,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")