Skip to content

Commit ae7ff3a

Browse files
Merge pull request #180 from CodeForPhilly/develop
Release: v0.2.3
2 parents ca6af00 + c3f052c commit ae7ff3a

8 files changed

Lines changed: 265 additions & 45 deletions

File tree

‎apps/api/src/app.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@
1313
* is built from the post-reconciliation tree)
1414
* 6b. push-daemon plugin → starts gitsheets push daemon
1515
* 6c. services plugin → builds in-memory state + FTS
16-
* 7. rate-limit plugin → in-memory counters keyed per-IP + per-account
17-
* 8. idempotency plugin → in-memory map keyed by personId+key
16+
* 7. session middleware → parses the JWT cookie into request.session
17+
* 8. rate-limit plugin → in-memory counters keyed per-IP + per-account
18+
* 8a. idempotency plugin → in-memory map keyed by personId+key
1819
* 9. @fastify/swagger → OpenAPI 3.1 doc generation
1920
* 10. @fastify/swagger-ui → Swagger UI at /api/_docs
2021
* 11. routes → registered last after all plumbing
@@ -151,15 +152,17 @@ export async function buildApp(opts: BuildAppOptions = {}): Promise<FastifyInsta
151152
await fastify.register(slugRedirectPlugin);
152153
await fastify.register(legacyRedirectPlugin);
153154

154-
// ----- 7. Rate limiting -----
155+
// ----- 7. Session middleware (JWT auth) -----
156+
// Before rate-limit and idempotency: all three use onRequest hooks, which run
157+
// in registration order, and the other two key on request.session.person.
158+
await fastify.register(sessionMiddlewarePlugin);
159+
160+
// ----- 8. Rate limiting -----
155161
await fastify.register(rateLimitPlugin);
156162

157-
// ----- 8. Idempotency -----
163+
// ----- 8a. Idempotency -----
158164
await fastify.register(idempotencyPlugin);
159165

160-
// ----- 8a. Session middleware (JWT auth) -----
161-
await fastify.register(sessionMiddlewarePlugin);
162-
163166
// ----- 9-10. OpenAPI / Swagger UI -----
164167
await fastify.register(fastifySwagger, {
165168
openapi: {

‎apps/api/src/plugins/rate-limit.ts‎

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,18 @@
22
* In-memory rate-limit plugin.
33
*
44
* Enforces per-IP and per-account caps per specs/api/conventions.md#rate-limiting:
5-
* - Unauthenticated reads: 60 req / min / IP
6-
* - Authenticated reads: 300 req / min / account
7-
* - Writes: 30 req / min / account
8-
* - Auth endpoints: 10 req / min / IP
5+
* - Unauthenticated reads: 1200 req / min / IP
6+
* - Authenticated reads: 300 req / min / account
7+
* - Writes: 30 req / min / account (120 / min / IP anonymous)
8+
* - Credential endpoints: 120 req / min / IP
9+
*
10+
* Only `/api/**` is counted. The SPA shell, its assets, and thumbnails come out
11+
* of the same process and a single page load fetches dozens of them; counting
12+
* those against the read cap is what produced a site-wide 429 storm at cutover.
13+
*
14+
* Per-IP caps are generous on purpose: production's load balancer does not yet
15+
* preserve client addresses (cfp-live-cluster #201), so every visitor shares
16+
* one "IP" until that lands.
917
*
1018
* Counters are reset on restart (intentional — single replica, civic scale).
1119
* Exceeded limit → RateLimitedError(retryAfterSeconds).
@@ -23,6 +31,14 @@ interface BucketEntry {
2331

2432
const WINDOW_MS = 60_000; // 1 minute
2533

34+
export const RATE_LIMITS = {
35+
unauthenticatedReadsPerIp: 1200,
36+
authenticatedReadsPerAccount: 300,
37+
writesPerAccount: 30,
38+
anonymousWritesPerIp: 120,
39+
credentialPerIp: 120,
40+
} as const;
41+
2642
function getOrCreate(map: Map<string, BucketEntry>, key: string): BucketEntry {
2743
let entry = map.get(key);
2844
if (!entry) {
@@ -60,37 +76,60 @@ function clientIp(request: FastifyRequest): string {
6076
}
6177

6278
const WRITE_METHODS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']);
63-
const AUTH_PATH_PREFIX = '/api/auth';
79+
const API_PREFIX = '/api/';
80+
81+
/**
82+
* Routes that accept or mint a credential. Session reads (`/api/auth/me`,
83+
* `/api/auth/refresh`, `/api/auth/sessions`, `/api/auth/logout`) are ordinary
84+
* traffic — `/me` runs on every page load.
85+
*/
86+
const CREDENTIAL_PATHS = [
87+
'/api/auth/login',
88+
'/api/auth/github/start',
89+
'/api/auth/github/callback',
90+
'/api/auth/link-github',
91+
'/api/auth/password-reset/',
92+
'/api/account-claim/by-password',
93+
];
94+
95+
function pathOf(url: string): string {
96+
const q = url.indexOf('?');
97+
return q === -1 ? url : url.slice(0, q);
98+
}
99+
100+
export function isCredentialPath(path: string): boolean {
101+
return CREDENTIAL_PATHS.some((p) => (p.endsWith('/') ? path.startsWith(p) : path === p));
102+
}
64103

65104
async function rateLimitPlugin(fastify: FastifyInstance): Promise<void> {
66105
const ipBuckets = new Map<string, BucketEntry>();
67106
const accountBuckets = new Map<string, BucketEntry>();
68107

69108
fastify.addHook('onRequest', (request, _reply, done) => {
70109
try {
110+
const path = pathOf(request.url);
111+
if (!path.startsWith(API_PREFIX)) {
112+
done();
113+
return;
114+
}
115+
71116
const ip = clientIp(request);
72117
const isWrite = WRITE_METHODS.has(request.method);
73-
const isAuthEndpoint = request.url.startsWith(AUTH_PATH_PREFIX);
74-
75118
const personId = request.session?.person?.id;
76119

77-
if (isAuthEndpoint) {
78-
// Auth endpoints: 10 req / min / IP
79-
check(ipBuckets, `auth:${ip}`, 10);
120+
if (isCredentialPath(path)) {
121+
check(ipBuckets, `credential:${ip}`, RATE_LIMITS.credentialPerIp);
80122
} else if (isWrite) {
81123
if (personId) {
82-
// Authenticated writes: 30 req / min / account
83-
check(accountBuckets, `write-account:${personId}`, 30);
124+
check(accountBuckets, `write-account:${personId}`, RATE_LIMITS.writesPerAccount);
84125
} else {
85-
check(ipBuckets, `write:${ip}`, 30);
126+
check(ipBuckets, `write:${ip}`, RATE_LIMITS.anonymousWritesPerIp);
86127
}
87128
} else {
88129
if (personId) {
89-
// Authenticated reads: 300 req / min / account
90-
check(accountBuckets, `account:${personId}`, 300);
130+
check(accountBuckets, `account:${personId}`, RATE_LIMITS.authenticatedReadsPerAccount);
91131
} else {
92-
// Unauthenticated reads: 60 req / min / IP
93-
check(ipBuckets, `read:${ip}`, 60);
132+
check(ipBuckets, `read:${ip}`, RATE_LIMITS.unauthenticatedReadsPerIp);
94133
}
95134
}
96135
} catch (err) {

‎apps/api/tests/api-skeleton.test.ts‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* - ValidationError surfaces as 422 validation_failed with expected shape
77
* - Unknown Error surfaces as 500 internal_error with no message leak
88
* - traceId appears in error responses
9-
* - Per-IP rate limit: 61 anonymous reads → 429 with Retry-After
9+
* - Per-IP rate limit: one read past the cap → 429 with Retry-After
1010
* - Idempotency-Key: repeat POST returns cached response
1111
* - /api/_openapi.json returns a valid OpenAPI 3.1 document
1212
* - /api/_docs renders (200 response)
@@ -16,6 +16,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest';
1616

1717
import type { FastifyInstance } from 'fastify';
1818
import { buildApp } from '../src/app.js';
19+
import { RATE_LIMITS } from '../src/plugins/rate-limit.js';
1920
import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js';
2021

2122
// ---------------------------------------------------------------------------
@@ -169,18 +170,19 @@ describe('/api/_test/* route gating', () => {
169170
// ---------------------------------------------------------------------------
170171

171172
describe('rate limiting', () => {
172-
it('61 anonymous reads from the same IP → 429 with Retry-After on the 61st', async () => {
173-
// Make 60 reads — all should succeed
174-
for (let i = 0; i < 60; i++) {
175-
const res = await app!.inject({
176-
method: 'GET',
177-
url: '/api/health',
178-
remoteAddress: '10.0.0.1',
179-
});
180-
expect(res.statusCode, `Request ${i + 1} should succeed`).toBe(200);
181-
}
173+
it('one read past the per-IP cap → 429 with Retry-After', async () => {
174+
// Prime the bucket to the cap rather than issuing 1200 requests; the last
175+
// in-cap read must still succeed.
176+
const limit = RATE_LIMITS.unauthenticatedReadsPerIp;
177+
app!.rateLimitBuckets.ip.set('read:10.0.0.1', { count: limit - 1, windowStart: Date.now() });
178+
const okRes = await app!.inject({
179+
method: 'GET',
180+
url: '/api/health',
181+
remoteAddress: '10.0.0.1',
182+
});
183+
expect(okRes.statusCode).toBe(200);
182184

183-
// The 61st should be rate-limited
185+
// The next one should be rate-limited
184186
const res = await app!.inject({
185187
method: 'GET',
186188
url: '/api/health',

‎apps/api/tests/auth.test.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { type FastifyInstance } from 'fastify';
2222
import { SignJWT } from 'jose';
2323

2424
import { buildApp } from '../src/app.js';
25+
import { RATE_LIMITS } from '../src/plugins/rate-limit.js';
2526
import { mintSessionFor } from '../src/auth/issue.js';
2627
import { verifyAccess, verifyRefresh } from '../src/auth/jwt.js';
2728
import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js';
@@ -499,12 +500,13 @@ describe('account-based rate limits', () => {
499500
it('authenticated reads key on account bucket (300/min), separate from IP bucket', async () => {
500501
const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY);
501502

502-
// Exhaust the IP bucket with anonymous reads (60 limit)
503-
for (let i = 0; i < 60; i++) {
504-
await app.inject({ method: 'GET', url: '/api/health', remoteAddress: '10.99.0.1' });
505-
}
503+
// Exhaust the IP bucket (prime it to the cap rather than issuing 1200 reads)
504+
app.rateLimitBuckets.ip.set('read:10.99.0.1', {
505+
count: RATE_LIMITS.unauthenticatedReadsPerIp,
506+
windowStart: Date.now(),
507+
});
506508

507-
// 61st anonymous request → 429
509+
// Next anonymous request → 429
508510
const anonRes = await app.inject({
509511
method: 'GET',
510512
url: '/api/health',

‎apps/api/tests/rate-limit.test.ts‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/**
2+
* Rate limiter scope — specs/api/conventions.md#rate-limiting.
3+
*
4+
* Covers what broke at cutover:
5+
* - non-/api requests (SPA shell, assets, thumbnails) are never counted
6+
* - GET /api/auth/me is an ordinary read, not a credential call
7+
* - credential endpoints share one per-IP bucket at RATE_LIMITS.credentialPerIp
8+
* - the unauthenticated read cap still trips one past RATE_LIMITS.unauthenticatedReadsPerIp
9+
*/
10+
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
11+
import type { FastifyInstance } from 'fastify';
12+
import { buildApp } from '../src/app.js';
13+
import { RATE_LIMITS, isCredentialPath } from '../src/plugins/rate-limit.js';
14+
import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js';
15+
16+
let dataRepo: { path: string; cleanup: () => Promise<void> };
17+
let privateStore: { path: string; cleanup: () => Promise<void> };
18+
let app: FastifyInstance | undefined;
19+
20+
const IP = '203.0.113.7';
21+
22+
beforeEach(async () => {
23+
dataRepo = await createFullDataRepo();
24+
privateStore = await createPrivateStorageDir();
25+
app = await buildApp({
26+
serverOptions: { logger: false },
27+
overrideEnv: {
28+
CFP_DATA_REPO_PATH: dataRepo.path,
29+
STORAGE_BACKEND: 'filesystem',
30+
CFP_PRIVATE_STORAGE_PATH: privateStore.path,
31+
CFP_JWT_SIGNING_KEY: 'test-jwt-signing-key-at-least-32-chars!!',
32+
NODE_ENV: 'test',
33+
},
34+
});
35+
});
36+
37+
afterEach(async () => {
38+
if (app) {
39+
await app.close();
40+
app = undefined;
41+
}
42+
await dataRepo.cleanup();
43+
await privateStore.cleanup();
44+
});
45+
46+
function bucket(key: string): number {
47+
return app!.rateLimitBuckets.ip.get(key)?.count ?? 0;
48+
}
49+
50+
describe('rate limiter scope', () => {
51+
it('does not count non-/api requests (SPA shell, assets, thumbnails)', async () => {
52+
for (const url of ['/', '/login', '/assets/index-abc123.js', '/thumbnail/1/100x100']) {
53+
const res = await app!.inject({ method: 'GET', url, headers: { 'x-forwarded-for': IP } });
54+
expect(res.statusCode).not.toBe(429);
55+
}
56+
expect(app!.rateLimitBuckets.ip.size).toBe(0);
57+
});
58+
59+
it('treats GET /api/auth/me as a read, not a credential call', async () => {
60+
const res = await app!.inject({
61+
method: 'GET',
62+
url: '/api/auth/me',
63+
headers: { 'x-forwarded-for': IP },
64+
});
65+
expect(res.statusCode).toBe(200);
66+
expect(bucket(`read:${IP}`)).toBe(1);
67+
expect(bucket(`credential:${IP}`)).toBe(0);
68+
});
69+
70+
it('classifies credential endpoints', () => {
71+
for (const p of [
72+
'/api/auth/login',
73+
'/api/auth/github/start',
74+
'/api/auth/github/callback',
75+
'/api/auth/link-github',
76+
'/api/auth/password-reset/request',
77+
'/api/auth/password-reset/confirm',
78+
'/api/account-claim/by-password',
79+
]) {
80+
expect(isCredentialPath(p), p).toBe(true);
81+
}
82+
for (const p of ['/api/auth/me', '/api/auth/refresh', '/api/auth/sessions', '/api/auth/logout', '/api/people']) {
83+
expect(isCredentialPath(p), p).toBe(false);
84+
}
85+
});
86+
87+
it('caps credential endpoints per IP at RATE_LIMITS.credentialPerIp', async () => {
88+
const limit = RATE_LIMITS.credentialPerIp;
89+
let last = 0;
90+
for (let i = 0; i < limit + 1; i++) {
91+
const res = await app!.inject({
92+
method: 'POST',
93+
url: '/api/auth/login',
94+
headers: { 'x-forwarded-for': IP, 'content-type': 'application/json' },
95+
payload: {},
96+
});
97+
last = res.statusCode;
98+
if (i < limit) expect(res.statusCode).not.toBe(429);
99+
}
100+
expect(last).toBe(429);
101+
expect(bucket(`read:${IP}`)).toBe(0);
102+
});
103+
104+
it('caps unauthenticated reads per IP one past RATE_LIMITS.unauthenticatedReadsPerIp', async () => {
105+
const limit = RATE_LIMITS.unauthenticatedReadsPerIp;
106+
// Prime the bucket directly rather than issuing 1200 requests.
107+
app!.rateLimitBuckets.ip.set(`read:${IP}`, { count: limit - 1, windowStart: Date.now() });
108+
const ok = await app!.inject({ method: 'GET', url: '/api/health', headers: { 'x-forwarded-for': IP } });
109+
expect(ok.statusCode).toBe(200);
110+
const over = await app!.inject({ method: 'GET', url: '/api/health', headers: { 'x-forwarded-for': IP } });
111+
expect(over.statusCode).toBe(429);
112+
expect(over.headers['retry-after']).toBeDefined();
113+
});
114+
});

‎plans/rate-limit-scope.md‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
status: done
3+
depends: [saml-login-return-path]
4+
specs:
5+
- specs/api/conventions.md
6+
- specs/api/auth.md
7+
issues: []
8+
pr: 179
9+
---
10+
11+
# Plan: scope and loosen the rate limiter
12+
13+
## Scope
14+
15+
Minutes after the 2026-09-17 cutover, visitors (including the Slack SAML
16+
test) started getting `429 rate_limited`. Three compounding causes:
17+
18+
1. The limiter's `onRequest` hook counted **every** request — SPA shell,
19+
JS/CSS chunks, thumbnails — against the 60/min unauthenticated read cap.
20+
One page load is dozens of requests.
21+
2. `GET /api/auth/me`, which the SPA calls on every page load, lived in the
22+
10/min `/api/auth/*` bucket alongside the credential endpoints.
23+
3. Production's NodeBalancer does not preserve client addresses, so Envoy —
24+
and therefore `X-Forwarded-For` — sees one address for all visitors. Every
25+
per-IP bucket is effectively site-wide (cfp-live-cluster #201).
26+
27+
In: spec the scoping and the new caps, implement, test. Out: PROXY protocol
28+
on the NodeBalancer (infra, tracked separately); tightening the credential
29+
cap back down once real client IPs arrive.
30+
31+
## Implements
32+
33+
- [api/conventions.md](../specs/api/conventions.md) — `## Rate limiting`:
34+
`/api/**` only; credential endpoints enumerated; new caps and the
35+
shared-address caveat.
36+
- [api/auth.md](../specs/api/auth.md) — the 429 line under GitHub start
37+
references the credential-endpoint cap instead of a stale number.
38+
39+
## Approach
40+
41+
- `apps/api/src/plugins/rate-limit.ts`: early return for non-`/api/` paths;
42+
`isCredentialPath()` replaces the `/api/auth` prefix test; caps in an
43+
exported `RATE_LIMITS` table so tests assert against the source of truth.
44+
- Tests: assets and the SPA fallthrough are uncounted; `/api/auth/me` is a
45+
read, not a credential call; credential paths share one IP bucket; the read
46+
cap still trips at `RATE_LIMITS.unauthenticatedReadsPerIp + 1`.
47+
48+
## Validation
49+
50+
- `npm run type-check && npm run lint && npm test` clean.
51+
- After deploy: no 429s in the pod log during normal browsing; Slack SAML test
52+
completes.
53+
54+
## Follow-ups
55+
56+
- Tracked as: cfp-live-cluster #201 (PROXY protocol so per-IP is per visitor).

0 commit comments

Comments
 (0)