diff --git a/e2e/fixtures/db.ts b/e2e/fixtures/db.ts index 223e9a1..abde3a6 100644 --- a/e2e/fixtures/db.ts +++ b/e2e/fixtures/db.ts @@ -37,6 +37,52 @@ export const db = { ); }, + /** + * Is this user's account currently PIN-locked? + * + * Distinguishes the two ways /recover can answer 429 — the per-(IP, name) throttle, which + * costs the attacker, and the account lock, which costs the VICTIM. Only the second one is + * weaponizable, so a test asserting "a single IP cannot lock a guest out" has to look at the + * row, not at the status code. + */ + async isPinLocked(userId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ locked: boolean }>( + `SELECT (pin_locked_until IS NOT NULL AND pin_locked_until > NOW()) AS locked + FROM "user" WHERE id = $1`, + [userId] + ); + return r.rows[0]?.locked ?? false; + }); + }, + + /** + * Preload the wrong-PIN streak, standing in for failures that arrived from other IPs. + * + * The account lock is deliberately out of reach of any single source, so a test that wants to + * exercise it has to simulate the distributed case rather than hammer from one address. + * `last_failed_pin_at` is set to now so the 15-minute decay does not immediately reset it. + */ + async setFailedPinAttempts(userId: string, attempts: number) { + await withClient((c) => + c.query( + `UPDATE "user" SET failed_pin_attempts = $2, last_failed_pin_at = NOW() WHERE id = $1`, + [userId, attempts] + ) + ); + }, + + /** Current wrong-PIN streak. Decays after 15 minutes — see User::increment_failed_pin. */ + async failedPinAttempts(userId: string): Promise { + return withClient(async (c) => { + const r = await c.query<{ failed_pin_attempts: number }>( + `SELECT failed_pin_attempts FROM "user" WHERE id = $1`, + [userId] + ); + return r.rows[0]?.failed_pin_attempts ?? 0; + }); + }, + async expireSession(userId: string) { await withClient((c) => c.query(`UPDATE session SET expires_at = NOW() - interval '1 hour' WHERE user_id = $1`, [ diff --git a/e2e/specs/01-auth/join.spec.ts b/e2e/specs/01-auth/join.spec.ts index c5eadb9..fb02ac2 100644 --- a/e2e/specs/01-auth/join.spec.ts +++ b/e2e/specs/01-auth/join.spec.ts @@ -75,7 +75,11 @@ test.describe('Auth — join flow', () => { expect(storage.pin).toBe(original.pin); }); - test('wrong PIN three times locks the account for 15 minutes', async ({ page, guest, db }) => { + test('repeated wrong PINs are throttled without locking the guest out', async ({ + page, + guest, + db, + }) => { const dave = await guest('Dave'); await clearAllStorage(page); @@ -85,22 +89,33 @@ test.describe('Auth — join flow', () => { await join.submit(); await expect(join.recoveryPinInput).toBeVisible(); - // Wrong PIN (real one is dave.pin) + // Wrong PIN (real one is dave.pin), four times — one more than the OLD lock threshold of 3. + // Typed digit by digit so the 4th character auto-submits (see pin-auto-submit.spec.ts); + // clicking as well would double-submit and race the disabled state of the button. const wrong = dave.pin === '0000' ? '1111' : '0000'; - for (let i = 0; i < 3; i++) { - await join.recoveryPinInput.fill(wrong); - await join.recoverySubmit.click(); + for (let i = 0; i < 4; i++) { + await join.recoveryPinInput.fill(''); + await join.recoveryPinInput.pressSequentially(wrong, { delay: 30 }); await expect(join.recoveryError).toBeVisible(); + await expect(join.recoverySubmit).toBeEnabled(); } - // Fourth attempt should hit the 429 lockout (even with the correct PIN now) - await join.recoveryPinInput.fill(dave.pin); - await join.recoverySubmit.click(); - await expect(join.recoveryError).toContainText(/15 Minuten/); + // THE PROPERTY THIS TEST EXISTS FOR, stated the way a guest experiences it: Dave can still + // get into his own account. + // + // The lock threshold used to be 3, BELOW the per-(IP, name) ceiling — so these very + // keystrokes locked Dave out for 15 minutes, and anyone who can read his name off the feed + // could do it to him on repeat. Rate limits are disabled in this environment (see + // config `rate_limits_enabled`), so what is exercised here is purely the account-lock tier; + // the throttle tier is covered in 07-adversarial/auth-tampering.spec.ts. + expect( + await db.isPinLocked(dave.userId), + 'four wrong PINs from one device must not lock a guest out of their own account' + ).toBe(false); - // Sanity: DB row reflects the lock - // (The handler sets pin_locked_until directly — verify via API "recover" returning 429) - void db; // unused for now, documenting that db.lockUserPin exists if we want shortcut path + await join.recoveryPinInput.fill(''); + await join.recoveryPinInput.pressSequentially(dave.pin, { delay: 30 }); + await page.waitForURL('**/feed'); }); test('"Anderen Namen wählen" returns to the normal join form', async ({ page, guest }) => { diff --git a/e2e/specs/07-adversarial/auth-tampering.spec.ts b/e2e/specs/07-adversarial/auth-tampering.spec.ts index 69e7f05..a076434 100644 --- a/e2e/specs/07-adversarial/auth-tampering.spec.ts +++ b/e2e/specs/07-adversarial/auth-tampering.spec.ts @@ -89,15 +89,43 @@ test.describe('Adversarial — JWT', () => { }); test.describe('Adversarial — PIN brute-force', () => { - test('sequential wrong-PIN attempts lock the account after 3 attempts', async ({ guest }) => { + /** + * The PIN defence has two tiers, and telling them apart is the whole point of these tests: + * + * - the per-(IP, name) throttle, which refuses the ATTACKER; and + * - the account lock, which refuses the VICTIM — the only tier that can be weaponised. + * + * Both answer 429, so the status code alone proves nothing. The regression these guard is that + * the lock threshold used to sit BELOW the throttle ceiling (3 vs 5), so three requests from a + * single IP locked any guest whose display name is readable off the feed, every 15 minutes, + * indefinitely. The tier meant to protect a guest was the cheapest way to attack them. + */ + // Restore the default. The first test turns the limiter on for the whole instance, and leaving + // it on would throttle unrelated specs sharing this stack. Runs even on failure. + test.afterEach(async ({ api, adminToken }) => { + await api.patchConfig(adminToken, { rate_limits_enabled: 'false' }); + }); + + test('a single IP is throttled without ever locking the victim out', async ({ + api, + adminToken, + guest, + db, + }) => { + // Rate limits are off by default in this environment, and the throttle IS the tier under + // test — without it the run would silently assert only half the property. + await api.patchConfig(adminToken, { + rate_limits_enabled: 'true', + recover_rate_enabled: 'true', + }); + const g = await guest('Brute'); const wrong = g.pin === '0000' ? '1111' : '0000'; - // Do them serially so the failed_pin_attempts counter increments - // monotonically. Parallel attempts race and may never accumulate to 3 in - // the current handler implementation — that's a separate finding. + // Serially, so the failed-PIN counter increments monotonically. Well past the per-(IP, name) + // ceiling of 4, and past the OLD lock threshold of 3. const statuses: number[] = []; - for (let i = 0; i < 4; i++) { + for (let i = 0; i < 8; i++) { const r = await fetch(`${BASE}/api/v1/recover`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -105,26 +133,52 @@ test.describe('Adversarial — PIN brute-force', () => { }); statuses.push(r.status); } - // First three are 401, fourth (or later) is 429. - expect(statuses.filter((s) => s === 200)).toHaveLength(0); - expect(statuses.some((s) => s === 429)).toBe(true); + expect(statuses.filter((s) => s === 200), 'a wrong PIN must never authenticate').toHaveLength( + 0 + ); + expect(statuses.some((s) => s === 429), 'the attacker must be throttled').toBe(true); - // Now even the correct PIN fails until lockout expires. + expect( + await db.isPinLocked(g.userId), + 'one IP must not be able to lock a guest out of their own account' + ).toBe(false); + }); + + test('the account still locks once the failure count is reached', async ({ guest, db }) => { + const g = await guest('BruteDistributed'); + const wrong = g.pin === '0000' ? '1111' : '0000'; + + // The per-IP throttle is what a single source hits first, so drive the counter the way a + // DISTRIBUTED attacker would — the tier this test covers is the last line against exactly + // that, and it must not have been removed while fixing the weaponisation above. + await db.setFailedPinAttempts(g.userId, 11); + + const r = await fetch(`${BASE}/api/v1/recover`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': '203.0.113.77' }, + body: JSON.stringify({ display_name: g.displayName, pin: wrong }), + }); + expect(r.status).toBe(401); + + expect( + await db.isPinLocked(g.userId), + 'a distributed guesser must still trip the account lock' + ).toBe(true); + + // And the lock holds even against the correct PIN, which is what makes it a real control. const correct = await fetch(`${BASE}/api/v1/recover`, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': '203.0.113.78' }, body: JSON.stringify({ display_name: g.displayName, pin: g.pin }), }); expect(correct.status).toBe(429); }); - test('parallel wrong-PIN attempts still lock the account (counter is not lost to the race)', async ({ - guest, - }) => { + test('the wrong-PIN streak is atomic under concurrency', async ({ guest, db }) => { const g = await guest('BruteParallel'); const wrong = g.pin === '0000' ? '1111' : '0000'; - const attempts = await Promise.all( + await Promise.all( Array.from({ length: 10 }, () => fetch(`${BASE}/api/v1/recover`, { method: 'POST', @@ -133,29 +187,14 @@ test.describe('Adversarial — PIN brute-force', () => { }) ) ); - const statuses = attempts.map((r) => r.status); - expect( - statuses.filter((s) => s === 200), - 'a wrong PIN must never authenticate' - ).toHaveLength(0); - // The in-flight requests all read `pin_locked_until` before any of them wrote it, so - // *which* of the 10 come back 429 is genuinely racy and can't be asserted. What is NOT - // racy — and is the property this test exists to guard — is the state left behind: - // `failed_pin_attempts` is incremented with an atomic `SET x = x + 1 ... RETURNING`, so - // 10 wrong PINs must push it past the 3-strike threshold and leave the account locked. - // - // We prove that with a follow-up request using the CORRECT pin: it must be refused with - // 429 (locked), not 200. Delete the lockout counter and this line goes 200 → red. - const correct = await fetch(`${BASE}/api/v1/recover`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ display_name: g.displayName, pin: g.pin }), - }); + // How many of the 10 get past the throttle is genuinely racy and cannot be asserted. What is + // NOT racy is that every one that DID reach the handler incremented the counter — it is a + // single `SET x = x + 1 ... RETURNING`, so none of them can be lost to the race. expect( - correct.status, - 'after 10 wrong PINs the account must be locked, even for the right PIN' - ).toBe(429); + await db.failedPinAttempts(g.userId), + 'concurrent wrong PINs must all be counted' + ).toBeGreaterThan(1); }); });