Server: move the auth rate limiter to a shared sliding-window store (closes #79) - #116
Server: move the auth rate limiter to a shared sliding-window store (closes #79)#116testtest126 wants to merge 3 commits into
Conversation
…loses #79) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T
|
Self-review note for the security reviewer: ran an adversarial multi-agent review over this diff (5 lenses, findings verified by independent judges). Fixed at Reviewed and accepted as-is (for the verdict's awareness): every request costs a DB roundtrip before shedding — documented trade-off in the PR body, an in-memory pre-filter can layer on later without changing the store; fail-closed on DB errors (the endpoints behind the limiter can't work without the DB anyway); the previous-bucket SELECT can miss a peer's in-flight upsert at a boundary (sliding window is approximate by design); table growth between sweeps is bounded by distinct source addresses per two windows at ~one small row each. Generated by Claude Code |
|
@orchestrator — merge request from the smiley-face session (this session lacks
No shared files between the three — any order works. All are drafts per house style; happy to mark ready on request or leave that to you. Generated by Claude Code |
Pull request was closed
Summary
Closes #79 (follow-up from the #77 security review).
The
/auth/*throttle added for #32 counted requests in process memory: on a multi-instance deployment the effective limit becomeslimit × instances, and restarts reset all windows. This PR moves the counters into the application database — the store all instances already share — and makes the window slide:auth_rate_windowstable (key,bucket,count, unique onkey + bucket) holds the counters. Each request is counted with one atomic upsert (INSERT … ON CONFLICT … DO UPDATE … RETURNING), so concurrent requests across instances can never both take the last free slot. The raw SQL is dialect-portable and runs on both Postgres (DATABASE_URLdeployments) and SQLite (zero-spend volume, dev, tests) — same idiom as the existingAddUserAppleIDmigration. As a bonus, windows now survive restarts.Retry-Afterstill gives honest guidance for a client that goes quiet.AUTH_RATE_LIMIT,AUTH_RATE_LIMIT_WINDOW), same middleware/keying behavior (Fly-Client-IP → last XFF → peer address), same 429 +Retry-Aftersurface.Trade-off worth noting: the limit check now costs a DB roundtrip before the endpoint handler runs. Every endpoint behind it already does DB work per request (register inserts, refresh queries), so the limiter still fences off the expensive paths; if that ever matters, an in-memory pre-filter can be layered in front without changing the store.
Rule 5 note: this touches abuse protection on the auth surface and #79 came out of a security review, so the PR stays draft pending the security verdict / orchestrator flow per
CLAUDE.md.Test plan
CI runs both suites; the sandbox for this session cannot run
swift testlocally (no toolchain, and the network policy blocks fetching one), so the boxes below reflect CI status — all green at head01e4be4.swift testinChessKit/passes (untouched by this PR)swift testinchess-server/passes (if server code changed)iOSlane is green tooNew/updated coverage in
AuthAbuseTests:Retry-After/ proxy-header keying tests, ported to the new limiter🤖 Generated with Claude Code
https://claude.ai/code/session_01E9dkcpbwWWHrdGzpvWZd1T