fix(rate-limit): key the guest-facing limiters per user, not per IP
At a venue every guest is behind one NAT, so an IP-keyed limiter hands the
whole party a single bucket. On a fresh deploy 12 guests arriving together
meant 5 joined and 7 were turned away, with no Retry-After telling them when
to retry. `/feed` (60/min) and `/export` (3 per DAY — the fourth guest to
fetch their keepsake locked out until tomorrow) had the same defect.
`feed_delta` was already keyed per-user and its comment states the exact
rationale ("so one client can't starve others behind a shared NAT"); this
makes its siblings match.
- feed:{ip} -> feed:{user_id} (auth was already in scope)
- export:{ip} -> export:{user_id} (resolved from the download ticket's
session, which was previously looked up and discarded)
- join:{ip}: pre-auth, so there is no user to key on. Split in two — a loose
per-IP ceiling that only bounds raw volume (new `join_ip_rate_per_min`,
default 60, migration 017), plus the real 5/60s anti-spam bucket keyed
per (ip, name), mirroring the existing `recover:{ip}:{name}`.
admin_login / recover / pin_reset_req stay IP-keyed on purpose and are now
commented as such: they guard credential guessing, where a per-user or
per-name key would just hand an attacker a fresh bucket per guess.
Retry-After: the machinery existed but 7 of 8 sites called `check()` and
hard-coded `None`, so a throttled client was told to back off but never for
how long. Delete the bool `check()` wrapper entirely so `check_with_retry`
is the only entry point and the delay cannot be discarded by accident. Also
surface it for the PIN lockout, where the deadline was already known.
Fix the "unknown" fallback while here: every client_ip() caller passed that
literal, so any request without X-Forwarded-For — anything reaching the app
directly rather than through Caddy — shared ONE global bucket. Serve with
connect-info and use the peer address.
Tests: the reseed forces every limiter toggle off before each test, which is
why this whole class was invisible. Add 01-auth/rate-limit-shared-nat, which
enables them and asserts 12 guests share an IP without collision, that one
guest hammering their own name IS still throttled (so the fix re-keys rather
than removes the limit), and that feed/export buckets are per-user. Retarget
the ddos join test at the new per-IP ceiling — it asserted the defect.
Also seed `admin_login_rate_enabled` (read by the handler, seeded by no
migration and no reseed) and register `join_ip_rate_per_min` in the admin
config allowlist. Unrelated pre-existing red test fixed: 01-auth/join
asserted a "Willkommen!" heading the wedding redesign removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -13,7 +13,11 @@ test.describe('Auth — join flow', () => {
|
||||
const join = new JoinPage(page);
|
||||
await join.goto();
|
||||
|
||||
await expect(page.getByRole('heading', { name: 'Willkommen!' })).toBeVisible();
|
||||
// The join form's landing state. There is no "Willkommen!" heading — the wedding
|
||||
// redesign (f243bfe) split it into a "Willkommen bei" lead-in plus the event name as
|
||||
// the <h1>, and this assertion was never updated, so it had been failing since.
|
||||
// Anchor on the testid the markup provides rather than on copy.
|
||||
await expect(page.getByTestId('join-event-name')).toBeVisible();
|
||||
|
||||
const { pin } = await join.joinAs('Alice');
|
||||
expect(pin).toMatch(/^\d{4}$/);
|
||||
|
||||
144
e2e/specs/01-auth/rate-limit-shared-nat.spec.ts
Normal file
144
e2e/specs/01-auth/rate-limit-shared-nat.spec.ts
Normal file
@@ -0,0 +1,144 @@
|
||||
/**
|
||||
* 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);
|
||||
});
|
||||
});
|
||||
@@ -15,7 +15,15 @@ test.describe('Adversarial — small-scale abuse', () => {
|
||||
await api.patchConfig(adminToken, { rate_limits_enabled: 'true', join_rate_enabled: 'true' });
|
||||
});
|
||||
|
||||
test('20 parallel /join from one IP — rate limiter catches the excess', async () => {
|
||||
test('a /join flood from one IP is caught by the per-IP ceiling', async ({ api, adminToken }) => {
|
||||
// This used to assert that 20 joins from one IP produced 429s under a 5/min per-IP
|
||||
// bucket. That "protection" was the bug: at a venue every guest shares one public IP,
|
||||
// so it turned real arriving guests away (see 01-auth/rate-limit-shared-nat). The
|
||||
// anti-spam bucket is now per (ip, name); what remains per-IP is a loose ceiling whose
|
||||
// job is only to bound raw volume. Squeeze the ceiling so a flood is reproducible here
|
||||
// without firing 60+ requests.
|
||||
await api.patchConfig(adminToken, { join_ip_rate_per_min: '5' });
|
||||
|
||||
const requests = Array.from({ length: 20 }, (_, i) =>
|
||||
fetch(`${BASE}/api/v1/join`, {
|
||||
method: 'POST',
|
||||
@@ -24,7 +32,7 @@ test.describe('Adversarial — small-scale abuse', () => {
|
||||
})
|
||||
);
|
||||
const statuses = (await Promise.all(requests)).map((r) => r.status);
|
||||
// 5/min limit → at least some should be 429.
|
||||
// Ceiling of 5 → the excess must be shed.
|
||||
expect(statuses.filter((s) => s === 429).length).toBeGreaterThan(0);
|
||||
// Server stays up — at least one succeeded.
|
||||
expect(statuses.some((s) => s === 201 || s === 409)).toBe(true);
|
||||
|
||||
Reference in New Issue
Block a user