All three are the same mistake in different clothes: a limit keyed on an IP that, behind the venue's NAT, is the entire party plus the host. * join_ip_rate_per_min was raised 60 -> 300 last round and it never took effect. A config default is only a fallback for a MISSING key, and migration 017 seeds this one, so the seed won and the raise was dead code on every real install. Migration 030 raises the seeded value the way 015 already did for upload_rate_per_hour. The e2e guard could not see this: it fires 12 concurrent joins, which is green at 60 and at 300 alike. * /recover's per-(IP, name) bucket charged EVERY request, including successful ones, and refused before verifying the PIN. Its ceiling clamps to 4. So four POSTs naming "Braut Sophie" with PIN 0000, from any phone on the venue wifi, locked Sophie out of her own recovery for fifteen minutes WITH THE CORRECT PIN — and four more every fifteen minutes sustained it indefinitely, at a rate far under every volume ceiling above it. The benign version needs no attacker: the host mistypes their own PIN four times. Hosts are promoted guests whose only credential is that PIN, and /recover is their only way back after losing a session. Now it counts failures, and a spent budget changes what a FAILURE answers instead of refusing outright. Guessing is bounded exactly as before — wrong PINs are what spend it — with the per-account lockout underneath. * /admin/login's pre-verify ceiling had the same shape, and the escape hatch was circular: admin_login_rate_enabled is only flippable through PATCH /admin/config, which needs the session being refused. One phone posting twice a minute cost the operator moderation, gallery release and every config key, including the ones that would undo it. Exceeding the ceiling now shortens the hash-permit wait rather than refusing: the CPU bound was always the semaphore, never this bucket, so a flood still sheds itself while a correct password gets a truthful answer. Adds a regression test that reads the value a fresh database actually ends up with, by replaying the migrations — the drift that made the first bullet invisible is not otherwise detectable from the code.
318 lines
14 KiB
TypeScript
318 lines
14 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';
|
||
import { seedUpload } from '../../helpers/seed';
|
||
|
||
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('one guest sweeping /recover cannot lock the venue — or the host — out of PIN recovery', async ({
|
||
api,
|
||
adminToken,
|
||
guest,
|
||
}) => {
|
||
// The sharpest version of this file's whole premise. `/recover` has a cross-name failure
|
||
// budget keyed on IP, meant to catch someone sweeping the public name list. Behind the venue
|
||
// NAT that budget is SHARED BY THE ENTIRE PARTY, and it used to be checked before the account
|
||
// was even looked up — so it refused a correct PIN.
|
||
//
|
||
// That is the host's problem specifically: hosts are promoted guests whose only credential is
|
||
// a 4-digit PIN, so /recover is their only way back in after losing a session. A guest posting
|
||
// invented names could deny it to everyone, indefinitely, for the price of ~2 requests/minute.
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
recover_rate_enabled: 'true',
|
||
// Raise the per-IP VOLUME ceiling out of the way. It defaults to 30/min, and the
|
||
// cross-name FAILURE budget under test is also 30 — so the sweep below would trip the
|
||
// volume limiter first and this test would pass for the wrong reason (a 429 that proves
|
||
// nothing about whether a correct PIN survives a spent failure budget).
|
||
recover_ip_rate_per_min: '500',
|
||
});
|
||
|
||
const victim = await guest('RecoverVictim');
|
||
|
||
// Burn the shared per-IP budget with names that do not exist — the cheapest sweep, and the
|
||
// one that needs no knowledge of the guest list at all.
|
||
for (let i = 0; i < 35; i++) {
|
||
await fetch(`${BASE}/api/v1/recover`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: `Ghost${i}-${Date.now()}`, pin: '0000' }),
|
||
});
|
||
}
|
||
|
||
// A real guest, on that same IP, with their REAL PIN, must still get in.
|
||
const res = await fetch(`${BASE}/api/v1/recover`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: 'RecoverVictim', pin: victim.pin }),
|
||
});
|
||
expect(
|
||
res.status,
|
||
'a correct PIN must survive a spent cross-name budget — otherwise any guest can lock the ' +
|
||
'host out of the only login path they have'
|
||
).toBe(200);
|
||
|
||
// ...and the sweep is still answered as a sweep: a WRONG pin gets 429, not a bare 401, so the
|
||
// budget still does its job on the traffic it was built for.
|
||
const wrong = await fetch(`${BASE}/api/v1/recover`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: 'RecoverVictim', pin: '0001' }),
|
||
});
|
||
expect(wrong.status, 'wrong PINs from an exhausted IP are still throttled').toBe(429);
|
||
});
|
||
|
||
test('the export limit is per-user — one guest cannot spend the whole venue’s quota', async ({
|
||
api,
|
||
adminToken,
|
||
guest,
|
||
host,
|
||
}) => {
|
||
// The sharpest case: 3 downloads per DAY on an IP key meant the 4th guest to fetch
|
||
// their keepsake was locked out until tomorrow.
|
||
//
|
||
// A REAL release, not `setExportReleased`. `/export/ticket` pre-validates that the archive is
|
||
// actually servable and answers 404 without charging the limiter — deliberately, so a guest
|
||
// never spends one of their three daily downloads on an archive that cannot be served. With
|
||
// only the released FLAG set and no archive on disk, every mint here 404'd and the per-day
|
||
// limiter under test was never reached at all.
|
||
await seedUpload(host.jwt, { caption: 'for the keepsake' });
|
||
expect(
|
||
(
|
||
await fetch(`${BASE}/api/v1/host/gallery/release`, {
|
||
method: 'POST',
|
||
headers: { Authorization: `Bearer ${host.jwt}` },
|
||
})
|
||
).status
|
||
).toBe(204);
|
||
await expect
|
||
.poll(
|
||
async () => {
|
||
const s = await (
|
||
await fetch(`${BASE}/api/v1/export/status`, {
|
||
headers: { Authorization: `Bearer ${host.jwt}` },
|
||
})
|
||
).json();
|
||
return s.released === true && s.zip?.status === 'done';
|
||
},
|
||
{ timeout: 90_000, intervals: [500] }
|
||
)
|
||
.toBe(true);
|
||
|
||
await api.patchConfig(adminToken, {
|
||
rate_limits_enabled: 'true',
|
||
export_rate_enabled: 'true',
|
||
export_rate_per_day: '1',
|
||
});
|
||
|
||
// The per-day export limit is charged at the MINT, not at the download: the ticket endpoint is
|
||
// the authenticated chokepoint, while `/export/zip` authenticates by ticket alone so a resumed
|
||
// transfer doesn't spend another of the guest's daily allowance. So a throttled guest is
|
||
// refused with 429 at `/export/ticket` and never reaches the archive.
|
||
//
|
||
// This helper used to destructure `ticket` from that 429 body regardless, then fetch with
|
||
// `ticket=undefined` — turning the 429 under test into an unrelated 401 from the download
|
||
// endpoint. Surface the mint's refusal instead; that IS the throttle.
|
||
const mintAndFetch = async (jwt: string) => {
|
||
const minted = await fetch(`${BASE}/api/v1/export/ticket?kind=zip`, {
|
||
method: 'POST',
|
||
headers: { Authorization: `Bearer ${jwt}` },
|
||
});
|
||
if (!minted.ok) return minted;
|
||
const { ticket } = await minted.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 — on a real archive, so this is a genuine 200 rather
|
||
// than merely "not 429", which would have been satisfied by any error at all.
|
||
expect((await mintAndFetch(a.jwt)).status, 'A’s first download must succeed').toBe(200);
|
||
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').toBe(
|
||
200
|
||
);
|
||
|
||
// And the host too, for good measure.
|
||
expect((await mintAndFetch(host.jwt)).status).toBe(200);
|
||
});
|
||
});
|
||
|
||
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);
|
||
|
||
// AND THE OTHER HALF, which is the whole reason this bucket counts failures instead of
|
||
// requests: the victim's own CORRECT PIN must still let them in, from the same IP, while that
|
||
// budget is spent. Behind the venue's NAT the "attacker" and the victim are the same address,
|
||
// so a bucket that refused before verifying handed any guest a fifteen-minute lockout of any
|
||
// named person — the host included, whose only credential is a 4-digit PIN and whose only way
|
||
// back after losing a session is this endpoint. Four wrong guesses did it, and four more every
|
||
// fifteen minutes sustained it indefinitely.
|
||
const rightful = await fetch(`${BASE}/api/v1/recover`, {
|
||
method: 'POST',
|
||
headers: { 'Content-Type': 'application/json' },
|
||
body: JSON.stringify({ display_name: victim.displayName, pin: victim.pin }),
|
||
});
|
||
expect(
|
||
rightful.status,
|
||
'a correct PIN must authenticate even when this IP has spent the name budget'
|
||
).toBe(200);
|
||
});
|
||
});
|