From d941ff2231a1332b367e0add43664934174c77ce Mon Sep 17 00:00:00 2001 From: Till JS Date: Wed, 8 Apr 2026 18:29:00 +0200 Subject: [PATCH] fix(mana-auth): account lockout was structurally dead + add failure-path tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While adding negative-path integration tests for the auth flow I discovered that *neither* of the lockout primitives in services/mana-auth/src/services/security.ts has actually been working in production. Two independent silent failures that combined into a "the lockout never triggers, ever" outcome: 1. recordAttempt() inserted into auth.login_attempts with explicit `id = gen_random_uuid()`, but auth.login_attempts.id is a `serial integer` column with `nextval('auth.login_attempts_id_seq')` as default. The UUID-into-integer cast threw a type error every single time, the bare `catch {}` swallowed it as "non-critical", and not a single login attempt was ever persisted. Lockout's "5 failures in 15 min" check was running against an empty table. 2. checkLockout() built `attempted_at > ${new Date(...)}` via the drizzle sql template, but postgres-js cannot bind a JS Date object directly — it tries to byteLength() the parameter and crashes with `Received an instance of Date`. Same anti-pattern: bare `catch`, returns `{locked: false}` (fail-open), no log, completely invisible. Both are "silent broken since the encryption-vault series of changes" class — caught only because the integration test for the lockout flow expected the 6th login attempt to return 429 and got 200 instead. Fixes: - recordAttempt(): drop the bogus `id` column from the INSERT (let the sequence default assign it), default ipAddress to null instead of letting `${undefined}` collapse the parameter slot, and surface errors in the catch instead of swallowing them silently. - checkLockout(): pass `windowStart.toISOString()` instead of the Date object so postgres-js can serialize it. Same catch upgrade — log the cause when failing open. Failure-path test additions (tests/integration/auth-failures.test.ts): - wrong password: assert 401, no JWT, +1 LOGIN_FAILURE in security_events, +1 row in auth.login_attempts - account lockout: 5 failed attempts then 6th returns 429 with remainingSeconds, even with the correct password - unverified email login: 403 with code = EMAIL_NOT_VERIFIED - validate with garbage token: valid !== true - resend verification: second mail arrives in mailpit Plus the run-integration-tests.sh helper now runs both .test.ts files and tests/integration/package.json's `test` script does the same. Negative-control: reverted the recordAttempt fix (re-added the bogus gen_random_uuid id), the wrong-password test failed at the login_attempts assertion. Reverted the checkLockout fix, the lockout test failed at the 429 assertion. Both fixes verified to be load-bearing. 6 tests, 45 expects, ~1.3s on a warm cache. --- scripts/run-integration-tests.sh | 4 +- services/mana-auth/src/services/security.ts | 34 ++- tests/integration/auth-failures.test.ts | 227 ++++++++++++++++++++ tests/integration/package.json | 2 +- 4 files changed, 258 insertions(+), 9 deletions(-) create mode 100644 tests/integration/auth-failures.test.ts diff --git a/scripts/run-integration-tests.sh b/scripts/run-integration-tests.sh index c42aea5c8..81011f568 100755 --- a/scripts/run-integration-tests.sh +++ b/scripts/run-integration-tests.sh @@ -68,9 +68,9 @@ echo "==> Restarting mana-auth so it picks up the freshly-created tables" $DOCKER compose -f "$COMPOSE_FILE" restart mana-auth >/dev/null $DOCKER compose -f "$COMPOSE_FILE" up -d --wait mana-auth -echo "==> Running auth-flow integration test" +echo "==> Running integration tests" cd "$TEST_DIR" -bun test auth-flow.test.ts +bun test auth-flow.test.ts auth-failures.test.ts echo echo "✅ integration tests passed" diff --git a/services/mana-auth/src/services/security.ts b/services/mana-auth/src/services/security.ts index 37ebe43c3..846df14ed 100644 --- a/services/mana-auth/src/services/security.ts +++ b/services/mana-auth/src/services/security.ts @@ -86,7 +86,10 @@ export class AccountLockoutService { async checkLockout(email: string): Promise<{ locked: boolean; remainingSeconds?: number }> { try { - const windowStart = new Date(Date.now() - WINDOW_MINUTES * 60 * 1000); + // postgres-js can't bind a JS Date directly via the drizzle sql + // template — it tries to byteLength() the parameter and crashes + // with `Received an instance of Date`. Pass an ISO string instead. + const windowStart = new Date(Date.now() - WINDOW_MINUTES * 60 * 1000).toISOString(); const result = await this.db.execute( sql`SELECT COUNT(*) as count, MAX(attempted_at) as last_attempt FROM auth.login_attempts @@ -104,19 +107,38 @@ export class AccountLockoutService { locked: true, remainingSeconds: Math.ceil((lockoutEnd.getTime() - Date.now()) / 1000), }; - } catch { + } catch (error) { + // Fail open on lockout-check errors (we'd rather let a legit + // user log in than block them on a transient DB hiccup), but + // surface the cause so the next bug doesn't take 4 hours to + // find like this one did. + console.warn( + 'checkLockout failed (fail-open):', + email, + error instanceof Error ? error.message : error + ); return { locked: false }; } } async recordAttempt(email: string, successful: boolean, ipAddress?: string) { try { + // Don't INSERT id — auth.login_attempts.id is a serial integer + // (`nextval('auth.login_attempts_id_seq')` default), not a UUID. + // The previous code passed `gen_random_uuid()` into it and the + // resulting type-cast error was silently eaten by the catch + // below — meaning lockout's "5 failures in 15 min" check ran on + // an empty table forever and the lockout never actually triggered. await this.db.execute( - sql`INSERT INTO auth.login_attempts (id, email, successful, ip_address, attempted_at) - VALUES (gen_random_uuid(), ${email}, ${successful}, ${ipAddress}, NOW())` + sql`INSERT INTO auth.login_attempts (email, successful, ip_address, attempted_at) + VALUES (${email}, ${successful}, ${ipAddress ?? null}, NOW())` + ); + } catch (error) { + console.warn( + 'Failed to record login attempt (non-critical):', + email, + error instanceof Error ? error.message : error ); - } catch { - // Non-critical } } diff --git a/tests/integration/auth-failures.test.ts b/tests/integration/auth-failures.test.ts new file mode 100644 index 000000000..0cb7129d3 --- /dev/null +++ b/tests/integration/auth-failures.test.ts @@ -0,0 +1,227 @@ +/** + * Negative-path integration tests for the auth flow. + * + * Companion to auth-flow.test.ts (the happy path). These cover the + * "user did something wrong" branches of mana-auth so refactors can't + * silently break them: + * + * 1. Login with wrong password → 401, LOGIN_FAILURE audit row, no JWT + * 2. Account lockout: 5 failed attempts → 6th returns 429 with + * remainingSeconds + * 3. Login as unverified user → 403 EMAIL_NOT_VERIFIED + * 4. POST /api/v1/auth/validate with garbage → valid: false + * 5. POST /api/v1/auth/resend-verification → second email lands in + * mailpit (catches the bug class where the resend handler swallows + * its own send error) + * + * Same docker-compose.test.yml stack as the happy-path test — both + * files run against `localhost:3091` (mana-auth) and `localhost:8026` + * (mailpit). Run via `./scripts/run-integration-tests.sh`. + */ + +import { test, expect, beforeAll, afterAll } from 'bun:test'; + +const AUTH_URL = process.env.AUTH_URL ?? 'http://localhost:3091'; +const MAILPIT_URL = process.env.MAILPIT_URL ?? 'http://localhost:8026'; + +// Two distinct users so the verified-user tests don't trip the +// unverified-user assertions and vice versa. +const VERIFIED_EMAIL = `failures-verified-${Date.now()}@manatest.local`; +const UNVERIFIED_EMAIL = `failures-unverified-${Date.now()}@manatest.local`; +const PASSWORD = 'TestPassword123!'; + +let verifiedUserId: string | null = null; +let unverifiedUserId: string | null = null; + +// ─── Tiny helpers (same shape as auth-flow.test.ts) ────────────────── + +async function postJson(path: string, body: unknown, headers?: HeadersInit) { + const res = await fetch(`${AUTH_URL}${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...headers }, + body: JSON.stringify(body), + }); + const json = (await res.json().catch(() => ({}))) as T; + return { status: res.status, json }; +} + +async function pgExec(sql: string): Promise { + const proc = Bun.spawn( + [ + 'docker', + 'exec', + 'mana-test-postgres', + 'psql', + '-U', + 'mana', + '-d', + 'mana_platform', + '-t', + '-A', + '-c', + sql, + ], + { stdout: 'pipe', stderr: 'pipe' } + ); + const out = await new Response(proc.stdout).text(); + const err = await new Response(proc.stderr).text(); + const code = await proc.exited; + if (code !== 0) throw new Error(`psql failed (${code}): ${err}`); + return out.trim(); +} + +async function countSecurityEvents(userId: string | null, eventType: string): Promise { + if (!userId) { + // LOGIN_FAILURE rows don't carry a user_id (we don't know which + // user yet). Match by event_type alone for those. + const out = await pgExec( + `SELECT COUNT(*) FROM auth.security_events WHERE event_type = '${eventType}';` + ); + return parseInt(out, 10); + } + const out = await pgExec( + `SELECT COUNT(*) FROM auth.security_events WHERE user_id = '${userId}' AND event_type = '${eventType}';` + ); + return parseInt(out, 10); +} + +async function countLoginAttempts(email: string): Promise { + const out = await pgExec( + `SELECT COUNT(*) FROM auth.login_attempts WHERE email = '${email}' AND successful = false;` + ); + return parseInt(out, 10); +} + +async function mailpitCount(to: string): Promise { + const res = await fetch(`${MAILPIT_URL}/api/v1/search?query=${encodeURIComponent(`to:${to}`)}`); + if (!res.ok) return 0; + const data = (await res.json()) as { messages?: unknown[] }; + return data.messages?.length ?? 0; +} + +// ─── Setup: create one verified + one unverified user ──────────────── + +beforeAll(async () => { + // Verified user + const reg1 = await postJson<{ user?: { id: string } }>('/api/v1/auth/register', { + email: VERIFIED_EMAIL, + password: PASSWORD, + name: 'Verified', + }); + expect(reg1.status).toBe(200); + verifiedUserId = reg1.json.user!.id; + await pgExec(`UPDATE auth.users SET email_verified = true WHERE id = '${verifiedUserId}';`); + + // Unverified user (just register, leave email_verified = false) + const reg2 = await postJson<{ user?: { id: string } }>('/api/v1/auth/register', { + email: UNVERIFIED_EMAIL, + password: PASSWORD, + name: 'Unverified', + }); + expect(reg2.status).toBe(200); + unverifiedUserId = reg2.json.user!.id; +}); + +// ─── Cleanup ───────────────────────────────────────────────────────── + +afterAll(async () => { + const ids = [verifiedUserId, unverifiedUserId].filter(Boolean) as string[]; + if (ids.length === 0) return; + const idList = ids.map((id) => `'${id}'`).join(','); + const emailList = `'${VERIFIED_EMAIL}', '${UNVERIFIED_EMAIL}'`; + try { + await pgExec( + `DELETE FROM auth.security_events WHERE user_id IN (${idList}); + DELETE FROM auth.login_attempts WHERE email IN (${emailList}); + DELETE FROM auth.encryption_vault_audit WHERE user_id IN (${idList}); + DELETE FROM auth.encryption_vaults WHERE user_id IN (${idList}); + DELETE FROM auth.users WHERE id IN (${idList});` + ); + } catch (err) { + console.warn('cleanup failed:', err); + } +}); + +// ─── Tests ─────────────────────────────────────────────────────────── + +test('login with wrong password → 401, no JWT, LOGIN_FAILURE audit row', async () => { + const failuresBefore = await countSecurityEvents(null, 'LOGIN_FAILURE'); + const attemptsBefore = await countLoginAttempts(VERIFIED_EMAIL); + + const res = await postJson<{ accessToken?: string }>('/api/v1/auth/login', { + email: VERIFIED_EMAIL, + password: 'definitely-not-the-password', + }); + expect(res.status).toBe(401); + expect(res.json.accessToken).toBeFalsy(); + + const failuresAfter = await countSecurityEvents(null, 'LOGIN_FAILURE'); + expect(failuresAfter).toBe(failuresBefore + 1); + + const attemptsAfter = await countLoginAttempts(VERIFIED_EMAIL); + expect(attemptsAfter).toBe(attemptsBefore + 1); +}); + +test('account lockout after 5 failed attempts → 6th returns 429', async () => { + // Clear any prior attempts so this test is independent of the one above + await pgExec(`DELETE FROM auth.login_attempts WHERE email = '${VERIFIED_EMAIL}';`); + + for (let i = 0; i < 5; i++) { + const res = await postJson('/api/v1/auth/login', { + email: VERIFIED_EMAIL, + password: 'still-not-the-password', + }); + expect(res.status).toBe(401); + } + + // 6th attempt — lockout should kick in BEFORE the password check runs + const lockedRes = await postJson<{ remainingSeconds?: number; error?: string }>( + '/api/v1/auth/login', + { email: VERIFIED_EMAIL, password: PASSWORD } // even with the right pw + ); + expect(lockedRes.status).toBe(429); + expect(lockedRes.json.error).toBe('Account locked'); + expect(lockedRes.json.remainingSeconds).toBeGreaterThan(0); + + // Clean up so the rest of the test file can still log in if needed + await pgExec(`DELETE FROM auth.login_attempts WHERE email = '${VERIFIED_EMAIL}';`); +}); + +test('login with unverified email → 403 EMAIL_NOT_VERIFIED', async () => { + const res = await postJson<{ code?: string; accessToken?: string }>('/api/v1/auth/login', { + email: UNVERIFIED_EMAIL, + password: PASSWORD, + }); + expect(res.status).toBe(403); + expect(res.json.code).toBe('EMAIL_NOT_VERIFIED'); + expect(res.json.accessToken).toBeFalsy(); +}); + +test('validate with garbage token → valid: false', async () => { + const res = await postJson<{ valid: boolean }>('/api/v1/auth/validate', { + token: 'not-a-real-jwt.totally-garbage.signature', + }); + // Either 200 with valid: false (well-formed but invalid signature) or 401 + // (unparseable). Both are acceptable as long as valid !== true. + expect([200, 401]).toContain(res.status); + expect(res.json.valid).not.toBe(true); +}); + +test('resend verification → second email lands in mailpit', async () => { + const before = await mailpitCount(UNVERIFIED_EMAIL); + + const res = await postJson<{ success?: boolean }>('/api/v1/auth/resend-verification', { + email: UNVERIFIED_EMAIL, + }); + expect(res.status).toBe(200); + expect(res.json.success).toBe(true); + + // Poll for the new mail (the original signup mail might also be present) + const deadline = Date.now() + 10_000; + while (Date.now() < deadline) { + const after = await mailpitCount(UNVERIFIED_EMAIL); + if (after > before) return; + await new Promise((r) => setTimeout(r, 200)); + } + throw new Error(`Resend verification did not produce a new email within 10s`); +}); diff --git a/tests/integration/package.json b/tests/integration/package.json index 7c1b18a10..b10c30b7a 100644 --- a/tests/integration/package.json +++ b/tests/integration/package.json @@ -4,6 +4,6 @@ "private": true, "type": "module", "scripts": { - "test": "bun test auth-flow.test.ts" + "test": "bun test auth-flow.test.ts auth-failures.test.ts" } }