fix(auth): three ways one guest on the venue NAT could lock everyone else out
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.
This commit is contained in:
@@ -296,5 +296,22 @@ test.describe('Rate limits — /recover name cycling', () => {
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user