diff --git a/apps/api/src/auth/localCredentials.ts b/apps/api/src/auth/localCredentials.ts index 7245fa4..ff11500 100644 --- a/apps/api/src/auth/localCredentials.ts +++ b/apps/api/src/auth/localCredentials.ts @@ -18,7 +18,31 @@ * Max length: ~83 chars — fits in varchar(256) password_hash column */ -import { scryptSync, randomBytes, timingSafeEqual } from 'node:crypto'; +import { scrypt, randomBytes, timingSafeEqual } from 'node:crypto'; +import type { BinaryLike, ScryptOptions } from 'node:crypto'; + +// WR-03: use the ASYNC scrypt (libuv threadpool) so password hashing does NOT block the +// single Node event loop. scryptSync ran on the main thread, so a burst of unauthenticated +// POST /local/login requests (each running scrypt N=16384, ~tens of ms of CPU, including the +// always-run dummy-hash path) could pin the loop and stall ALL other API traffic — a cheap +// unauthenticated DoS. This async wrapper offloads the CPU to the threadpool, preserving the +// timing-defense property while keeping the loop responsive. +// +// A hand-rolled Promise wrapper is used (rather than promisify) because promisify's typings +// do not cover the options-carrying scrypt overload (N/r/p). +function scryptAsync( + password: BinaryLike, + salt: BinaryLike, + keylen: number, + options: ScryptOptions, +): Promise { + return new Promise((resolve, reject) => { + scrypt(password, salt, keylen, options, (err, derivedKey) => { + if (err) reject(err); + else resolve(derivedKey); + }); + }); +} // OWASP-compatible scrypt parameters for password hashing const SCRYPT_N = 16384; // CPU/memory cost factor (2^14) @@ -36,12 +60,15 @@ const KEY_LEN = 32; // 256-bit derived key output * the hash without relying on hardcoded constants — supports future parameter * migration without a DB schema change. * - * NOTE: scryptSync blocks the event loop. For a 2-person household with - * infrequent logins this is acceptable. Use promisify(scrypt) if async is needed. + * WR-03: async — scrypt runs on the libuv threadpool, not the event loop. */ -export function hashPassword(password: string): string { +export async function hashPassword(password: string): Promise { const salt = randomBytes(16); - const hash = scryptSync(password, salt, KEY_LEN, { N: SCRYPT_N, r: SCRYPT_R, p: SCRYPT_P }); + const hash = await scryptAsync(password, salt, KEY_LEN, { + N: SCRYPT_N, + r: SCRYPT_R, + p: SCRYPT_P, + }); return [ 'scrypt', SCRYPT_N, @@ -66,9 +93,12 @@ export function hashPassword(password: string): string { * scrypt parameter error — safe to call with untrusted input. * - Never logs the candidate password. * - * @returns true if candidate matches the stored hash; false otherwise (incl. errors) + * WR-03: async — scrypt runs on the libuv threadpool, not the event loop. + * + * @returns Promise if candidate matches the stored hash; Promise otherwise + * (including all parse/format/crypto errors — never rejects). */ -export function verifyPassword(storedEncoded: string, candidate: string): boolean { +export async function verifyPassword(storedEncoded: string, candidate: string): Promise { try { const parts = storedEncoded.split('$'); if (parts.length !== 6) return false; @@ -76,7 +106,7 @@ export function verifyPassword(storedEncoded: string, candidate: string): boolea const salt = Buffer.from(saltB64, 'base64url'); const storedHash = Buffer.from(hashB64, 'base64url'); if (salt.length === 0 || storedHash.length === 0) return false; - const candidateHash = scryptSync(candidate, salt, storedHash.length, { + const candidateHash = await scryptAsync(candidate, salt, storedHash.length, { N: Number(n), r: Number(r), p: Number(p), diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index 4ea63a9..507f3e4 100644 --- a/apps/api/src/routes/admin.ts +++ b/apps/api/src/routes/admin.ts @@ -141,6 +141,10 @@ adminRouter.post( COLOR_PALETTE.find((c) => !usedColors.has(c)) ?? COLOR_PALETTE[usedColors.size % COLOR_PALETTE.length]; + // WR-03: hash the initial password BEFORE opening the transaction so the (now async, + // threadpool) scrypt work does not hold the DB transaction open for its duration. + const initialPasswordHash = await hashPassword(initialPassword); + try { let newUserId: number; @@ -163,7 +167,7 @@ adminRouter.post( await tx.insert(localCredentials).values({ userId: newUserId, username, - passwordHash: hashPassword(initialPassword), + passwordHash: initialPasswordHash, }); }); @@ -236,7 +240,7 @@ adminRouter.post( try { await db .update(localCredentials) - .set({ passwordHash: hashPassword(newPassword) }) + .set({ passwordHash: await hashPassword(newPassword) }) .where(eq(localCredentials.userId, targetId)); // CR-04: an admin password reset must immediately clear any rate-limit / lockout diff --git a/apps/api/src/routes/localAuth.ts b/apps/api/src/routes/localAuth.ts index 39ee678..c86c45d 100644 --- a/apps/api/src/routes/localAuth.ts +++ b/apps/api/src/routes/localAuth.ts @@ -99,8 +99,10 @@ export function resetLoginAttempts(username: string): void { // Pre-computed dummy hash used to run verifyPassword on unknown-username paths // (timing defense — prevents timing-oracle username enumeration, RESEARCH Pitfall 2). -// Computed once at module load time; the actual value is never used for auth. -const DUMMY_HASH = hashPassword('dummy-constant-time-filler-xyzzy'); +// WR-03: hashPassword is now async. Kick off the computation once at module load and keep +// the PROMISE; the login handler awaits it. The value is never used for auth — only to make +// the unknown-username path perform the same scrypt work as the known-username path. +const dummyHashPromise: Promise = hashPassword('dummy-constant-time-filler-xyzzy'); // --------------------------------------------------------------------------- // POST /local/login → POST /api/auth/local/login @@ -160,9 +162,10 @@ localAuthRouter.post('/local/login', zValidator('json', loginSchema, noEchoHook) // ALWAYS run verifyPassword — even for unknown usernames — to prevent timing-oracle // username enumeration (T-19-12 / RESEARCH Pitfall 2). Use a pre-computed dummy hash // so the scrypt work is always performed regardless of whether username was found. + // WR-03: verifyPassword is async (threadpool scrypt) — await it. const valid = cred - ? verifyPassword(cred.passwordHash, password) - : verifyPassword(DUMMY_HASH, password); + ? await verifyPassword(cred.passwordHash, password) + : await verifyPassword(await dummyHashPromise, password); if (!valid || !cred) { // Increment failure counter (keyed on username) diff --git a/apps/api/src/routes/me.ts b/apps/api/src/routes/me.ts index dd18473..f766aea 100644 --- a/apps/api/src/routes/me.ts +++ b/apps/api/src/routes/me.ts @@ -255,8 +255,8 @@ meRouter.post( return c.json({ error: 'No local credential found' }, 404); } - // T-19-07: verify current password before any update - const isCorrect = verifyPassword(credRow.passwordHash, currentPassword); + // T-19-07: verify current password before any update (WR-03: async scrypt) + const isCorrect = await verifyPassword(credRow.passwordHash, currentPassword); if (!isCorrect) { // CR-03: return 403 (NOT 401) for a wrong current password. The PWA's global // MutationCache treats any 401 as "session expired" and arms the re-auth @@ -269,7 +269,7 @@ meRouter.post( try { await db .update(localCredentials) - .set({ passwordHash: hashPassword(newPassword) }) + .set({ passwordHash: await hashPassword(newPassword) }) .where(eq(localCredentials.userId, currentUserId)); return c.json({ ok: true }, 200); diff --git a/apps/api/tests/auth/localCredentials.test.ts b/apps/api/tests/auth/localCredentials.test.ts index 5bc5433..6c3a591 100644 --- a/apps/api/tests/auth/localCredentials.test.ts +++ b/apps/api/tests/auth/localCredentials.test.ts @@ -4,6 +4,9 @@ * Uses node:crypto scrypt under the hood; no external dependencies. * All tests run without MariaDB or any external service. * + * WR-03: hashPassword / verifyPassword are now async (promisify(scrypt), threadpool) — + * all assertions await them. + * * Test suite (TDD RED → GREEN — Plan 19-01 Task 1): * Test 1: correct password verifies true * Test 2: wrong password verifies false @@ -16,33 +19,32 @@ import { describe, it, expect } from 'vitest'; import { hashPassword, verifyPassword } from '../../src/auth/localCredentials.js'; describe('hashPassword / verifyPassword', () => { - it('Test 1: verifyPassword(hashPassword(pw), pw) === true (round-trip)', () => { - const encoded = hashPassword('hunter2'); - const result = verifyPassword(encoded, 'hunter2'); + it('Test 1: verifyPassword(hashPassword(pw), pw) === true (round-trip)', async () => { + const encoded = await hashPassword('hunter2'); + const result = await verifyPassword(encoded, 'hunter2'); expect(result).toBe(true); }); - it('Test 2: verifyPassword(hashPassword(pw), wrong) === false', () => { - const encoded = hashPassword('hunter2'); - const result = verifyPassword(encoded, 'wrong-password'); + it('Test 2: verifyPassword(hashPassword(pw), wrong) === false', async () => { + const encoded = await hashPassword('hunter2'); + const result = await verifyPassword(encoded, 'wrong-password'); expect(result).toBe(false); }); - it('Test 3: two hashPassword calls on same input produce different encoded strings (unique salt)', () => { - const encoded1 = hashPassword('x'); - const encoded2 = hashPassword('x'); + it('Test 3: two hashPassword calls on same input produce different encoded strings (unique salt)', async () => { + const encoded1 = await hashPassword('x'); + const encoded2 = await hashPassword('x'); expect(encoded1).not.toBe(encoded2); }); - it('Test 4: verifyPassword returns false (never throws) on a malformed stored hash', () => { - expect(() => verifyPassword('not-a-valid-hash', 'x')).not.toThrow(); - expect(verifyPassword('not-a-valid-hash', 'x')).toBe(false); - expect(verifyPassword('', 'x')).toBe(false); - expect(verifyPassword('scrypt$bad$data', 'x')).toBe(false); + it('Test 4: verifyPassword returns false (never throws) on a malformed stored hash', async () => { + await expect(verifyPassword('not-a-valid-hash', 'x')).resolves.toBe(false); + await expect(verifyPassword('', 'x')).resolves.toBe(false); + await expect(verifyPassword('scrypt$bad$data', 'x')).resolves.toBe(false); }); - it('Test 5: encoded string has scrypt$N$r$p$salt$hash shape (6 $-delimited segments)', () => { - const encoded = hashPassword('testpassword'); + it('Test 5: encoded string has scrypt$N$r$p$salt$hash shape (6 $-delimited segments)', async () => { + const encoded = await hashPassword('testpassword'); const segments = encoded.split('$'); expect(segments).toHaveLength(6); expect(segments[0]).toBe('scrypt'); diff --git a/apps/api/tests/routes/admin.test.ts b/apps/api/tests/routes/admin.test.ts index c525fe4..870b5e8 100644 --- a/apps/api/tests/routes/admin.test.ts +++ b/apps/api/tests/routes/admin.test.ts @@ -890,7 +890,7 @@ describe('POST /api/admin/members', () => { .where(eq(localCredentials.userId, body.id)) .limit(1); expect(credRow).toBeDefined(); - expect(verifyPassword(credRow.passwordHash, initialPassword)).toBe(true); + expect(await verifyPassword(credRow.passwordHash, initialPassword)).toBe(true); }); it('Test 2: duplicate username returns 409 — transaction rolls back (no orphaned users row)', async () => { @@ -967,8 +967,8 @@ describe('POST /api/admin/members', () => { .where(eq(localCredentials.userId, newMemberId)) .limit(1); expect(credRow).toBeDefined(); - expect(verifyPassword(credRow.passwordHash, newPassword)).toBe(true); - expect(verifyPassword(credRow.passwordHash, 'old-password-123')).toBe(false); + expect(await verifyPassword(credRow.passwordHash, newPassword)).toBe(true); + expect(await verifyPassword(credRow.passwordHash, 'old-password-123')).toBe(false); }); it('Test 4: non-admin gets 403 on POST /members and POST /members/:id/password', async () => { diff --git a/apps/api/tests/routes/localAuth.test.ts b/apps/api/tests/routes/localAuth.test.ts index 66d5eda..6f511d3 100644 --- a/apps/api/tests/routes/localAuth.test.ts +++ b/apps/api/tests/routes/localAuth.test.ts @@ -164,7 +164,7 @@ async function getLocalCredentials() { describe('POST /api/auth/local/login', () => { it('Test 1: valid username+password → 200 { ok:true } and issueLocalSessionCookie called', async () => { const { hashPassword } = await getLocalCredentials(); - const hash = hashPassword('correcthorse'); + const hash = await hashPassword('correcthorse'); mockCredRow = { userId: 5, passwordHash: hash }; const app = await getApp(); @@ -179,7 +179,7 @@ describe('POST /api/auth/local/login', () => { it('Test 2: wrong password → 401 { error: "Invalid credentials" }', async () => { const { hashPassword } = await getLocalCredentials(); - const hash = hashPassword('correcthorse'); + const hash = await hashPassword('correcthorse'); mockCredRow = { userId: 5, passwordHash: hash }; const app = await getApp(); diff --git a/apps/api/tests/routes/me.test.ts b/apps/api/tests/routes/me.test.ts index a464bed..72a1527 100644 --- a/apps/api/tests/routes/me.test.ts +++ b/apps/api/tests/routes/me.test.ts @@ -274,7 +274,7 @@ describe('POST /api/me/password — self-change password (AUTH-LOCAL-09)', () => const oldPassword = 'old-password-correct-123'; const newPassword = 'new-password-secure-456'; - const storedHash = hashPassword(oldPassword); + const storedHash = await hashPassword(oldPassword); let updatedHash: string | null = null; // Mock sequence: resolveUserId (devBypass sets user), then: @@ -326,15 +326,15 @@ describe('POST /api/me/password — self-change password (AUTH-LOCAL-09)', () => // The updatedHash must verify the new password expect(updatedHash).not.toBeNull(); const { verifyPassword } = await import('../../src/auth/localCredentials.js'); - expect(verifyPassword(updatedHash!, newPassword)).toBe(true); - expect(verifyPassword(updatedHash!, oldPassword)).toBe(false); + expect(await verifyPassword(updatedHash!, newPassword)).toBe(true); + expect(await verifyPassword(updatedHash!, oldPassword)).toBe(false); }); it('Test 2: wrong currentPassword → 403 and update is NOT called', async () => { const { db } = await import('../../src/db/client.js'); const realPassword = 'real-password-correct-789'; - const storedHash = hashPassword(realPassword); + const storedHash = await hashPassword(realPassword); let updateWasCalled = false; let callCount = 0;