diff --git a/apps/api/scripts/reset-admin.ts b/apps/api/scripts/reset-admin.ts index ec1675d..3bda451 100644 --- a/apps/api/scripts/reset-admin.ts +++ b/apps/api/scripts/reset-admin.ts @@ -51,16 +51,43 @@ function hashPassword(password: string): string { } // ── CLI arg parsing (no new deps — process.argv only) ─────────────────────────────────── -function parseArgs(argv: string[]): Record { - const result: Record = {}; +// WR-01: support both `--key=value` and `--key value`, and parse values EXPLICITLY rather +// than inferring an empty string whenever the next token starts with '--'. The old heuristic +// coerced `--password --foo` (and a legitimately `--`-prefixed or empty password) silently to +// ''. Here, known value-taking flags (--username, --password) always consume the next token +// verbatim as their value; the only boolean flag (--dry-run) takes no value. This keeps a +// password that begins with '--', or an intentionally empty password, intact. +const VALUE_FLAGS = new Set(['username', 'password']); +const BOOLEAN_FLAGS = new Set(['dry-run']); + +function parseArgs(argv: string[]): Record { + const result: Record = {}; for (let i = 0; i < argv.length; i++) { const arg = argv[i]; - if (arg.startsWith('--')) { - const key = arg.slice(2); - const value = argv[i + 1] !== undefined && !argv[i + 1].startsWith('--') ? argv[i + 1] : ''; - result[key] = value; - if (value) i++; // skip the value token + if (!arg.startsWith('--')) continue; + + const eq = arg.indexOf('='); + if (eq !== -1) { + // `--key=value` form — value is everything after the first '=', taken verbatim + // (so `--password=--weird` and `--password=` both work correctly). + result[arg.slice(2, eq)] = arg.slice(eq + 1); + continue; } + + const key = arg.slice(2); + if (BOOLEAN_FLAGS.has(key)) { + result[key] = ''; // presence-only flag; detected via hasOwnProperty + continue; + } + if (VALUE_FLAGS.has(key)) { + // Consume the NEXT token verbatim as the value — even if it starts with '--' or is + // empty. If there is no next token, record undefined (genuinely absent, not ''). + result[key] = argv[i + 1]; + if (i + 1 < argv.length) i++; // skip the consumed value token + continue; + } + // Unknown flag — record presence with no value (forward-compatible, no crash). + result[key] = ''; } return result; } @@ -120,7 +147,8 @@ try { // Existing local_credentials row — update password and ensure is_admin userId = lcRows[0].user_id; await conn.execute('UPDATE users SET is_admin = true, claimed = true WHERE id = ?', [userId]); - console.log(`[reset-admin] Found existing user id=${userId} for username="${username}"`); + // WR-01: do not echo the username — log only the resolved user id (no credential data). + console.log(`[reset-admin] Found existing user id=${userId}`); } else { // No existing row — insert a new user const displayName = username; @@ -130,7 +158,8 @@ try { [displayName], ); userId = (insertResult as unknown as { insertId: number }).insertId; - console.log(`[reset-admin] Created new user id=${userId} for username="${username}"`); + // WR-01: do not echo the username — log only the resolved user id. + console.log(`[reset-admin] Created new user id=${userId}`); } // ── Upsert local_credentials row ───────────────────────────────────────────────────── @@ -142,7 +171,7 @@ try { [userId, username, passwordHash], ); - console.log(`[reset-admin] Local credential upserted for user id=${userId} username="${username}"`); + console.log(`[reset-admin] Local credential upserted for user id=${userId}`); console.log(`[reset-admin] Done. User id=${userId} is now a local admin.`); } finally { await conn.end(); diff --git a/apps/api/src/auth/devBypass.ts b/apps/api/src/auth/devBypass.ts index d810838..2f2671b 100644 --- a/apps/api/src/auth/devBypass.ts +++ b/apps/api/src/auth/devBypass.ts @@ -44,15 +44,32 @@ export const DEV_USER = { color: COLOR_PALETTE[0], // '#4A90D9' — first palette slot } as const; +/** + * The shape stored on c.get('user') across the bypass, local-session, and OIDC paths. + * + * BL-04: oidcIss/oidcSub are NULLABLE. Local users have null OIDC fields, and + * localAuthMiddleware must NOT fabricate sentinel ('local'/String(id)) values — those + * share the uniqueness domain (uniq_oidc_identity) with real OIDC identities and could + * collide with a genuine (iss,sub) pair if ever persisted. DEV_USER carries non-null + * 'dev'/'dev-user' values and remains assignable to this widened shape. + */ +export interface ContextUser { + id: number; + oidcIss: string | null; + oidcSub: string | null; + displayName: string | null; + color: string; +} + /** * Extend Hono's ContextVariableMap so that c.get('user') / c.set('user', ...) - * are statically typed throughout the app. The value type is the DEV_USER shape, - * which is compatible with both the bypass path and any future app-level user object - * stored on context (they share the same id/displayName/color subset). + * are statically typed throughout the app. The value type is ContextUser — the shape + * shared by the dev-bypass path, the local-session path (nullable oidc fields), and any + * future app-level user object stored on context. */ declare module 'hono' { interface ContextVariableMap { - user: typeof DEV_USER; + user: ContextUser; } } @@ -114,10 +131,36 @@ export function devSessionCookieMiddleware(): MiddlewareHandler { // LOCAL_SESSION_SECRET not set — bypass mode exempts the secret requirement // (assertLocalSessionSecretSet skips when DEV_AUTH_BYPASS=true), but we cannot // issue a cookie without it. Degrade gracefully so devAuthBypass still works. - if (!process.env.LOCAL_SESSION_SECRET) { + const secret = process.env.LOCAL_SESSION_SECRET; + if (!secret) { return async (_c, next) => next(); } + // BL-01: do not treat "present" as "safe". The boot guard's length floor + // (assertLocalSessionSecretSet, >= 32 chars) is SKIPPED in bypass mode, so apply the + // same floor here before minting a real, signature-valid local-session JWT for DEV_USER + // (id=1). A short/forgeable secret must NOT issue a genuine session token. Degrade to a + // no-op so the cookie is never signed with a weak key. + if (secret.length < 32) { + console.warn( + '[devSessionCookieMiddleware] LOCAL_SESSION_SECRET is shorter than 32 characters — ' + + 'refusing to issue a dev local-session cookie. Generate a strong value with ' + + 'node scripts/generate-secrets.mjs.', + ); + return async (_c, next) => next(); + } + + // BL-01: warn loudly if the secret is the well-known dev placeholder. A genuine, + // signature-valid session token minted under this known value is trivially forgeable + // if the same secret ever leaks into a non-bypass environment. + if (secret === 'dev-secret-change-me-0000000000000000') { + console.warn( + '[devSessionCookieMiddleware] LOCAL_SESSION_SECRET is the well-known dev placeholder. ' + + 'This is acceptable ONLY for local dev/CI under DEV_AUTH_BYPASS — never reuse this ' + + 'value in any non-bypass or shared environment.', + ); + } + // Bypass active + secret set: issue a real local-session cookie for DEV_USER // on each request that does not already carry one. return async (c, next) => { diff --git a/apps/api/src/auth/linkNonceStore.ts b/apps/api/src/auth/linkNonceStore.ts new file mode 100644 index 0000000..a0cc2df --- /dev/null +++ b/apps/api/src/auth/linkNonceStore.ts @@ -0,0 +1,55 @@ +/** + * linkNonceStore.ts — single-use nonce store for the OIDC-link state (IN-04). + * + * POST /api/me/link-oidc mints a signed `state` JWT carrying a random `nonce` "to prevent + * replay". Previously the /callback handler never recorded or checked that nonce, so a + * captured state JWT was fully replayable within its 10-minute signature window — the nonce + * provided no actual protection (this underlies the BL-03 takeover concern). + * + * This in-memory store makes the nonce genuinely single-use: + * - registerLinkNonce(nonce, expEpochSeconds): called when the state is issued. + * - consumeLinkNonce(nonce): called on /callback; returns true exactly ONCE per nonce + * (and only while unexpired), false on replay / unknown / expired. + * + * In-memory is sufficient for a single-process household deployment (same scope as the + * loginAttempts limiter). Expired entries are swept opportunistically on each access so the + * map stays bounded. If this app ever runs multi-process, move this to Redis. + */ + +// nonce → expiry (epoch ms). Presence means "issued and not yet consumed". +const issuedNonces = new Map(); + +function sweepExpired(now: number): void { + for (const [nonce, expiresAt] of issuedNonces) { + if (now >= expiresAt) issuedNonces.delete(nonce); + } +} + +/** + * Record a freshly-issued link nonce as valid until expEpochSeconds (the state JWT's exp). + */ +export function registerLinkNonce(nonce: string, expEpochSeconds: number): void { + const now = Date.now(); + sweepExpired(now); + issuedNonces.set(nonce, expEpochSeconds * 1000); +} + +/** + * Consume a link nonce. Returns true exactly once for a known, unexpired nonce; false for + * any replay, unknown, or expired nonce. Single-use: the entry is deleted on first success. + */ +export function consumeLinkNonce(nonce: string): boolean { + const now = Date.now(); + sweepExpired(now); + const expiresAt = issuedNonces.get(nonce); + if (expiresAt === undefined || now >= expiresAt) return false; + issuedNonces.delete(nonce); // single-use + return true; +} + +/** + * Test-only: clear all issued nonces. + */ +export function _clearLinkNonces(): void { + issuedNonces.clear(); +} diff --git a/apps/api/src/auth/localAuthMiddleware.ts b/apps/api/src/auth/localAuthMiddleware.ts index b952137..65befe9 100644 --- a/apps/api/src/auth/localAuthMiddleware.ts +++ b/apps/api/src/auth/localAuthMiddleware.ts @@ -33,7 +33,7 @@ import { eq } from 'drizzle-orm'; import { db } from '../db/client.js'; import { users } from '../db/schema.js'; import { verifyLocalSessionCookie } from './localSession.js'; -import type { DEV_USER } from './devBypass.js'; +import type { ContextUser } from './devBypass.js'; /** * Returns a Hono MiddlewareHandler that: @@ -81,17 +81,20 @@ export function localAuthMiddleware(): MiddlewareHandler { return; } - // Populate c.get('user') with the same shape as DEV_USER (devBypass.ts ContextVariableMap). - // oidcIss/oidcSub: local users have nullable oidcIss/oidcSub — use fallback strings so the - // shape is compatible with typeof DEV_USER at runtime. Cast required because ContextVariableMap - // is narrowed to the const DEV_USER literal type. + // Populate c.get('user') with the ContextUser shape (devBypass.ts ContextVariableMap). + // BL-04: keep oidcIss/oidcSub as NULL for local users — do NOT fabricate + // 'local'/String(id) sentinels. Those values share the uniq_oidc_identity uniqueness + // domain with real OIDC identities, so persisting them (e.g. a future upsertUser call + // using these context values) would let two local users collide or a local user shadow + // a genuine OIDC identity. ContextUser widens oidcIss/oidcSub to string | null so no + // cast is needed. c.set('user', { id: row.id, - oidcIss: row.oidcIss ?? 'local', - oidcSub: row.oidcSub ?? String(row.id), + oidcIss: row.oidcIss ?? null, + oidcSub: row.oidcSub ?? null, displayName: row.displayName ?? null, color: row.color ?? '#4A90D9', - } as typeof DEV_USER); + } satisfies ContextUser); await next(); }; 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/auth/localSession.ts b/apps/api/src/auth/localSession.ts index 1308e8b..39017ec 100644 --- a/apps/api/src/auth/localSession.ts +++ b/apps/api/src/auth/localSession.ts @@ -27,8 +27,15 @@ import type { Context } from 'hono'; // Cookie name must be distinct from the OIDC cookie 'oidc-auth' (Pitfall 4) const COOKIE_NAME = 'local-session'; -// Session max age: default 1 day (86400s); configurable via LOCAL_SESSION_EXPIRES env -const SESSION_MAX_AGE_SECONDS = Number(process.env.LOCAL_SESSION_EXPIRES ?? 86400); +// Session max age: default 1 day (86400s); configurable via LOCAL_SESSION_EXPIRES env. +// IN-01: validate the coercion. A malformed value yields NaN, which would produce a JWT +// with exp = now + NaN (→ NaN) and a cookie maxAge: NaN — making verify behaviour +// "always expired" or "never expires" depending on the lib's NaN handling. Fall back to +// the 86400s default for any non-finite or non-positive value. +const SESSION_MAX_AGE_SECONDS = (() => { + const n = Number(process.env.LOCAL_SESSION_EXPIRES ?? 86400); + return Number.isFinite(n) && n > 0 ? n : 86400; +})(); /** * Issue a signed local-session JWT cookie for the given userId. @@ -98,9 +105,13 @@ export function clearLocalSessionCookie(c: Context): void { deleteCookie(c, COOKIE_NAME, { path: '/', httpOnly: true, - // Use secure:true for delete (browsers only accept the attribute in matching context) - // In practice this is safe because logout should happen over HTTPS in production. - secure: true, + // BL-02: mirror the issue-time `secure` logic. issueLocalSessionCookie sets + // secure:false over plain HTTP (non-production), and a browser will REJECT a + // Secure delete-cookie sent over HTTP — so a hard-coded secure:true left the + // local-session cookie uncleared on every non-HTTPS deployment (local dev and any + // HTTP-only self-host), leaving the user "logged in" after logout. Match the + // context so the deletion cookie is accepted. + secure: process.env.NODE_ENV === 'production', sameSite: 'Lax', }); } diff --git a/apps/api/src/auth/oidcConfig.ts b/apps/api/src/auth/oidcConfig.ts new file mode 100644 index 0000000..eaafdf3 --- /dev/null +++ b/apps/api/src/auth/oidcConfig.ts @@ -0,0 +1,91 @@ +/** + * oidcConfig.ts — centralized "is OIDC configured" resolution + endpoint discovery. + * + * WR-04: three sites previously had independent notions of whether OIDC is configured: + * - routes/authMode.ts → OIDC_ISSUER env OR app_config.oidc_issuer + * - auth/middleware.ts → injects issuer/client-id/external-url from app_config + * - routes/me.ts (link-oidc) → ONLY env (OIDC_ISSUER && OIDC_CLIENT_ID && OIDC_REDIRECT_URI) + * + * The me.ts divergence meant a wizard-configured-but-not-restarted instance reported + * oidcEnabled:true from /api/auth/mode (and showed the "Link OIDC" button) but link-oidc + * returned authorizationUrl:null. This helper makes the env-OR-app_config resolution a + * single source of truth. + * + * WR-02: me.ts also hardcoded Authelia's `/api/oidc/authorization` path. The project's + * design is to discover endpoints from the issuer's /.well-known/openid-configuration + * document (the whole reason @hono/oidc-auth is used). discoverAuthorizationEndpoint() + * resolves the real authorization_endpoint so any RFC-compliant provider works. + */ + +import { eq } from 'drizzle-orm'; +import { db } from '../db/client.js'; +import { appConfig } from '../db/schema.js'; + +export interface ResolvedOidcConfig { + issuer: string; + clientId: string; + redirectUri: string; +} + +/** + * Read a single app_config value (or null when absent). + */ +async function readAppConfig(key: string): Promise { + const [row] = await db + .select({ value: appConfig.value }) + .from(appConfig) + .where(eq(appConfig.key, key)) + .limit(1); + return row?.value ?? null; +} + +/** + * Resolve issuer + clientId + redirectUri using env first, then app_config (WR-04). + * + * Returns null when any of the three cannot be resolved from EITHER source — i.e. OIDC is + * not (fully) configured, so no authorization URL can be built. The redirect URI is taken + * from OIDC_REDIRECT_URI when present, else derived from the external app URL + * (OIDC_AUTH_EXTERNAL_URL env or app_config.app_external_url) as `${externalUrl}/callback`, + * matching the middleware's redirect-uri construction. + */ +export async function resolveOidcConfig(): Promise { + const issuer = process.env.OIDC_ISSUER ?? (await readAppConfig('oidc_issuer')); + const clientId = process.env.OIDC_CLIENT_ID ?? (await readAppConfig('oidc_client_id')); + + let redirectUri = process.env.OIDC_REDIRECT_URI ?? null; + if (!redirectUri) { + const externalUrl = + process.env.OIDC_AUTH_EXTERNAL_URL ?? (await readAppConfig('app_external_url')); + if (externalUrl) { + redirectUri = `${externalUrl.replace(/\/$/, '')}/callback`; + } + } + + if (!issuer || !clientId || !redirectUri) return null; + return { issuer, clientId, redirectUri }; +} + +/** + * Discover the provider's authorization_endpoint from its OIDC discovery document (WR-02). + * + * Fetches `${issuer}/.well-known/openid-configuration` (5s timeout, matching the setup + * wizard's validate/oidc step) and returns the `authorization_endpoint` URL. Returns null + * on any network error, non-2xx, or missing field — callers treat null as "cannot build + * the authorization URL" and degrade gracefully (the PWA disables the Link button). + */ +export async function discoverAuthorizationEndpoint(issuer: string): Promise { + try { + const res = await fetch(`${issuer.replace(/\/$/, '')}/.well-known/openid-configuration`, { + signal: AbortSignal.timeout(5000), + }); + if (!res.ok) return null; + const doc = (await res.json()) as { authorization_endpoint?: unknown }; + return typeof doc.authorization_endpoint === 'string' ? doc.authorization_endpoint : null; + } catch (err) { + console.error( + '[oidcConfig/discoverAuthorizationEndpoint]', + err instanceof Error ? err.message : String(err), + ); + return null; + } +} diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index 6616545..d1b2891 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -20,6 +20,8 @@ import { } from './auth/middleware.js'; import { devAuthBypass, devSessionCookieMiddleware } from './auth/devBypass.js'; import { localAuthMiddleware } from './auth/localAuthMiddleware.js'; +import { verifyLocalSessionCookie } from './auth/localSession.js'; +import { consumeLinkNonce } from './auth/linkNonceStore.js'; import { persistSessionCookie } from './auth/persistSessionCookie.js'; import { startBrokerPoller } from './broker/poller.js'; import { startOutboxWorker, initOutboxTrigger } from './broker/outboxWorker.js'; @@ -58,6 +60,7 @@ app.get('/callback', async (c) => { // Attempt to extract linkUserId from the signed state param BEFORE processOAuthCallback // consumes it. The state param may be our signed JWT (link mode) or a random string (normal). let linkUserId: number | null = null; + let linkNonce: string | null = null; const rawState = c.req.query('state'); if (rawState) { const secret = process.env.LOCAL_SESSION_SECRET; @@ -66,6 +69,7 @@ app.get('/callback', async (c) => { const payload = await Jwt.verify(rawState, secret, 'HS256'); if (typeof payload.linkUserId === 'number') { linkUserId = payload.linkUserId; + linkNonce = typeof payload.nonce === 'string' ? payload.nonce : null; } } catch { // Not our signed link state — normal OIDC callback, proceed normally. @@ -79,10 +83,41 @@ app.get('/callback', async (c) => { // Link mode: after session is established, bind the OIDC identity to the local user. if (linkUserId !== null) { try { + // IN-04: enforce SINGLE USE of the link nonce. The signed state JWT is otherwise + // replayable for its full 10-minute signature lifetime; consuming the nonce here means + // a captured state can be used at most once. A replay (already-consumed), unknown, or + // expired nonce is rejected before any binding occurs. + if (!linkNonce || !consumeLinkNonce(linkNonce)) { + console.warn('[callback] OIDC-link rejected: link nonce missing, replayed, or expired.'); + return c.redirect('/?error=oidc-link-conflict'); + } + + // BL-03: cross-check that the local session completing this callback is the SAME + // user the link flow was initiated for. The signed `state` JWT proves the state was + // minted by POST /api/me/link-oidc, but NOT that the person finishing the OIDC login + // is that user. Without this check, an attacker who gets a victim to complete an OIDC + // login while replaying a still-valid (10-min) captured link state would bind the + // ATTACKER's OIDC identity onto the VICTIM's account (account takeover). Require the + // initiating local session to still be present and to match linkUserId. + const sessionUserId = await verifyLocalSessionCookie(c); + if (sessionUserId !== linkUserId) { + console.warn( + '[callback] OIDC-link rejected: local session does not match link state (possible replay).', + ); + return c.redirect('/?error=oidc-link-conflict'); + } + const auth = await getAuth(c); if (auth) { const iss = (auth.iss as string | undefined) ?? ''; const sub = auth.sub ?? ''; + // BL-03: never bind on a blank/partial identity. linkOidcToUser writes oidc_iss/ + // oidc_sub AND deletes the user's local_credentials — binding empty iss/sub would + // both corrupt identity and lock the user out of BOTH auth methods. Reject instead. + if (!iss || !sub) { + console.warn('[callback] OIDC-link rejected: empty iss/sub from getAuth.'); + return c.redirect('/?error=oidc-link-conflict'); + } await linkOidcToUser(linkUserId, iss, sub); // On success: user is now OIDC-only; normal redirect via callbackResponse proceeds. } diff --git a/apps/api/src/routes/admin.ts b/apps/api/src/routes/admin.ts index 968bdd3..aa2c183 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'; @@ -77,6 +78,19 @@ const noEchoHook = (result: { success: boolean }, c: Context) => { } }; +/** + * WR-07: strictly parse a positive-integer route param. parseInt('12abc', 10) returns 12 + * and passes an isNaN guard, silently accepting malformed ids. Number('12abc') is NaN, so + * Number.isInteger(Number(raw)) rejects trailing garbage. Returns null for anything that is + * not a whole positive integer (empty, '12abc', '1.5', '-3', '0', etc.) so the caller can 400. + */ +function parsePositiveIntParam(raw: string | undefined): number | null { + if (raw === undefined || raw.trim() === '') return null; + const n = Number(raw); + if (!Number.isInteger(n) || n <= 0) return null; + return n; +} + // --------------------------------------------------------------------------- // GET /api/admin/members // @@ -140,6 +154,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; @@ -162,7 +180,7 @@ adminRouter.post( await tx.insert(localCredentials).values({ userId: newUserId, username, - passwordHash: hashPassword(initialPassword), + passwordHash: initialPasswordHash, }); }); @@ -212,17 +230,18 @@ adminRouter.post( '/members/:id/password', zValidator('json', resetPasswordSchema, noEchoHook), async (c) => { - const targetId = parseInt(c.req.param('id'), 10); - if (isNaN(targetId)) { + const targetId = parsePositiveIntParam(c.req.param('id')); + if (targetId === null) { return c.json({ error: 'Invalid member id' }, 400); } 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); @@ -234,9 +253,14 @@ 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 + // 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( @@ -305,8 +329,8 @@ adminRouter.get('/calendars', async (c) => { // --------------------------------------------------------------------------- adminRouter.put('/calendars/:id/shared', async (c) => { - const targetId = parseInt(c.req.param('id'), 10); - if (isNaN(targetId)) { + const targetId = parsePositiveIntParam(c.req.param('id')); + if (targetId === null) { return c.json({ error: 'Invalid calendar id' }, 400); } diff --git a/apps/api/src/routes/localAuth.ts b/apps/api/src/routes/localAuth.ts index 5069c99..68bd47e 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,61 +60,122 @@ 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()); +} + +/** + * IN-03: evict stale rate-limit entries to bound the in-memory map size. + * + * An entry is stale (and safe to drop) when it is neither inside an active rate-limit + * window nor inside an active lockout TTL: + * - locked-out entries expire LOCKOUT_TTL_MS after lockedAt + * - non-locked entries expire once their lockedUntil window has passed + * + * Called opportunistically at the top of each login request. Dropping an entry is + * behaviourally identical to the entry having naturally expired, so eviction never + * weakens the brute-force defense — it only reclaims memory for identities no longer + * under active rate-limiting. + */ +function evictStaleLoginAttempts(now: number): void { + for (const [k, v] of loginAttempts) { + const lockoutExpired = v.lockedOut ? now - v.lockedAt >= LOCKOUT_TTL_MS : true; + const windowExpired = now >= v.lockedUntil; + if (lockoutExpired && windowExpired) { + loginAttempts.delete(k); + } + } +} // 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 // --------------------------------------------------------------------------- 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(); + // IN-03: opportunistically reclaim memory from entries that are no longer actively + // rate-limited or locked out. Cheap at household scale; bounds the map under input churn. + evictStaleLoginAttempts(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; + // WR-06: do NOT extend lockedUntil here. This request was itself REJECTED by the window; + // re-arming the cooldown on every blocked attempt let an attacker who keeps hammering the + // endpoint slide the window forward forever, so a legitimate user behind the same identity + // could never get back in even after pausing. The window stays anchored to when it was + // first armed (in the failure path below); it expires on schedule regardless of rejected + // traffic. The lockout (423) still triggers once the failure count crosses the threshold. + live.lockedOut = live.count >= LOCKOUT_FAILURES; + if (live.lockedOut && live.lockedAt === 0) 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 { @@ -124,23 +193,25 @@ 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 - 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/src/routes/me.ts b/apps/api/src/routes/me.ts index 6587312..3cf8961 100644 --- a/apps/api/src/routes/me.ts +++ b/apps/api/src/routes/me.ts @@ -41,6 +41,8 @@ import { CredentialValidationError, } from '../broker/credentialSync.js'; import { hashPassword, verifyPassword } from '../auth/localCredentials.js'; +import { resolveOidcConfig, discoverAuthorizationEndpoint } from '../auth/oidcConfig.js'; +import { registerLinkNonce } from '../auth/linkNonceStore.js'; // Side-effect import: brings in the ContextVariableMap augmentation for c.get('user') import '../auth/devBypass.js'; @@ -254,16 +256,21 @@ 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) { - 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 { await db .update(localCredentials) - .set({ passwordHash: hashPassword(newPassword) }) + .set({ passwordHash: await hashPassword(newPassword) }) .where(eq(localCredentials.userId, currentUserId)); return c.json({ ok: true }, 200); @@ -313,28 +320,37 @@ meRouter.post('/link-oidc', async (c) => { // T-19-09: nonce prevents state replay attacks (each link attempt gets a fresh nonce) const nonce = randomBytes(16).toString('hex'); const now = Math.floor(Date.now() / 1000); + const exp = now + 600; // 10-minute window const signedState = await Jwt.sign( - { linkUserId: currentUserId, nonce, iat: now, exp: now + 600 }, // 10-minute window + { linkUserId: currentUserId, nonce, iat: now, exp }, secret, 'HS256', ); - // Build the OIDC authorization URL if OIDC is configured (else return null — PWA disables button) - const issuer = process.env.OIDC_ISSUER ?? null; - const clientId = process.env.OIDC_CLIENT_ID ?? null; - const redirectUri = process.env.OIDC_REDIRECT_URI ?? null; + // IN-04: record the nonce so /callback can enforce SINGLE USE. Without this the signed + // state JWT is fully replayable for its 10-minute signature lifetime and the nonce is + // decorative. registerLinkNonce keeps it valid only until the state's own exp. + registerLinkNonce(nonce, exp); + // Build the OIDC authorization URL if OIDC is configured (else return null — PWA disables button). + // WR-04: resolve issuer/clientId/redirectUri from env-OR-app_config (single source of truth, + // consistent with /api/auth/mode and the OIDC fallback middleware) so a wizard-configured + // instance does not report oidcEnabled:true while returning authorizationUrl:null here. + // WR-02: discover the authorization_endpoint from the provider's discovery document instead + // of hardcoding Authelia's /api/oidc/authorization path. let authorizationUrl: string | null = null; - if (issuer && clientId && redirectUri) { - // Construct the authorization URL. plan 19-03 will handle the full PKCE flow; - // for now encode the signed state so the callback can read linkUserId. - const url = new URL(`${issuer.replace(/\/$/, '')}/api/oidc/authorization`); - url.searchParams.set('response_type', 'code'); - url.searchParams.set('client_id', clientId); - url.searchParams.set('redirect_uri', redirectUri); - url.searchParams.set('scope', 'openid profile email'); - url.searchParams.set('state', signedState); - authorizationUrl = url.toString(); + const oidc = await resolveOidcConfig(); + if (oidc) { + const authEndpoint = await discoverAuthorizationEndpoint(oidc.issuer); + if (authEndpoint) { + const url = new URL(authEndpoint); + url.searchParams.set('response_type', 'code'); + url.searchParams.set('client_id', oidc.clientId); + url.searchParams.set('redirect_uri', oidc.redirectUri); + url.searchParams.set('scope', 'openid profile email'); + url.searchParams.set('state', signedState); + authorizationUrl = url.toString(); + } } return c.json({ signedState, authorizationUrl }, 200); diff --git a/apps/api/tests/auth/localAuthMiddleware.test.ts b/apps/api/tests/auth/localAuthMiddleware.test.ts index 4dc9590..6bba8ab 100644 --- a/apps/api/tests/auth/localAuthMiddleware.test.ts +++ b/apps/api/tests/auth/localAuthMiddleware.test.ts @@ -155,6 +155,36 @@ describe('localAuthMiddleware', () => { expect(u.color).toBe('#FF5733'); }); + it('Test 1c (BL-04): local user with null DB oidc fields → context oidcIss/oidcSub are NULL (no fabricated sentinels)', async () => { + mockVerifyResult = 9; + mockDbSelectResult.push({ + id: 9, + oidcIss: null, // local user — no OIDC identity bound + oidcSub: null, + displayName: 'Local Member', + color: '#22AA88', + }); + + const localAuthMiddleware = await getMiddleware(); + const app = new Hono(); + let capturedUser: unknown; + + app.use('/api/*', localAuthMiddleware()); + app.get('/api/test', (c) => { + capturedUser = c.get('user'); + return c.json({ ok: true }); + }); + + const res = await app.request('/api/test'); + expect(res.status).toBe(200); + const u = capturedUser as { id: number; oidcIss: string | null; oidcSub: string | null }; + expect(u.id).toBe(9); + // BL-04: must be null — NOT 'local' / String(id) sentinels that share the + // uniq_oidc_identity domain with real OIDC identities. + expect(u.oidcIss).toBeNull(); + expect(u.oidcSub).toBeNull(); + }); + it('Test 2: no cookie → pure passthrough; c.get("user") remains unset (Pitfall-1 guard)', async () => { mockVerifyResult = null; // No cookie / invalid diff --git a/apps/api/tests/auth/localCredentials.test.ts b/apps/api/tests/auth/localCredentials.test.ts index 5bc5433..55b03e8 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 @@ -13,36 +16,72 @@ */ import { describe, it, expect } from 'vitest'; +import { scryptSync, randomBytes } from 'node:crypto'; 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 6 (IN-02): a hash produced by the INLINED scrypt parameters round-trips against the canonical verifyPassword', async () => { + // IN-02: the PHC scrypt hash is copy-pasted in three places — the canonical module + // (src/auth/localCredentials.ts), the break-glass CLI (scripts/reset-admin.ts), and the + // CI seed step (.gitea/workflows/ci.yml). If the parameters ever drift, those inlined + // copies would produce hashes the canonical verifyPassword cannot validate, silently + // breaking login for seeded/reset accounts. This test pins the inlined parameter set: + // if anyone changes N/r/p/KEY_LEN in the canonical module without updating the inlined + // copies (or vice versa), this round-trip fails loudly in CI. + // + // These constants MUST match scripts/reset-admin.ts and .gitea/workflows/ci.yml exactly. + const INLINE_N = 16384; + const INLINE_R = 8; + const INLINE_P = 1; + const INLINE_KEY_LEN = 32; + + const password = 'inline-roundtrip-pw'; + const salt = randomBytes(16); + const hash = scryptSync(password, salt, INLINE_KEY_LEN, { + N: INLINE_N, + r: INLINE_R, + p: INLINE_P, + }); + const inlineEncoded = [ + 'scrypt', + INLINE_N, + INLINE_R, + INLINE_P, + salt.toString('base64url'), + hash.toString('base64url'), + ].join('$'); + + // The canonical verifyPassword must validate a hash produced by the inlined params. + expect(await verifyPassword(inlineEncoded, password)).toBe(true); + expect(await verifyPassword(inlineEncoded, 'wrong')).toBe(false); + }); + + 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..0a12737 100644 --- a/apps/api/tests/routes/admin.test.ts +++ b/apps/api/tests/routes/admin.test.ts @@ -483,6 +483,19 @@ describe('PUT /api/admin/calendars/:id/shared', () => { .limit(1); expect(rowA.isShared).toBe(true); }); + + it('WR-07: rejects a calendar id with trailing garbage (e.g. "1abc") with 400', async () => { + const adminId = await seedUser('admin-shared-badid', true); + currentDevUserId = adminId; + const app = await getApp(); + + // parseInt('1abc', 10) === 1 would have silently accepted this; the strict + // Number.isInteger parse must reject it as a malformed id. + const res = await app.fetch(jsonRequest('PUT', '/api/admin/calendars/1abc/shared')); + expect(res.status).toBe(400); + const body = (await res.json()) as { error: string }; + expect(body.error).toBe('Invalid calendar id'); + }); }); // =========================================================================== @@ -890,7 +903,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 +980,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 () => { @@ -1023,4 +1036,29 @@ describe('POST /api/admin/members', () => { expect(adminRow).toBeDefined(); expect(adminRow!.hasLocalCredential).toBe(false); }); + + it('WR-05 (no-echo): malformed create-member body never echoes the submitted password or Zod received field', async () => { + const adminId = await seedUser('admin-create-noecho', true); + currentDevUserId = adminId; + const app = await getApp(); + + // initialPassword too short (< 8) → Zod rejects. The noEchoHook must return only + // { error: 'Invalid request' } and NEVER leak the submitted password or Zod's + // issues[].received field (T-19-06 / the T-19-14 leak this guards against). + const submittedPassword = 'shortpw-secret'; + const res = await app.fetch( + jsonRequest('POST', '/api/admin/members', { + displayName: 'No Echo', + username: `noecho-${randomUUID()}`, + initialPassword: submittedPassword.slice(0, 3), // 3 chars — fails min(8) + }), + ); + expect(res.status).toBe(400); + const bodyText = await res.text(); + expect(bodyText).not.toContain('received'); + expect(bodyText).not.toContain('issues'); + expect(bodyText).not.toContain(submittedPassword.slice(0, 3)); + const parsed = JSON.parse(bodyText) as { error: string }; + expect(parsed.error).toBe('Invalid request'); + }); }); diff --git a/apps/api/tests/routes/localAuth.test.ts b/apps/api/tests/routes/localAuth.test.ts index ee4cbb3..6f511d3 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' }; @@ -163,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(); @@ -178,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(); @@ -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/api/tests/routes/me.test.ts b/apps/api/tests/routes/me.test.ts index 7eb6e30..6f50480 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 → 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'; - const storedHash = hashPassword(realPassword); + const storedHash = await hashPassword(realPassword); let updateWasCalled = false; let callCount = 0; @@ -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 @@ -401,6 +403,27 @@ describe('POST /api/me/password — self-change password (AUTH-LOCAL-09)', () => expect(res.status).toBe(404); }); + + it('Test 4 (WR-05 no-echo): malformed body never echoes the submitted password or Zod received field', async () => { + // newPassword too short (< 8) → Zod rejects via meNoEchoHook. The response must be + // ONLY { error: 'Invalid request' } and must NOT leak the submitted password or the + // Zod issues[].received field (T-19-06). + const { app } = await import('../../src/index.js'); + const submitted = 'my-secret-current-pw'; + const res = await app.request('/api/me/password', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ currentPassword: submitted, newPassword: 'short' }), + }); + + expect(res.status).toBe(400); + const bodyText = await res.text(); + expect(bodyText).not.toContain(submitted); + expect(bodyText).not.toContain('received'); + expect(bodyText).not.toContain('issues'); + const parsed = JSON.parse(bodyText) as { error: string }; + expect(parsed.error).toBe('Invalid request'); + }); }); // --------------------------------------------------------------------------- diff --git a/apps/pwa/src/api/client.ts b/apps/pwa/src/api/client.ts index 7067342..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 }; @@ -168,10 +175,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 +194,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'); @@ -219,11 +239,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 +258,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.'); 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) { }} >