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>
This commit is contained in:
@@ -142,3 +142,65 @@ test.describe('Rate limits — guests behind a shared NAT', () => {
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user