Files
EventSnap/e2e/specs/01-auth/rate-limit-shared-nat.spec.ts
fabi 6920e5bf7a fix(recover): cap name cycling, and stop bcrypt blocking the runtime
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>
2026-07-28 20:54:58 +02:00

207 lines
8.1 KiB
TypeScript
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
/**
* 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 As exhausted bucket').toBe(200);
});
test('the export limit is per-user — one guest cannot spend the whole venues 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, 'As 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 As 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);
});
});