Round 1 gave /join a per-IP ceiling and left /recover with only its
`recover:{ip}:{name}` bucket. That key is right for the job it was written for —
stopping someone who knows a display name (they're listed on the feed) from
burning the victim's 3-strike PIN counter and locking them out on repeat. But the
name is ATTACKER-CHOSEN, so cycling names mints a fresh 5-attempt bucket every
time and the per-IP cost is unbounded.
What sits behind that limiter makes it worse than a normal flood: every call runs
a cost-12 bcrypt verify, including an UNCONDITIONAL throwaway verify for names
that don't exist — added deliberately to close a timing oracle. So an unknown name
is the single cheapest way to make the server do ~200ms of hashing.
Adds `recover_ip_rate_per_min` (default 30, migration 019), checked BEFORE the
per-name bucket so a name generator can't walk past it. 30/min is far above any
real recovery attempt while capping a flood. The per-name bucket is untouched and
remains the anti-guessing control.
The second half matters as much as the first: bcrypt was running inline on the
async runtime everywhere. At cost 12 that pins a tokio worker thread for ~200ms,
and there is only one per core — so a login flood stalled every other request on
the box, including the feed. There was no spawn_blocking anywhere in the auth
module, despite SECURITY-BACKLOG claiming bcrypt had been offloaded.
Route all of it through `verify_password` / `hash_password` on the blocking pool.
That covers /recover, /admin/login, the host PIN reset, and — the one most likely
to bite at a real event — the PIN hash minted on every single /join. Saturating
the blocking pool degrades logins; saturating the worker threads degrades
everything.
Tests: cycling distinct names from one IP now hits the ceiling with a Retry-After,
and — the assertion that keeps the fix honest — repeated wrong PINs against ONE
name are still throttled with the ceiling set generously high, so the ceiling
added protection rather than replacing it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
207 lines
8.1 KiB
TypeScript
207 lines
8.1 KiB
TypeScript
/**
|
||
* Regression guard — the door must not close on a venue behind one NAT.
|
||
*
|
||
* `/join` was throttled 5 per 60s keyed purely on the client IP. Every guest at a venue
|
||
* arrives from the same public IP (that is what a NAT is), so the whole party shared one
|
||
* bucket: 12 guests scanning the QR code within a few seconds meant 5 got in and 7 were
|
||
* turned away — with no Retry-After to tell them when to try again. `/feed` (60/min) and
|
||
* `/export` (3/DAY) had the identical defect.
|
||
*
|
||
* These ran green for the same structural reason every time: the e2e reseed forces every
|
||
* limiter toggle OFF before each test, so nothing here was ever exercised. Enable them
|
||
* explicitly, exactly as 02-upload/rate-limit does.
|
||
*/
|
||
import { test, expect } from '../../fixtures/test';
|
||
import { BASE } from '../../helpers/env';
|
||
|
||
test.describe('Rate limits — guests behind a shared NAT', () => {
|
||
test('a dozen guests can all join from one IP, and 429s carry Retry-After', async ({
|
||
api,
|
||
adminToken,
|
||
}) => {
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
join_rate_enabled: 'true',
|
||
});
|
||
|
||
// Twelve DISTINCT guests, same source IP — the arrival burst at a real party.
|
||
const names = Array.from({ length: 12 }, (_, i) => `NatGuest${i}`);
|
||
const results = await Promise.all(
|
||
names.map((display_name) =>
|
||
fetch(`${BASE}/api/v1/join`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name }),
|
||
})
|
||
)
|
||
);
|
||
|
||
const rejected = results.filter((r) => r.status === 429);
|
||
expect(
|
||
rejected.length,
|
||
`all 12 guests must get in from one IP; ${rejected.length} were turned away`
|
||
).toBe(0);
|
||
expect(results.every((r) => r.status === 201)).toBe(true);
|
||
});
|
||
|
||
test('one guest retrying their own name is still throttled, and told for how long', async ({
|
||
api,
|
||
adminToken,
|
||
}) => {
|
||
// The per-name bucket must still bite — otherwise the NAT fix would have simply
|
||
// removed the anti-spam limit rather than re-keyed it.
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
join_rate_enabled: 'true',
|
||
});
|
||
|
||
const attempt = () =>
|
||
fetch(`${BASE}/api/v1/join`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: 'RepeatOffender' }),
|
||
});
|
||
|
||
// 5 per 60s for the same (ip, name): the first succeeds (201), the next four collide
|
||
// with the taken name (409), and the sixth exhausts the bucket.
|
||
const codes: number[] = [];
|
||
for (let i = 0; i < 6; i++) codes.push((await attempt()).status);
|
||
|
||
expect(codes[0], 'the first join should succeed').toBe(201);
|
||
expect(codes.at(-1), 'the 6th attempt on one name must be throttled').toBe(429);
|
||
|
||
const throttled = await attempt();
|
||
expect(throttled.status).toBe(429);
|
||
const retryAfter = throttled.headers.get('retry-after');
|
||
expect(retryAfter, '429 must tell the client when to come back').toBeTruthy();
|
||
expect(Number(retryAfter)).toBeGreaterThan(0);
|
||
expect(Number(retryAfter)).toBeLessThanOrEqual(60);
|
||
});
|
||
|
||
test('the feed limit is per-user, not per-IP', async ({ api, adminToken, guest }) => {
|
||
// Two guests, one IP. With a limit of 3/min an IP key would let the first guest's
|
||
// three reads starve the second entirely.
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
feed_rate_enabled: 'true',
|
||
feed_rate_per_min: '3',
|
||
});
|
||
|
||
const a = await guest('FeedHog');
|
||
const b = await guest('FeedVictim');
|
||
const read = (jwt: string) =>
|
||
fetch(`${BASE}/api/v1/feed`, { headers: { Authorization: `Bearer ${jwt}` } });
|
||
|
||
// Guest A burns their whole allowance.
|
||
for (let i = 0; i < 3; i++) expect((await read(a.jwt)).status).toBe(200);
|
||
expect((await read(a.jwt)).status, "A's own 4th read is throttled").toBe(429);
|
||
|
||
// Guest B must be entirely unaffected.
|
||
expect((await read(b.jwt)).status, 'B must not inherit A’s exhausted bucket').toBe(200);
|
||
});
|
||
|
||
test('the export limit is per-user — one guest cannot spend the whole venue’s quota', async ({
|
||
api,
|
||
adminToken,
|
||
guest,
|
||
host,
|
||
db,
|
||
}) => {
|
||
// The sharpest case: 3 downloads per DAY on an IP key meant the 4th guest to fetch
|
||
// their keepsake was locked out until tomorrow.
|
||
await db.setExportReleased('e2e-test-event', true);
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
export_rate_enabled: 'true',
|
||
export_rate_per_day: '1',
|
||
});
|
||
|
||
const mintAndFetch = async (jwt: string) => {
|
||
const res = await fetch(`${BASE}/api/v1/export/ticket`, {
|
||
method: 'POST',
|
||
headers: { Authorization: `Bearer ${jwt}` },
|
||
});
|
||
const { ticket } = await res.json();
|
||
return fetch(`${BASE}/api/v1/export/zip?ticket=${encodeURIComponent(ticket)}`);
|
||
};
|
||
|
||
const a = await guest('ExportFirst');
|
||
const b = await guest('ExportSecond');
|
||
|
||
// A spends their single daily allowance. The archive itself may not exist (404) —
|
||
// what matters is that the limiter admitted the request rather than 429ing it.
|
||
expect((await mintAndFetch(a.jwt)).status).not.toBe(429);
|
||
expect((await mintAndFetch(a.jwt)).status, 'A’s second download is throttled').toBe(429);
|
||
|
||
// B shares A's IP and must still get their keepsake.
|
||
expect((await mintAndFetch(b.jwt)).status, 'B must not be locked out by A’s download').not.toBe(
|
||
429
|
||
);
|
||
|
||
// And the host too, for good measure.
|
||
expect((await mintAndFetch(host.jwt)).status).not.toBe(429);
|
||
});
|
||
});
|
||
|
||
test.describe('Rate limits — /recover name cycling', () => {
|
||
test('cycling names from one IP hits the ceiling, while one name is still throttled', async ({
|
||
api,
|
||
adminToken,
|
||
}) => {
|
||
// /recover is keyed `recover:{ip}:{name}` — right for its job (stopping someone who
|
||
// knows a display name from burning the victim's 3-strike PIN counter), but the name is
|
||
// ATTACKER-CHOSEN, so cycling names minted a fresh bucket every time. Behind it sits a
|
||
// cost-12 bcrypt verify, including an unconditional throwaway one for unknown names, so
|
||
// a name generator was the cheapest way to make the server hash forever.
|
||
//
|
||
// Squeeze the ceiling so the flood is reproducible without firing 30+ requests.
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
recover_rate_enabled: 'true',
|
||
recover_ip_rate_per_min: '5',
|
||
});
|
||
|
||
const attempt = (name: string) =>
|
||
fetch(`${BASE}/api/v1/recover`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: name, pin: '0000' }),
|
||
});
|
||
|
||
// Every name is distinct, so the per-name bucket can never fire — only the ceiling can.
|
||
const codes: number[] = [];
|
||
for (let i = 0; i < 12; i++) codes.push((await attempt(`Unbekannt${i}_${Date.now()}`)).status);
|
||
|
||
expect(
|
||
codes.filter((c) => c === 429).length,
|
||
'name cycling must be capped by the per-IP ceiling'
|
||
).toBeGreaterThan(0);
|
||
|
||
const throttled = await attempt(`Unbekannt99_${Date.now()}`);
|
||
expect(throttled.status).toBe(429);
|
||
expect(Number(throttled.headers.get('retry-after'))).toBeGreaterThan(0);
|
||
});
|
||
|
||
test('the per-name bucket still protects a real account', async ({ api, adminToken, guest }) => {
|
||
// The ceiling must not have REPLACED the anti-guessing control. With a generous ceiling,
|
||
// repeated wrong PINs against ONE name must still be shut down by the per-name bucket.
|
||
const victim = await guest('PinVictim');
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
recover_rate_enabled: 'true',
|
||
recover_ip_rate_per_min: '1000',
|
||
});
|
||
|
||
const attempt = () =>
|
||
fetch(`${BASE}/api/v1/recover`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: victim.displayName, pin: '9999' }),
|
||
});
|
||
|
||
const codes: number[] = [];
|
||
for (let i = 0; i < 7; i++) codes.push((await attempt()).status);
|
||
expect(codes.at(-1), 'guessing one name must still be throttled').toBe(429);
|
||
});
|
||
});
|