merge(19): land code-review fixes (CR-01..04, BL-01..04, WR-01..07, IN-01..04)

This commit is contained in:
Lucas Berger
2026-06-17 20:39:49 -04:00
19 changed files with 738 additions and 137 deletions
+39 -10
View File
@@ -51,16 +51,43 @@ function hashPassword(password: string): string {
}
// ── CLI arg parsing (no new deps — process.argv only) ───────────────────────────────────
function parseArgs(argv: string[]): Record<string, string> {
const result: Record<string, string> = {};
// 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<string, string | undefined> {
const result: Record<string, string | undefined> = {};
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();
+48 -5
View File
@@ -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) => {
+55
View File
@@ -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<string, number>();
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();
}
+11 -8
View File
@@ -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();
};
+38 -8
View File
@@ -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<Buffer> {
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<string> {
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<true> if candidate matches the stored hash; Promise<false> 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<boolean> {
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),
+16 -5
View File
@@ -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',
});
}
+91
View File
@@ -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<string | null> {
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<ResolvedOidcConfig | null> {
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<string | null> {
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;
}
}
+35
View File
@@ -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.
}
+32 -8
View File
@@ -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);
}
+101 -30
View File
@@ -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<string, { count: number; lockedUntil: number; lockedOut: boolean }>();
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<string> = 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) {
+35 -19
View File
@@ -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);
@@ -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
+55 -16
View File
@@ -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');
+41 -3
View File
@@ -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');
});
});
+36 -4
View File
@@ -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)
+29 -6
View File
@@ -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');
});
});
// ---------------------------------------------------------------------------
+36 -12
View File
@@ -141,9 +141,14 @@ export async function fetchLocalLogout(): Promise<void> {
* 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 ────────────────────────────────────────────────────────────────
+8 -2
View File
@@ -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.');
+2 -1
View File
@@ -332,7 +332,8 @@ export function LoginPage({ authMode }: LoginPageProps) {
}}
>
<AlertCircle size={16} aria-hidden="true" style={{ flexShrink: 0 }} />
This account is temporarily locked. Contact your admin to reset access.
Too many failed attempts for this account. Try again in about 15 minutes, or
contact your admin to reset access.
</div>
)}