From 15b1535305c996a1c0046569394370b6925aad68 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 18:42:37 -0400 Subject: [PATCH 1/5] docs(specs): scope rate limiting to /api and enumerate credential endpoints Only /api/** is counted; the SPA shell, assets and thumbnails never are. Credential endpoints get their own per-IP cap; session reads such as GET /api/auth/me are ordinary reads. Per-IP caps are generous because production's load balancer does not yet preserve client addresses, so every visitor currently shares one bucket (cfp-live-cluster #201). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LFyA5poHwrhAktrnsKrUiQ --- specs/api/auth.md | 2 +- specs/api/conventions.md | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/specs/api/auth.md b/specs/api/auth.md index c1ccf53..3cdf4ba 100644 --- a/specs/api/auth.md +++ b/specs/api/auth.md @@ -130,7 +130,7 @@ Plus `Set-Cookie` headers for `cfp_session` and `cfp_refresh`. ### Errors - `401 unauthenticated` with `error.code = "invalid_credentials"` — covers no-such-user, wrong-password, unknown-hash-format. Single response, comparable timing across cases. -- `429 too_many_requests` — per the auth-endpoint rate cap (10/min/IP) in [api/conventions.md](conventions.md). +- `429 too_many_requests` — per the auth-endpoint rate cap (the credential-endpoint cap in [api/conventions.md](conventions.md#rate-limiting)). ## POST /api/auth/password-reset/request diff --git a/specs/api/conventions.md b/specs/api/conventions.md index dd2b6c9..b71d80c 100644 --- a/specs/api/conventions.md +++ b/specs/api/conventions.md @@ -159,10 +159,14 @@ Every request body and query string is validated by a zod schema declared alongs Single-replica means rate-limit state is in-memory. Counters reset on restart; acceptable at civic scale. -- Unauthenticated reads: 60 requests / minute / IP +Only `/api/**` requests are counted. The SPA shell, its assets, and image thumbnails are served by the same process but are never rate limited — a single page load fetches dozens of them. + +- Unauthenticated reads: 1200 requests / minute / IP - Authenticated reads: 300 requests / minute / account -- Writes: 30 requests / minute / account -- Auth endpoints (`/api/auth/*`): 10 requests / minute / IP +- Writes: 30 requests / minute / account (120 / minute / IP when anonymous) +- Credential endpoints: 120 requests / minute / IP. These are the routes that accept or mint a credential: `POST /api/auth/login`, `/api/auth/github/start` and `/callback`, `/api/auth/link-github`, `/api/auth/password-reset/*`, and `POST /api/account-claim/by-password`. Session reads such as `GET /api/auth/me` (called on every page load) and `/api/auth/refresh` are ordinary reads. + +The per-IP caps are deliberately generous: in production the gateway sits behind a load balancer that does not yet preserve client addresses, so every visitor currently shares one "IP" (cfp-live-cluster #201). Tighten the credential cap once that lands. Exceeded → `429 rate_limited`, `Retry-After` header in seconds. From 55da0735129aa2774345780e3e0e4ccd05cae6a9 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 18:42:37 -0400 Subject: [PATCH 2/5] fix(api): rate-limit only /api and split credential endpoints from reads Minutes after cutover the site was returning 429s to everyone. The onRequest hook counted every asset and SPA request against the 60/min read cap (a page load is dozens), GET /api/auth/me sat in the 10/min /api/auth bucket alongside login, and the NodeBalancer hides client addresses so all of that was one shared bucket. Skip non-/api paths, classify credential endpoints explicitly, and put the caps in an exported table the tests assert against. Existing tests that issued 60 requests now prime the bucket instead. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LFyA5poHwrhAktrnsKrUiQ --- apps/api/src/plugins/rate-limit.ts | 73 +++++++++++++----- apps/api/tests/api-skeleton.test.ts | 26 ++++--- apps/api/tests/auth.test.ts | 12 +-- apps/api/tests/rate-limit.test.ts | 114 ++++++++++++++++++++++++++++ 4 files changed, 191 insertions(+), 34 deletions(-) create mode 100644 apps/api/tests/rate-limit.test.ts diff --git a/apps/api/src/plugins/rate-limit.ts b/apps/api/src/plugins/rate-limit.ts index 27a1310..77dab08 100644 --- a/apps/api/src/plugins/rate-limit.ts +++ b/apps/api/src/plugins/rate-limit.ts @@ -2,10 +2,18 @@ * In-memory rate-limit plugin. * * Enforces per-IP and per-account caps per specs/api/conventions.md#rate-limiting: - * - Unauthenticated reads: 60 req / min / IP - * - Authenticated reads: 300 req / min / account - * - Writes: 30 req / min / account - * - Auth endpoints: 10 req / min / IP + * - Unauthenticated reads: 1200 req / min / IP + * - Authenticated reads: 300 req / min / account + * - Writes: 30 req / min / account (120 / min / IP anonymous) + * - Credential endpoints: 120 req / min / IP + * + * Only `/api/**` is counted. The SPA shell, its assets, and thumbnails come out + * of the same process and a single page load fetches dozens of them; counting + * those against the read cap is what produced a site-wide 429 storm at cutover. + * + * Per-IP caps are generous on purpose: production's load balancer does not yet + * preserve client addresses (cfp-live-cluster #201), so every visitor shares + * one "IP" until that lands. * * Counters are reset on restart (intentional — single replica, civic scale). * Exceeded limit → RateLimitedError(retryAfterSeconds). @@ -23,6 +31,14 @@ interface BucketEntry { const WINDOW_MS = 60_000; // 1 minute +export const RATE_LIMITS = { + unauthenticatedReadsPerIp: 1200, + authenticatedReadsPerAccount: 300, + writesPerAccount: 30, + anonymousWritesPerIp: 120, + credentialPerIp: 120, +} as const; + function getOrCreate(map: Map, key: string): BucketEntry { let entry = map.get(key); if (!entry) { @@ -60,7 +76,30 @@ function clientIp(request: FastifyRequest): string { } const WRITE_METHODS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']); -const AUTH_PATH_PREFIX = '/api/auth'; +const API_PREFIX = '/api/'; + +/** + * Routes that accept or mint a credential. Session reads (`/api/auth/me`, + * `/api/auth/refresh`, `/api/auth/sessions`, `/api/auth/logout`) are ordinary + * traffic — `/me` runs on every page load. + */ +const CREDENTIAL_PATHS = [ + '/api/auth/login', + '/api/auth/github/start', + '/api/auth/github/callback', + '/api/auth/link-github', + '/api/auth/password-reset/', + '/api/account-claim/by-password', +]; + +function pathOf(url: string): string { + const q = url.indexOf('?'); + return q === -1 ? url : url.slice(0, q); +} + +export function isCredentialPath(path: string): boolean { + return CREDENTIAL_PATHS.some((p) => (p.endsWith('/') ? path.startsWith(p) : path === p)); +} async function rateLimitPlugin(fastify: FastifyInstance): Promise { const ipBuckets = new Map(); @@ -68,29 +107,29 @@ async function rateLimitPlugin(fastify: FastifyInstance): Promise { fastify.addHook('onRequest', (request, _reply, done) => { try { + const path = pathOf(request.url); + if (!path.startsWith(API_PREFIX)) { + done(); + return; + } + const ip = clientIp(request); const isWrite = WRITE_METHODS.has(request.method); - const isAuthEndpoint = request.url.startsWith(AUTH_PATH_PREFIX); - const personId = request.session?.person?.id; - if (isAuthEndpoint) { - // Auth endpoints: 10 req / min / IP - check(ipBuckets, `auth:${ip}`, 10); + if (isCredentialPath(path)) { + check(ipBuckets, `credential:${ip}`, RATE_LIMITS.credentialPerIp); } else if (isWrite) { if (personId) { - // Authenticated writes: 30 req / min / account - check(accountBuckets, `write-account:${personId}`, 30); + check(accountBuckets, `write-account:${personId}`, RATE_LIMITS.writesPerAccount); } else { - check(ipBuckets, `write:${ip}`, 30); + check(ipBuckets, `write:${ip}`, RATE_LIMITS.anonymousWritesPerIp); } } else { if (personId) { - // Authenticated reads: 300 req / min / account - check(accountBuckets, `account:${personId}`, 300); + check(accountBuckets, `account:${personId}`, RATE_LIMITS.authenticatedReadsPerAccount); } else { - // Unauthenticated reads: 60 req / min / IP - check(ipBuckets, `read:${ip}`, 60); + check(ipBuckets, `read:${ip}`, RATE_LIMITS.unauthenticatedReadsPerIp); } } } catch (err) { diff --git a/apps/api/tests/api-skeleton.test.ts b/apps/api/tests/api-skeleton.test.ts index 142c2b9..e64bd41 100644 --- a/apps/api/tests/api-skeleton.test.ts +++ b/apps/api/tests/api-skeleton.test.ts @@ -6,7 +6,7 @@ * - ValidationError surfaces as 422 validation_failed with expected shape * - Unknown Error surfaces as 500 internal_error with no message leak * - traceId appears in error responses - * - Per-IP rate limit: 61 anonymous reads → 429 with Retry-After + * - Per-IP rate limit: one read past the cap → 429 with Retry-After * - Idempotency-Key: repeat POST returns cached response * - /api/_openapi.json returns a valid OpenAPI 3.1 document * - /api/_docs renders (200 response) @@ -16,6 +16,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import type { FastifyInstance } from 'fastify'; import { buildApp } from '../src/app.js'; +import { RATE_LIMITS } from '../src/plugins/rate-limit.js'; import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js'; // --------------------------------------------------------------------------- @@ -169,18 +170,19 @@ describe('/api/_test/* route gating', () => { // --------------------------------------------------------------------------- describe('rate limiting', () => { - it('61 anonymous reads from the same IP → 429 with Retry-After on the 61st', async () => { - // Make 60 reads — all should succeed - for (let i = 0; i < 60; i++) { - const res = await app!.inject({ - method: 'GET', - url: '/api/health', - remoteAddress: '10.0.0.1', - }); - expect(res.statusCode, `Request ${i + 1} should succeed`).toBe(200); - } + it('one read past the per-IP cap → 429 with Retry-After', async () => { + // Prime the bucket to the cap rather than issuing 1200 requests; the last + // in-cap read must still succeed. + const limit = RATE_LIMITS.unauthenticatedReadsPerIp; + app!.rateLimitBuckets.ip.set('read:10.0.0.1', { count: limit - 1, windowStart: Date.now() }); + const okRes = await app!.inject({ + method: 'GET', + url: '/api/health', + remoteAddress: '10.0.0.1', + }); + expect(okRes.statusCode).toBe(200); - // The 61st should be rate-limited + // The next one should be rate-limited const res = await app!.inject({ method: 'GET', url: '/api/health', diff --git a/apps/api/tests/auth.test.ts b/apps/api/tests/auth.test.ts index d3c8e16..258e77a 100644 --- a/apps/api/tests/auth.test.ts +++ b/apps/api/tests/auth.test.ts @@ -22,6 +22,7 @@ import { type FastifyInstance } from 'fastify'; import { SignJWT } from 'jose'; import { buildApp } from '../src/app.js'; +import { RATE_LIMITS } from '../src/plugins/rate-limit.js'; import { mintSessionFor } from '../src/auth/issue.js'; import { verifyAccess, verifyRefresh } from '../src/auth/jwt.js'; import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js'; @@ -499,12 +500,13 @@ describe('account-based rate limits', () => { it('authenticated reads key on account bucket (300/min), separate from IP bucket', async () => { const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); - // Exhaust the IP bucket with anonymous reads (60 limit) - for (let i = 0; i < 60; i++) { - await app.inject({ method: 'GET', url: '/api/health', remoteAddress: '10.99.0.1' }); - } + // Exhaust the IP bucket (prime it to the cap rather than issuing 1200 reads) + app.rateLimitBuckets.ip.set('read:10.99.0.1', { + count: RATE_LIMITS.unauthenticatedReadsPerIp, + windowStart: Date.now(), + }); - // 61st anonymous request → 429 + // Next anonymous request → 429 const anonRes = await app.inject({ method: 'GET', url: '/api/health', diff --git a/apps/api/tests/rate-limit.test.ts b/apps/api/tests/rate-limit.test.ts new file mode 100644 index 0000000..c8e04f0 --- /dev/null +++ b/apps/api/tests/rate-limit.test.ts @@ -0,0 +1,114 @@ +/** + * Rate limiter scope — specs/api/conventions.md#rate-limiting. + * + * Covers what broke at cutover: + * - non-/api requests (SPA shell, assets, thumbnails) are never counted + * - GET /api/auth/me is an ordinary read, not a credential call + * - credential endpoints share one per-IP bucket at RATE_LIMITS.credentialPerIp + * - the unauthenticated read cap still trips one past RATE_LIMITS.unauthenticatedReadsPerIp + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import type { FastifyInstance } from 'fastify'; +import { buildApp } from '../src/app.js'; +import { RATE_LIMITS, isCredentialPath } from '../src/plugins/rate-limit.js'; +import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js'; + +let dataRepo: { path: string; cleanup: () => Promise }; +let privateStore: { path: string; cleanup: () => Promise }; +let app: FastifyInstance | undefined; + +const IP = '203.0.113.7'; + +beforeEach(async () => { + dataRepo = await createFullDataRepo(); + privateStore = await createPrivateStorageDir(); + app = await buildApp({ + serverOptions: { logger: false }, + overrideEnv: { + CFP_DATA_REPO_PATH: dataRepo.path, + STORAGE_BACKEND: 'filesystem', + CFP_PRIVATE_STORAGE_PATH: privateStore.path, + CFP_JWT_SIGNING_KEY: 'test-jwt-signing-key-at-least-32-chars!!', + NODE_ENV: 'test', + }, + }); +}); + +afterEach(async () => { + if (app) { + await app.close(); + app = undefined; + } + await dataRepo.cleanup(); + await privateStore.cleanup(); +}); + +function bucket(key: string): number { + return app!.rateLimitBuckets.ip.get(key)?.count ?? 0; +} + +describe('rate limiter scope', () => { + it('does not count non-/api requests (SPA shell, assets, thumbnails)', async () => { + for (const url of ['/', '/login', '/assets/index-abc123.js', '/thumbnail/1/100x100']) { + const res = await app!.inject({ method: 'GET', url, headers: { 'x-forwarded-for': IP } }); + expect(res.statusCode).not.toBe(429); + } + expect(app!.rateLimitBuckets.ip.size).toBe(0); + }); + + it('treats GET /api/auth/me as a read, not a credential call', async () => { + const res = await app!.inject({ + method: 'GET', + url: '/api/auth/me', + headers: { 'x-forwarded-for': IP }, + }); + expect(res.statusCode).toBe(200); + expect(bucket(`read:${IP}`)).toBe(1); + expect(bucket(`credential:${IP}`)).toBe(0); + }); + + it('classifies credential endpoints', () => { + for (const p of [ + '/api/auth/login', + '/api/auth/github/start', + '/api/auth/github/callback', + '/api/auth/link-github', + '/api/auth/password-reset/request', + '/api/auth/password-reset/confirm', + '/api/account-claim/by-password', + ]) { + expect(isCredentialPath(p), p).toBe(true); + } + for (const p of ['/api/auth/me', '/api/auth/refresh', '/api/auth/sessions', '/api/auth/logout', '/api/people']) { + expect(isCredentialPath(p), p).toBe(false); + } + }); + + it('caps credential endpoints per IP at RATE_LIMITS.credentialPerIp', async () => { + const limit = RATE_LIMITS.credentialPerIp; + let last = 0; + for (let i = 0; i < limit + 1; i++) { + const res = await app!.inject({ + method: 'POST', + url: '/api/auth/login', + headers: { 'x-forwarded-for': IP, 'content-type': 'application/json' }, + payload: {}, + }); + last = res.statusCode; + if (i < limit) expect(res.statusCode).not.toBe(429); + } + expect(last).toBe(429); + expect(bucket(`read:${IP}`)).toBe(0); + }); + + it('caps unauthenticated reads per IP one past RATE_LIMITS.unauthenticatedReadsPerIp', async () => { + const limit = RATE_LIMITS.unauthenticatedReadsPerIp; + // Prime the bucket directly rather than issuing 1200 requests. + app!.rateLimitBuckets.ip.set(`read:${IP}`, { count: limit - 1, windowStart: Date.now() }); + const ok = await app!.inject({ method: 'GET', url: '/api/health', headers: { 'x-forwarded-for': IP } }); + expect(ok.statusCode).toBe(200); + const over = await app!.inject({ method: 'GET', url: '/api/health', headers: { 'x-forwarded-for': IP } }); + expect(over.statusCode).toBe(429); + expect(over.headers['retry-after']).toBeDefined(); + }); +}); From 10c1c37c95773e64bb51e4016d8b0441902b1a53 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 18:42:37 -0400 Subject: [PATCH 3/5] fix(api): parse the session before the rate-limit and idempotency hooks All three plugins use onRequest hooks, which run in registration order, so rate-limit and idempotency saw request.session unset and keyed every signed-in request by IP. The existing test only passed because /me used to fall into a different bucket. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LFyA5poHwrhAktrnsKrUiQ --- apps/api/src/app.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 93e391e..99ea97a 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -13,8 +13,9 @@ * is built from the post-reconciliation tree) * 6b. push-daemon plugin → starts gitsheets push daemon * 6c. services plugin → builds in-memory state + FTS - * 7. rate-limit plugin → in-memory counters keyed per-IP + per-account - * 8. idempotency plugin → in-memory map keyed by personId+key + * 7. session middleware → parses the JWT cookie into request.session + * 8. rate-limit plugin → in-memory counters keyed per-IP + per-account + * 8a. idempotency plugin → in-memory map keyed by personId+key * 9. @fastify/swagger → OpenAPI 3.1 doc generation * 10. @fastify/swagger-ui → Swagger UI at /api/_docs * 11. routes → registered last after all plumbing @@ -151,15 +152,17 @@ export async function buildApp(opts: BuildAppOptions = {}): Promise Date: Thu, 17 Sep 2026 18:42:37 -0400 Subject: [PATCH 4/5] chore(plans): add rate-limit-scope Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LFyA5poHwrhAktrnsKrUiQ --- plans/rate-limit-scope.md | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 plans/rate-limit-scope.md diff --git a/plans/rate-limit-scope.md b/plans/rate-limit-scope.md new file mode 100644 index 0000000..64ef0ff --- /dev/null +++ b/plans/rate-limit-scope.md @@ -0,0 +1,56 @@ +--- +status: in-progress +depends: [saml-login-return-path] +specs: + - specs/api/conventions.md + - specs/api/auth.md +issues: [] +pr: null +--- + +# Plan: scope and loosen the rate limiter + +## Scope + +Minutes after the 2026-09-17 cutover, visitors (including the Slack SAML +test) started getting `429 rate_limited`. Three compounding causes: + +1. The limiter's `onRequest` hook counted **every** request — SPA shell, + JS/CSS chunks, thumbnails — against the 60/min unauthenticated read cap. + One page load is dozens of requests. +2. `GET /api/auth/me`, which the SPA calls on every page load, lived in the + 10/min `/api/auth/*` bucket alongside the credential endpoints. +3. Production's NodeBalancer does not preserve client addresses, so Envoy — + and therefore `X-Forwarded-For` — sees one address for all visitors. Every + per-IP bucket is effectively site-wide (cfp-live-cluster #201). + +In: spec the scoping and the new caps, implement, test. Out: PROXY protocol +on the NodeBalancer (infra, tracked separately); tightening the credential +cap back down once real client IPs arrive. + +## Implements + +- [api/conventions.md](../specs/api/conventions.md) — `## Rate limiting`: + `/api/**` only; credential endpoints enumerated; new caps and the + shared-address caveat. +- [api/auth.md](../specs/api/auth.md) — the 429 line under GitHub start + references the credential-endpoint cap instead of a stale number. + +## Approach + +- `apps/api/src/plugins/rate-limit.ts`: early return for non-`/api/` paths; + `isCredentialPath()` replaces the `/api/auth` prefix test; caps in an + exported `RATE_LIMITS` table so tests assert against the source of truth. +- Tests: assets and the SPA fallthrough are uncounted; `/api/auth/me` is a + read, not a credential call; credential paths share one IP bucket; the read + cap still trips at `RATE_LIMITS.unauthenticatedReadsPerIp + 1`. + +## Validation + +- `npm run type-check && npm run lint && npm test` clean. +- After deploy: no 429s in the pod log during normal browsing; Slack SAML test + completes. + +## Follow-ups + +- Tracked as: cfp-live-cluster #201 (PROXY protocol so per-IP is per visitor). From 1d953775dfff68d33ca5a4ac3649e3125cd031eb Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 17 Sep 2026 18:42:50 -0400 Subject: [PATCH 5/5] chore(plans): mark rate-limit-scope done (PR #179) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LFyA5poHwrhAktrnsKrUiQ --- plans/rate-limit-scope.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plans/rate-limit-scope.md b/plans/rate-limit-scope.md index 64ef0ff..fac12f2 100644 --- a/plans/rate-limit-scope.md +++ b/plans/rate-limit-scope.md @@ -1,11 +1,11 @@ --- -status: in-progress +status: done depends: [saml-login-return-path] specs: - specs/api/conventions.md - specs/api/auth.md issues: [] -pr: null +pr: 179 --- # Plan: scope and loosen the rate limiter