From 1688f229e04d2397b63d17a8e74ba8461f153570 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 17 Jun 2026 20:15:23 -0400 Subject: [PATCH 01/18] fix(19): CR-01 align OIDC-link client contract with server (authorizationUrl) --- apps/pwa/src/api/client.ts | 14 +++++++++----- apps/pwa/src/components/SettingsSheet.tsx | 10 ++++++++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/apps/pwa/src/api/client.ts b/apps/pwa/src/api/client.ts index 7067342..743acd9 100644 --- a/apps/pwa/src/api/client.ts +++ b/apps/pwa/src/api/client.ts @@ -219,11 +219,15 @@ export async function fetchAdminResetPassword( /** * POST /api/me/link-oidc — initiate the OIDC-link flow for the current local user (Surface 13). * - * The server returns a redirect URL to begin the OIDC authorization-code flow with a - * state parameter encoding the linkUserId claim. The caller should follow the redirect - * via top-level navigation (window.location.href = result.redirectUrl). + * The server returns the OIDC authorization endpoint URL (with a signed `state` parameter + * encoding the linkUserId claim) to begin the authorization-code flow. The caller should + * follow it via top-level navigation (window.location.href = authorizationUrl) when present. + * + * authorizationUrl is null when OIDC is not configured in env (the server cannot build the + * URL); callers MUST handle that case and surface an error instead of navigating to null. + * The server contract is { signedState, authorizationUrl } (see apps/api/src/routes/me.ts). */ -export async function fetchLinkOidc(): Promise<{ redirectUrl: string }> { +export async function fetchLinkOidc(): Promise<{ authorizationUrl: string | null }> { const res = await fetch('/api/me/link-oidc', { method: 'POST', credentials: 'include', @@ -234,7 +238,7 @@ export async function fetchLinkOidc(): Promise<{ redirectUrl: string }> { if (!res.ok) { throw new Error(`fetchLinkOidc failed: ${res.status}`); } - return res.json() as Promise<{ redirectUrl: string }>; + return res.json() as Promise<{ signedState: string; authorizationUrl: string | null }>; } // ── /api/me ──────────────────────────────────────────────────────────────── diff --git a/apps/pwa/src/components/SettingsSheet.tsx b/apps/pwa/src/components/SettingsSheet.tsx index 287af72..59f5b1a 100644 --- a/apps/pwa/src/components/SettingsSheet.tsx +++ b/apps/pwa/src/components/SettingsSheet.tsx @@ -859,9 +859,15 @@ function LinkOidcSheet({ isOpen, onClose }: LinkOidcSheetProps) { const linkMutation = useMutation({ mutationFn: fetchLinkOidc, onSuccess: (data) => { - // Close the sheet and initiate OIDC link flow + // authorizationUrl is null when OIDC is not configured in env (server could not + // build the URL). Do NOT navigate to null — surface an error and keep the sheet open. + if (!data.authorizationUrl) { + setError('Something went wrong. Please try again.'); + return; + } + // Close the sheet and initiate the OIDC link flow via top-level navigation. onClose(); - window.location.href = data.redirectUrl; + window.location.href = data.authorizationUrl; }, onError: () => { setError('Something went wrong. Please try again.'); From 93c47b38aa5d8f3cdbba7d2efdbf2a5653f9441a Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 17 Jun 2026 20:15:58 -0400 Subject: [PATCH 02/18] fix(19): CR-02 send initialPassword + map 409 conflict in fetchCreateMember --- apps/pwa/src/api/client.ts | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/apps/pwa/src/api/client.ts b/apps/pwa/src/api/client.ts index 743acd9..2c054c9 100644 --- a/apps/pwa/src/api/client.ts +++ b/apps/pwa/src/api/client.ts @@ -168,10 +168,14 @@ export async function fetchChangePassword(body: { * POST /api/admin/members — create a new local member account (Phase 19, Surface 11A). * Admin-only; server enforces requireAdmin. * + * Request contract: the server's createMemberSchema requires + * { displayName, username, initialPassword } + * (see apps/api/src/routes/admin.ts). The caller-facing `password` field is mapped to + * `initialPassword` here so the request validates server-side. + * * Status codes: - * 409 → username already taken - * 422 → validation failure (short password / mismatch) - * other non-ok → generic error + * 409 → username already taken (throws Error with message 'conflict') + * other non-ok → generic error (throws Error('server')) */ export async function fetchCreateMember(body: { displayName: string; @@ -183,10 +187,19 @@ export async function fetchCreateMember(body: { headers: { 'Content-Type': 'application/json' }, credentials: 'include', redirect: 'manual', - body: JSON.stringify(body), + // CR-02: the server expects `initialPassword`, not `password`. Send the field it + // validates against — otherwise Zod rejects every create with a generic 400. + body: JSON.stringify({ + displayName: body.displayName, + username: body.username, + initialPassword: body.password, + }), }); if (res.type === 'opaqueredirect' || res.status === 401) throw new SessionExpiredError(); + // 409 → username conflict. The server returns { error: 'Username already in use' } (no + // `code` field), so map the status to the 'conflict' sentinel the AdminPage handler expects. + if (res.status === 409) throw new Error('conflict'); if (!res.ok) { const detail = (await res.json().catch(() => ({}))) as { code?: string }; throw new Error(detail.code ?? 'server'); From 6ef8e03f8c9fdb8e7d99017e2d24051df25c6c81 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 17 Jun 2026 20:17:32 -0400 Subject: [PATCH 03/18] fix(19): CR-03 return 403 for wrong current password so change-password does not log user out --- apps/api/src/routes/me.ts | 7 ++++++- apps/api/tests/routes/me.test.ts | 6 ++++-- apps/pwa/src/api/client.ts | 13 ++++++++++--- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/apps/api/src/routes/me.ts b/apps/api/src/routes/me.ts index 6587312..1fff649 100644 --- a/apps/api/src/routes/me.ts +++ b/apps/api/src/routes/me.ts @@ -257,7 +257,12 @@ meRouter.post( // T-19-07: verify current password before any update const isCorrect = verifyPassword(credRow.passwordHash, currentPassword); if (!isCorrect) { - return c.json({ error: 'Current password incorrect' }, 401); + // 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 + // interstitial / login redirect — so a 401 here would force-log-out a user who + // merely mistyped their current password. 403 is in-app authorization-failure and + // lets the client surface "current password incorrect" without dropping the session. + return c.json({ error: 'Current password incorrect' }, 403); } try { diff --git a/apps/api/tests/routes/me.test.ts b/apps/api/tests/routes/me.test.ts index 7eb6e30..a464bed 100644 --- a/apps/api/tests/routes/me.test.ts +++ b/apps/api/tests/routes/me.test.ts @@ -330,7 +330,7 @@ describe('POST /api/me/password — self-change password (AUTH-LOCAL-09)', () => expect(verifyPassword(updatedHash!, oldPassword)).toBe(false); }); - it('Test 2: wrong currentPassword → 401 and update is NOT called', async () => { + 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'; @@ -374,7 +374,9 @@ describe('POST /api/me/password — self-change password (AUTH-LOCAL-09)', () => body: JSON.stringify({ currentPassword: 'WRONG-password', newPassword: 'new-pass-12345678' }), }); - expect(res.status).toBe(401); + // CR-03: wrong current password returns 403 (in-app authz failure), NOT 401. + // A 401 would be interpreted by the PWA as session expiry and log the user out. + expect(res.status).toBe(403); const body = (await res.json()) as { error: string }; expect(body.error).toBe('Current password incorrect'); // Update must NOT have been called diff --git a/apps/pwa/src/api/client.ts b/apps/pwa/src/api/client.ts index 2c054c9..7bd52a8 100644 --- a/apps/pwa/src/api/client.ts +++ b/apps/pwa/src/api/client.ts @@ -141,9 +141,14 @@ export async function fetchLocalLogout(): Promise { * Requires the user's current password and a new password (min 8 chars). * * Status codes: - * 401 → wrong current password (throws Error with code 'wrong-current') - * 422 → validation failure (throws Error with code 'validation') - * other non-ok → generic error + * 403 → wrong current password (throws Error('wrong-current')) — NOT a session expiry + * 401 / opaqueredirect → genuine session expiry (throws SessionExpiredError) + * other non-ok → generic error (throws Error('server')) + * + * CR-03: the server returns 403 (not 401) for an incorrect current password so this + * client can distinguish an in-app authorization failure from a real session expiry. + * Treating that case as 401 would route it to the global MutationCache session-expiry + * handler and forcibly log the user out for a simple mistyped password. */ export async function fetchChangePassword(body: { currentPassword: string; @@ -157,6 +162,8 @@ export async function fetchChangePassword(body: { body: JSON.stringify(body), }); + // 403 → wrong current password (in-app). Check BEFORE the 401 session-expiry branch. + if (res.status === 403) throw new Error('wrong-current'); if (res.type === 'opaqueredirect' || res.status === 401) throw new SessionExpiredError(); if (!res.ok) { const detail = (await res.json().catch(() => ({}))) as { code?: string }; From b083cb71934501cc0be51c8c76e71b374ceaedd5 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 17 Jun 2026 20:20:43 -0400 Subject: [PATCH 04/18] fix(19): CR-04 scope login lockout to username, add TTL auto-expiry + admin-reset unlock --- apps/api/src/routes/admin.ts | 11 ++- apps/api/src/routes/localAuth.ts | 89 +++++++++++++++++-------- apps/api/tests/routes/localAuth.test.ts | 36 +++++++++- apps/pwa/src/routes/LoginPage.tsx | 3 +- 4 files changed, 108 insertions(+), 31 deletions(-) diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index 968bdd3..4ea63a9 100644 --- a/apps/api/src/routes/admin.ts +++ b/apps/api/src/routes/admin.ts @@ -35,6 +35,7 @@ import { CredentialValidationError, } from '../broker/credentialSync.js'; import { hashPassword } from '../auth/localCredentials.js'; +import { resetLoginAttempts } from './localAuth.js'; import { COLOR_PALETTE } from '../auth/user.js'; // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user') import '../auth/devBypass.js'; @@ -220,9 +221,10 @@ adminRouter.post( const { newPassword } = c.req.valid('json'); // T-19-06: NEVER log newPassword or the request body - // Verify the target user has a local_credentials row (404 if not) + // Verify the target user has a local_credentials row (404 if not). + // Also read the username so we can clear any login lockout for it (CR-04). const [credRow] = await db - .select({ id: localCredentials.id }) + .select({ id: localCredentials.id, username: localCredentials.username }) .from(localCredentials) .where(eq(localCredentials.userId, targetId)) .limit(1); @@ -237,6 +239,11 @@ adminRouter.post( .set({ passwordHash: hashPassword(newPassword) }) .where(eq(localCredentials.userId, targetId)); + // CR-04: an admin password reset must immediately clear any rate-limit / lockout + // state for this username, so a locked-out member regains access at once rather than + // waiting for the TTL. The lockout is keyed on username (not member id). + resetLoginAttempts(credRow.username); + return c.json({ ok: true }, 200); } catch (err) { console.error( diff --git a/apps/api/src/routes/localAuth.ts b/apps/api/src/routes/localAuth.ts index 5069c99..39ee678 100644 --- a/apps/api/src/routes/localAuth.ts +++ b/apps/api/src/routes/localAuth.ts @@ -14,8 +14,16 @@ * - Dummy-hash timing defense: verifyPassword is always called, even for unknown usernames, * to prevent timing-oracle username enumeration attacks (T-19-12 / RESEARCH Pitfall 2). * - Same 401 body for wrong password AND unknown username — no field discrimination (T-19-12). - * - Per-IP in-memory rate-limiting: 5 failures → 429; 10 failures → 423 (T-19-11). - * - Lockout (423) cleared only by admin password reset — no self-service unlock. + * - Per-USERNAME in-memory rate-limiting: 5 failures → 429; 10 failures → 423 (T-19-11). + * - Lockout (423) auto-expires after LOCKOUT_TTL_MS (CR-04) so a single attacker cannot + * permanently deny login for the whole household, and no process restart is needed to + * recover. Admin password reset still clears it immediately (resetLoginAttempts). + * + * CR-04: the limiter is keyed on the submitted username, NOT the client IP. In this + * deployment all household traffic egresses the Pangolin tunnel with the same + * X-Forwarded-For first hop, so IP-keying made one bad actor (or one fat-fingered user) + * able to lock out every member, and X-Forwarded-For is attacker-spoofable. Username-keying + * scopes the lockout to the identity actually under attack and the TTL makes it self-healing. */ import { Hono } from 'hono'; @@ -52,22 +60,42 @@ const loginSchema = z.object({ }); // --------------------------------------------------------------------------- -// Per-IP rate-limiting state (in-memory Map — household scale, no Redis needed). +// Per-USERNAME rate-limiting state (in-memory Map — household scale, no Redis needed). // -// State shape per IP: { count, lockedUntil (epoch ms), lockedOut (bool) } +// State shape per username: { count, lockedUntil (epoch ms), lockedOut (bool), lockedAt (epoch ms) } // count >= RATE_WINDOW_FAILURES AND Date.now() < lockedUntil → 429 -// count >= LOCKOUT_FAILURES → 423 (permanent until admin reset) +// count >= LOCKOUT_FAILURES → 423 (until LOCKOUT_TTL_MS elapses OR admin reset) // Success → delete entry (clears counter) // // RATE_WINDOW_FAILURES: 5 failures → 60s cooldown (429) // LOCKOUT_FAILURES: 10 failures → account locked (423) +// LOCKOUT_TTL_MS: 15 min after which a 423 lockout auto-expires (CR-04) +// +// CR-04: keyed on the validated username (not the client IP) so a lockout is scoped to +// the identity under attack, and self-heals after LOCKOUT_TTL_MS without a restart. // --------------------------------------------------------------------------- -export const loginAttempts = new Map(); +export const loginAttempts = new Map< + string, + { count: number; lockedUntil: number; lockedOut: boolean; lockedAt: number } +>(); const RATE_WINDOW_FAILURES = 5; const RATE_WINDOW_SECS = 60; const LOCKOUT_FAILURES = 10; +const LOCKOUT_TTL_MS = 15 * 60 * 1000; // CR-04: 423 lockout auto-expires after 15 minutes + +/** + * Immediately clear any rate-limit / lockout state for a username (CR-04). + * + * Called by the admin password-reset path so a reset is an instant unlock and the + * lockout is not "unrecoverable without a process restart". Safe to call for an + * unknown username (no-op). Normalizes the username the same way the login schema + * does (trim) so the key matches what the limiter stored. + */ +export function resetLoginAttempts(username: string): void { + loginAttempts.delete(username.trim()); +} // Pre-computed dummy hash used to run verifyPassword on unknown-username paths // (timing defense — prevents timing-oracle username enumeration, RESEARCH Pitfall 2). @@ -79,34 +107,42 @@ const DUMMY_HASH = hashPassword('dummy-constant-time-filler-xyzzy'); // --------------------------------------------------------------------------- localAuthRouter.post('/local/login', zValidator('json', loginSchema, noEchoHook), async (c) => { - // Derive client IP from Pangolin-set X-Forwarded-For header; fall back to host header. - const ip = c.req.header('x-forwarded-for')?.split(',')[0]?.trim() - ?? c.req.raw.headers.get('host') - ?? 'unknown'; + // CR-04: the rate-limit / lockout key is the validated, normalized username — NOT the + // client IP. zValidator has already run, so c.req.valid('json') is available here. + const { username, password } = c.req.valid('json'); + const key = username; // loginSchema .trim()s the username; the map key matches resetLoginAttempts - const attempt = loginAttempts.get(ip); + const now = Date.now(); + const attempt = loginAttempts.get(key); - // 423: account locked (>= LOCKOUT_FAILURES total failures, admin must reset) - // Check lockedOut FIRST — lockout takes precedence over rate window. + // 423: account locked (>= LOCKOUT_FAILURES failures). CR-04: the lockout auto-expires + // after LOCKOUT_TTL_MS so a single attacker cannot deny login indefinitely and no restart + // is required to recover. On expiry, drop the entry so the next attempt starts clean. if (attempt?.lockedOut) { - return c.json({ error: 'Account locked' }, 423); + if (now - attempt.lockedAt >= LOCKOUT_TTL_MS) { + loginAttempts.delete(key); + } else { + return c.json({ error: 'Account locked' }, 423); + } } + // Re-read after a possible expiry-delete above. + const live = loginAttempts.get(key); + // 429: rate window — >= RATE_WINDOW_FAILURES failures within the cooldown window. // Increment the counter even on 429 so continued brute-force accumulates toward lockout. - if (attempt && attempt.count >= RATE_WINDOW_FAILURES && Date.now() < attempt.lockedUntil) { - attempt.count += 1; - attempt.lockedUntil = Date.now() + RATE_WINDOW_SECS * 1000; - attempt.lockedOut = attempt.count >= LOCKOUT_FAILURES; - loginAttempts.set(ip, attempt); - if (attempt.lockedOut) { + if (live && live.count >= RATE_WINDOW_FAILURES && now < live.lockedUntil) { + live.count += 1; + live.lockedUntil = now + RATE_WINDOW_SECS * 1000; + live.lockedOut = live.count >= LOCKOUT_FAILURES; + if (live.lockedOut) live.lockedAt = now; + loginAttempts.set(key, live); + if (live.lockedOut) { return c.json({ error: 'Account locked' }, 423); } return c.json({ error: 'Too many attempts' }, 429); } - const { username, password } = c.req.valid('json'); - // Look up local_credentials by username let cred: { userId: number; passwordHash: string } | undefined; try { @@ -129,18 +165,19 @@ localAuthRouter.post('/local/login', zValidator('json', loginSchema, noEchoHook) : verifyPassword(DUMMY_HASH, password); if (!valid || !cred) { - // Increment failure counter - const cur = loginAttempts.get(ip) ?? { count: 0, lockedUntil: 0, lockedOut: false }; + // Increment failure counter (keyed on username) + const cur = loginAttempts.get(key) ?? { count: 0, lockedUntil: 0, lockedOut: false, lockedAt: 0 }; cur.count += 1; cur.lockedUntil = Date.now() + RATE_WINDOW_SECS * 1000; cur.lockedOut = cur.count >= LOCKOUT_FAILURES; - loginAttempts.set(ip, cur); + if (cur.lockedOut && cur.lockedAt === 0) cur.lockedAt = Date.now(); + loginAttempts.set(key, cur); // Same 401 body for wrong password AND unknown username — no field discrimination (T-19-12) return c.json({ error: 'Invalid credentials' }, 401); } // Success: clear failure counter, issue the local-session JWT cookie, return ok - loginAttempts.delete(ip); + loginAttempts.delete(key); try { await issueLocalSessionCookie(c, cred.userId); } catch (err) { diff --git a/apps/api/tests/routes/localAuth.test.ts b/apps/api/tests/routes/localAuth.test.ts index ee4cbb3..66d5eda 100644 --- a/apps/api/tests/routes/localAuth.test.ts +++ b/apps/api/tests/routes/localAuth.test.ts @@ -5,8 +5,9 @@ * Test 1: valid username+password → 200 { ok:true } + Set-Cookie for local-session * Test 2: wrong password → 401 { error: 'Invalid credentials' } * Test 3: unknown username → 401 with SAME body as Test 2 (no enumeration / no field discrimination) - * Test 4: 5 consecutive failures from one IP → 6th returns 429 + * Test 4: 5 consecutive failures for one username → 6th returns 429 * Test 5: 10 failures → 423 (lockedOut); a cleared map resets the counter + * Test 5b (CR-04): a 423 lockout auto-expires after LOCKOUT_TTL_MS (no admin reset needed) * Test 6: POST /api/auth/local/logout clears the local-session cookie (expired Set-Cookie) * Test 6b: GET /api/auth/local/logout (alias) also clears the local-session cookie * Test 7 (no-echo): malformed body (missing password) → 400 { error: 'Invalid request' }; @@ -246,8 +247,9 @@ describe('POST /api/auth/local/login', () => { expect(body.error).toBe('Account locked'); // Clear the map (simulates admin reset) → counter gone → next attempt is 401 again (not locked) + // CR-04: the limiter is keyed on the USERNAME ('alice'), not the IP. const { loginAttempts } = await import('../../src/routes/localAuth.js'); - loginAttempts.delete('10.0.0.2'); + loginAttempts.delete('alice'); const resAfterReset = await app.fetch( makeLoginRequest({ username: 'alice', password: 'wrong' }, '10.0.0.2') @@ -255,6 +257,36 @@ describe('POST /api/auth/local/login', () => { expect(resAfterReset.status).toBe(401); }); + it('Test 5b (CR-04): a 423 lockout auto-expires after the TTL — no admin reset needed', async () => { + mockCredRow = undefined; + + const app = await getApp(); + + // 10 failures → lockout for username 'bob' + for (let i = 0; i < 10; i++) { + await app.fetch(makeLoginRequest({ username: 'bob', password: 'wrong' }, '10.0.0.3')); + } + + // Confirm locked (423) + const resLocked = await app.fetch( + makeLoginRequest({ username: 'bob', password: 'wrong' }, '10.0.0.3'), + ); + expect(resLocked.status).toBe(423); + + // Simulate the TTL elapsing by back-dating lockedAt well past LOCKOUT_TTL_MS (15 min). + const { loginAttempts } = await import('../../src/routes/localAuth.js'); + const entry = loginAttempts.get('bob'); + expect(entry?.lockedOut).toBe(true); + if (entry) entry.lockedAt = Date.now() - 16 * 60 * 1000; + + // Next attempt: the lockout has expired → handler drops the entry and processes the + // login normally, so a wrong password is a fresh 401 (not a 423). CR-04: self-healing. + const resAfterTtl = await app.fetch( + makeLoginRequest({ username: 'bob', password: 'wrong' }, '10.0.0.3'), + ); + expect(resAfterTtl.status).toBe(401); + }); + it('Test 7 (no-echo): malformed body (missing password) → 400 { error: "Invalid request" }; body has no echoed value or Zod received field', async () => { const app = await getApp(); // Body with username but missing password (Zod will reject) diff --git a/apps/pwa/src/routes/LoginPage.tsx b/apps/pwa/src/routes/LoginPage.tsx index 22f3807..921178e 100644 --- a/apps/pwa/src/routes/LoginPage.tsx +++ b/apps/pwa/src/routes/LoginPage.tsx @@ -332,7 +332,8 @@ export function LoginPage({ authMode }: LoginPageProps) { }} >