From 4b635784e42325cd36ea0836adfad263db76a168 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 17 Jun 2026 20:08:26 -0400 Subject: [PATCH] docs(19): add code review report --- .../19-local-auth-no-oidc-mode/19-REVIEW.md | 387 ++++++++++++++++++ 1 file changed, 387 insertions(+) create mode 100644 .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md diff --git a/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md new file mode 100644 index 0000000..eb38370 --- /dev/null +++ b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md @@ -0,0 +1,387 @@ +--- +phase: 19-local-auth-no-oidc-mode +reviewed: 2026-06-17T00:00:00Z +depth: deep +files_reviewed: 39 +files_reviewed_list: + - apps/api/scripts/reset-admin.ts + - apps/api/src/auth/devBypass.ts + - apps/api/src/auth/linkOidc.ts + - apps/api/src/auth/localAuthMiddleware.ts + - apps/api/src/auth/localCredentials.ts + - apps/api/src/auth/localSession.ts + - apps/api/src/auth/middleware.ts + - apps/api/src/db/migrations/0003_warm_deathstrike.sql + - apps/api/src/db/schema.ts + - apps/api/src/index.ts + - apps/api/src/lib/bootGuards.ts + - apps/api/src/routes/admin.ts + - apps/api/src/routes/authMode.ts + - apps/api/src/routes/localAuth.ts + - apps/api/src/routes/me.ts + - apps/api/tests/auth/localAuthMiddleware.test.ts + - apps/api/tests/auth/localCredentials.test.ts + - apps/api/tests/auth/localSession.test.ts + - apps/api/test/setup.ts + - apps/api/tests/lib/requireAdmin.test.ts + - apps/api/tests/routes/admin.test.ts + - apps/api/tests/routes/authMode.test.ts + - apps/api/tests/routes/lists.test.ts + - apps/api/tests/routes/localAuth.test.ts + - apps/api/tests/routes/me.test.ts + - apps/api/tests/routes/push.test.ts + - apps/api/tests/routes/setup.test.ts + - apps/pwa/e2e/global-setup.ts + - apps/pwa/e2e/login.spec.ts + - apps/pwa/src/api/client.ts + - apps/pwa/src/App.test.tsx + - apps/pwa/src/App.tsx + - apps/pwa/src/components/BrandSlot.tsx + - apps/pwa/src/components/InstructionSheet.test.tsx + - apps/pwa/src/components/SettingsSheet.tsx + - apps/pwa/src/routes/AdminPage.tsx + - apps/pwa/src/routes/LoginPage.tsx + - apps/pwa/src/styles/tokens.css + - .gitea/workflows/ci.yml + - scripts/generate-secrets.mjs +findings: + critical: 4 + blocker: 4 + warning: 7 + info: 4 + total: 15 +status: issues_found +--- + +# Phase 19: Code Review Report + +**Reviewed:** 2026-06-17 +**Depth:** deep +**Files Reviewed:** 39 (auth source + routes + PWA + CI) +**Status:** issues_found + +## Summary + +Phase 19 adds a local username/password authentication mode alongside the existing +OIDC flow. The cryptographic core (scrypt PHC hashing, constant-time compare, dummy-hash +timing defense, HS256 session JWT with boot guards) is implemented carefully and is sound. +The middleware chain (`devAuthBypass → devSessionCookie → localAuthMiddleware → OIDC guard`) +and the admin-privilege boundary (`requireAdmin` DB-enforced on every `/api/admin/*` request) +are correct. + +The serious problems are at the **client↔server API contract boundary** — the exact place +a deep cross-file review is meant to catch. Three Phase-19 client functions in +`apps/pwa/src/api/client.ts` disagree with their server routes on field names, response +shape, or status-code handling, so the corresponding features (OIDC-link, admin +create-member, and self-service password change error handling) are broken end-to-end +despite each side individually passing its own unit tests. There is also an +authentication availability defect: the per-IP login lockout is mislabeled as +"account locked," is global, and never expires — a single attacker IP can permanently +deny login for the whole household with no self-recovery path. + +## Critical Issues + +### CR-01: OIDC-link flow is broken — client/server response-shape mismatch + +**File:** `apps/pwa/src/api/client.ts:226-238` and `apps/api/src/routes/me.ts:301-341` +**Issue:** `fetchLinkOidc()` reads `result.redirectUrl` and its return type is +`{ redirectUrl: string }`. The server's `POST /api/me/link-oidc` returns +`{ signedState, authorizationUrl }` — there is no `redirectUrl` key. The caller +(Surface 13 "Link OIDC") will navigate to `undefined`, so the entire OIDC-link feature +(AUTH-LOCAL-10) cannot work in the browser. Each side's own unit tests pass because +neither test crosses the boundary. Additionally `authorizationUrl` can legitimately be +`null` (OIDC unconfigured), which the client type does not model. +**Fix:** Make the contract agree. Either return `redirectUrl` from the server, or read +`authorizationUrl` on the client and handle `null`: +```ts +export async function fetchLinkOidc(): Promise<{ authorizationUrl: string | null }> { + // ... + return res.json() as Promise<{ signedState: string; authorizationUrl: string | null }>; +} +// caller: +const { authorizationUrl } = await fetchLinkOidc(); +if (authorizationUrl) window.location.href = authorizationUrl; +``` + +### CR-02: Admin "create member" always fails — request field-name mismatch + +**File:** `apps/pwa/src/api/client.ts:176-194` and `apps/api/src/routes/admin.ts:123-133` +**Issue:** `fetchCreateMember` POSTs `{ displayName, username, password }`. The server's +`createMemberSchema` requires `{ displayName, username, initialPassword }`. The `password` +field is ignored and `initialPassword` is missing, so Zod validation fails and the route +returns `400 { error: 'Invalid request' }` (via `noEchoHook`) for *every* valid admin +attempt to create a local member (AUTH-LOCAL-07). The Admin UI maps a non-409 error to +the generic "Something went wrong" banner, so the admin can never create an account. +**Fix:** Send the field the server expects: +```ts +body: JSON.stringify({ + displayName: body.displayName, + username: body.username, + initialPassword: body.password, +}), +``` + +### CR-03: Self-service password change logs the user out on a wrong current password + +**File:** `apps/pwa/src/api/client.ts:148-165` and `apps/api/src/routes/me.ts:233-261` +**Issue:** `fetchChangePassword` treats `res.status === 401` as `SessionExpiredError`, +which the global MutationCache handler interprets as "session expired → arm the +re-auth interstitial / redirect to login." But `POST /api/me/password` returns **401** +`{ error: 'Current password incorrect' }` when the supplied current password is wrong +(me.ts:259-260). So a user who simply mistypes their current password is forcibly logged +out instead of seeing "current password incorrect." The client doc comment even claims +"401 → wrong current password" while the code routes 401 to `SessionExpiredError`. The +documented 422 validation branch also never fires — the server returns 400 (noEchoHook) +or 404, not 422. +**Fix:** Distinguish auth-expiry from an in-app 401. Have the route return a distinct +status for "wrong current password" (e.g. 403 or a body code), and branch on it client-side +before treating 401 as session expiry: +```ts +if (res.status === 401) { + const body = await res.json().catch(() => ({})); + if (body?.error === 'Current password incorrect') throw new Error('wrong-current'); + throw new SessionExpiredError(); +} +``` + +### CR-04: Login lockout is per-IP, global, permanent, and unrecoverable + +**File:** `apps/api/src/routes/localAuth.ts:55-140` +**Issue:** Multiple correctness/availability defects in one mechanism: +1. The `loginAttempts` map is keyed by **IP**, yet the 423 response and the PWA banner say + "This account is temporarily locked." It is neither account-scoped nor temporary. +2. Once `count >= LOCKOUT_FAILURES (10)`, `lockedOut` is set permanently. The only documented + clear path is "admin password reset" — but no admin route ever clears `loginAttempts` + (admin.ts reset-password updates the hash, not the in-memory map). So **the lockout is + genuinely unrecoverable without a process restart**. +3. Because it is per-IP and all household traffic arrives via the Pangolin tunnel with the + same `X-Forwarded-For` first hop, one bad actor (or one user fat-fingering 10 times) can + lock out **every** member at that egress IP. This is a self-inflicted DoS on a 2-person + household whose entire reason for existing is low-friction access. +4. `X-Forwarded-For` is attacker-controllable on any request that does not pass through the + trusted proxy; an attacker can rotate the header to get unlimited fresh rate-limit + buckets, defeating the brute-force defense entirely while still being able to lock + *other* identities by spoofing their IP if it were ever known. +**Fix:** Re-scope the limiter to the submitted username (not IP), make the 423 lockout +expire on a timer (or actually wire admin reset to clear it), correct the banner copy, and +only trust `X-Forwarded-For` when the request demonstrably came from the known proxy +(or use the leftmost-trusted hop). At minimum, give the lockout a TTL so a restart is not +required: +```ts +// derive key from the validated username, and expire lockout after N minutes +const LOCKOUT_TTL_MS = 15 * 60 * 1000; +if (attempt?.lockedOut && Date.now() < attempt.lockedUntil) { /* 423 */ } +``` + +## Blockers + +### BL-01: `devSessionCookieMiddleware` issues a session for user id=1 without verifying the user exists or the secret is strong + +**File:** `apps/api/src/auth/devBypass.ts:102-131` and `apps/api/src/lib/bootGuards.ts:53-66` +**Issue:** In dev-bypass mode the boot guard `assertLocalSessionSecretSet()` is *skipped* +entirely (returns early when `DEV_AUTH_BYPASS==='true'`). `devSessionCookieMiddleware` then +only checks that `LOCAL_SESSION_SECRET` is *present* (truthy), not that it is ≥32 chars, and +mints a real, fully-valid `local-session` JWT for `DEV_USER.id (=1)`. The CI sets a weak +fixed secret `'dev-secret-change-me-0000000000000000'`. Any cookie minted under bypass is a +genuine, signature-valid session token for user 1 — if that same weak/known secret is ever +present in a non-bypass environment (e.g. an operator copies the dev compose), forged +sessions are trivial. The hard `NODE_ENV==='production'` guard mitigates the worst case, but +this is a latent footgun: the boot guard's length check is the documented defense and it is +bypassed here. +**Fix:** Apply the same `secret.length >= 32` floor inside `devSessionCookieMiddleware` +before issuing, and emit a loud warning if the dev secret is the placeholder value. Do not +treat "present" as "safe." + +### BL-02: Logout cannot clear the cookie in non-production (attribute mismatch) + +**File:** `apps/api/src/auth/localSession.ts:53-59` vs `97-106` +**Issue:** `issueLocalSessionCookie` sets `secure: process.env.NODE_ENV === 'production'` +(i.e. `secure:false` in dev/test over HTTP). `clearLocalSessionCookie` hard-codes +`secure: true`. Browsers require the `Secure` attribute on a deletion cookie to match the +context: over plain HTTP a `Secure` delete-cookie is rejected, so `POST /local/logout` +returns 200 but the `local-session` cookie is **not actually cleared** in any non-HTTPS +deployment (local dev, and any HTTP-only self-host). The user appears logged in after +"logout." The inline comment acknowledges the mismatch but waves it away with "logout should +happen over HTTPS" — that is an unsafe assumption for a self-hosted app that explicitly +supports private-IP/HTTP internal access. +**Fix:** Mirror the issue-time logic on delete: +```ts +deleteCookie(c, COOKIE_NAME, { + path: '/', httpOnly: true, + secure: process.env.NODE_ENV === 'production', + sameSite: 'Lax', +}); +``` + +### BL-03: OIDC-link binding swallows `processOAuthCallback` failure and can bind on a stale/blank identity + +**File:** `apps/api/src/index.ts:57-101` +**Issue:** In the `/callback` link path the code calls `processOAuthCallback(c)` then +`getAuth(c)` and binds whatever `iss`/`sub` it finds to `linkUserId`. Two problems: +(1) `iss`/`sub` fall back to `''` (`auth.iss ?? ''`, `auth.sub ?? ''`); if `getAuth` returns +a partially-populated session, `linkOidcToUser(linkUserId, '', '')` will write +`oidc_iss=''`/`oidc_sub=''` onto the user and **delete their local_credentials** — locking +them out of both auth methods. (2) `linkUserId` comes from a JWT signed with +`LOCAL_SESSION_SECRET`, but nothing verifies the *currently authenticated session* matches +`linkUserId`; the signed-state CSRF defense assumes the state can only have been produced by +`POST /api/me/link-oidc`, but the callback binds to `linkUserId` regardless of who completes +the OIDC login. An attacker who can get a victim to complete an OIDC login while replaying a +captured (still-valid, 10-min) link state binds the *attacker's* OIDC identity to the +*victim's* account — account takeover. +**Fix:** Reject the bind unless `iss` and `sub` are both non-empty, and cross-check that the +OIDC identity being bound is the one the initiating user intended (e.g. require the +post-callback session's subject to be confirmed by the user, or bind only when the +initiating local session is still present and matches `linkUserId`). Never call +`linkOidcToUser` with empty iss/sub. + +### BL-04: `localAuthMiddleware` fabricates `oidcSub` collisions for local users + +**File:** `apps/api/src/auth/localAuthMiddleware.ts:88-94` and `apps/api/src/db/schema.ts:60-64` +**Issue:** When populating context for a local user with null OIDC fields, the middleware +substitutes `oidcIss: 'local'` and `oidcSub: String(row.id)`. This is only a context shape +and is not persisted by the middleware — but `me.ts:resolveUserId` (the OIDC path) and +`upsertUser` key identity on `iss+sub`, and the `users` table has +`unique('uniq_oidc_identity').on(oidcIss, oidcSub)`. If any code path ever upserts using the +context's `('local', String(id))` pair (e.g. a future call to `upsertUser` with these +values), two local users would deterministically collide or a local user could shadow a real +OIDC identity whose `(iss,sub)` happened to equal `('local','')`. The fabricated values +leak a synthetic identity namespace that overlaps the real one. +**Fix:** Keep the context `oidcIss/oidcSub` as `null` for local users (widen the +`ContextVariableMap` `user` type to allow null) rather than inventing `'local'`/`String(id)` +sentinels that share a uniqueness domain with real OIDC identities. + +## Warnings + +### WR-01: `reset-admin.ts` interpolates the username into a log line and trusts `--password ''` + +**File:** `apps/api/scripts/reset-admin.ts:60-66, 123, 133` +**Issue:** Two issues. (1) The arg parser treats any token starting with `--` as a new flag, +so `--password --foo` yields `password=''`; combined with the dry-run branch the validation +is loose. More importantly a password that legitimately begins with `--` (or is the empty +string) is silently coerced to `''`. (2) The username is interpolated directly into +`console.log(... username="${username}")`; while not an injection into SQL (queries are +parameterized — good), logging the username is a minor info disclosure for a break-glass tool +and inconsistent with the password-never-logged contract. +**Fix:** Parse `--password=value` and `--password value` explicitly; do not infer empty +strings from a following flag. Avoid echoing the username, or document it as acceptable. + +### WR-02: `me.ts` builds the OIDC authorization URL with a hardcoded Authelia path + +**File:** `apps/api/src/routes/me.ts:331` +**Issue:** `new URL(`${issuer}/api/oidc/authorization`)` hardcodes Authelia's authorization +endpoint path. The project's stated design is to discover endpoints via +`/.well-known/openid-configuration` (the whole reason `@hono/oidc-auth` is used). Any +non-Authelia or differently-mounted provider will get a wrong URL. This is a latent +correctness bug that compounds CR-01. +**Fix:** Resolve the authorization endpoint from the discovery document rather than assuming +`/api/oidc/authorization`. + +### WR-03: `scryptSync` blocks the event loop on the login hot path + +**File:** `apps/api/src/auth/localCredentials.ts:42-53, 71-90` and `localAuth.ts:127-129` +**Issue:** `verifyPassword` always runs `scryptSync` (N=16384) synchronously, including the +dummy-hash branch on every failed/unknown login. The file comment justifies this for a +2-person household, but combined with the per-IP rate limiter and the always-run dummy hash, +a burst of unauthenticated `POST /local/login` requests can pin the single Node event loop +(each scrypt is ~tens of ms of blocking CPU) and stall *all* other API traffic — a cheap +unauthenticated DoS. The rate limiter does not protect this because the scrypt runs *before* +the failure counter is consulted on the dummy path for new IPs. +**Fix:** Use `promisify(scrypt)` (async) so hashing does not block the loop, as the comment +itself suggests. This keeps the timing-defense property while preventing loop starvation. + +### WR-04: `authMode` / OIDC-enabled detection diverges across three files + +**File:** `apps/api/src/routes/authMode.ts:33-49`, `apps/api/src/auth/middleware.ts:62-114`, +`apps/api/src/routes/me.ts:323-328` +**Issue:** Three independent notions of "is OIDC configured": `authMode` checks +`OIDC_ISSUER` env OR `app_config.oidc_issuer`; the fallback middleware injects +issuer/client-id/external-url from app_config; but `me.ts` link-oidc only builds a URL when +`OIDC_ISSUER && OIDC_CLIENT_ID && OIDC_REDIRECT_URI` are all in **env** (it never consults +app_config). So a wizard-configured-but-not-restarted instance reports `oidcEnabled:true` +from `/api/auth/mode`, shows the "Link OIDC" button, but `link-oidc` returns +`authorizationUrl:null` — inconsistent state surfaced to the user. +**Fix:** Centralize the "OIDC configured" resolution (env-or-app_config) in one helper and +use it in all three sites. + +### WR-05: `noEchoHook` return value is ignored by `@hono/zod-validator` in one of two styles + +**File:** `apps/api/src/routes/localAuth.ts:39-43`, `apps/api/src/routes/admin.ts:74-78`, +`apps/api/src/routes/me.ts:178-182` +**Issue:** The hook signature is `(result, c)` and returns `c.json(...)` only on failure. This +relies on zValidator short-circuiting when the hook returns a Response. That contract holds +for current `@hono/zod-validator`, but the hook does not `return` anything on success and does +not assert `result.success` narrows the type, so a future validator version that requires an +explicit early-return-on-success, or that passes through when the hook returns `undefined`, +would silently start echoing Zod errors (the exact T-19-14 leak this guards against). It is +correct today but fragile and untested for the pass-through case. +**Fix:** Add a focused test asserting that a malformed body never includes `received`/the +submitted value for each hook site (localAuth has one; admin/me password routes should too), +and pin the `@hono/zod-validator` version. + +### WR-06: Rate-limit `lockedUntil` is refreshed on every blocked attempt, extending the window indefinitely + +**File:** `apps/api/src/routes/localAuth.ts:97-106, 133-137` +**Issue:** On a 429 the code sets `attempt.lockedUntil = Date.now() + RATE_WINDOW_SECS*1000` +again, so an attacker who keeps hitting the endpoint perpetually slides the cooldown forward — +a legitimate user behind the same IP can never get back in even after pausing, because every +attacker request re-arms the window. Coupled with CR-04 this makes the household-wide lock +effectively permanent under sustained traffic. +**Fix:** Do not extend `lockedUntil` on requests that are themselves rejected by the window; +only set it when transitioning from below-threshold to at-threshold. + +### WR-07: `parseInt` member/calendar id accepts trailing garbage + +**File:** `apps/api/src/routes/admin.ts:215-218, 307-311` +**Issue:** `parseInt(c.req.param('id'), 10)` returns `12` for `"12abc"` and the `isNaN` +guard passes. Not exploitable here (the value is used only in a parameterized `eq`), but it +silently accepts malformed ids and could mask client bugs. The `/members/:id/password` and +`/calendars/:id/shared` routes both use this pattern. +**Fix:** Validate with `Number.isInteger(Number(raw))` or a Zod param schema so `"12abc"` is +rejected with 400. + +## Info + +### IN-01: `localSession` `maxAge`/expiry parsing has no validation + +**File:** `apps/api/src/auth/localSession.ts:31` +**Issue:** `Number(process.env.LOCAL_SESSION_EXPIRES ?? 86400)` yields `NaN` for a malformed +value, producing a JWT with `exp = now + NaN` (→ `NaN`) and a cookie `maxAge: NaN`. Verify +behavior is then "always expired" or "never expires" depending on the JWT lib's NaN handling. +**Fix:** Coerce and validate: `const n = Number(env); SESSION_MAX_AGE = Number.isFinite(n) && n > 0 ? n : 86400;` + +### IN-02: Duplicated inline scrypt implementation across three locations + +**File:** `apps/api/scripts/reset-admin.ts:45-51`, `.gitea/workflows/ci.yml:307-311`, +`apps/api/src/auth/localCredentials.ts:42-53` +**Issue:** The PHC scrypt hash is copy-pasted in the reset-admin script, the CI seed step, +and the canonical module. If the parameters ever change (the file comment advertises +parameter evolution as a feature), these three drift and produce incompatible hashes. The +duplication is documented as necessary (cannot import compiled TS from a plain script), but +there is no test asserting the three stay in lockstep. +**Fix:** Add a test that imports `hashPassword` and asserts a known input round-trips against +a hash produced by the inlined parameters, so a parameter change fails CI loudly. + +### IN-03: `loginAttempts` map is unbounded (memory growth) + +**File:** `apps/api/src/routes/localAuth.ts:66` +**Issue:** Entries are only removed on a *successful* login for that IP. Spoofed/rotated +`X-Forwarded-For` values (see CR-04) accumulate map entries with no eviction, a slow memory +leak. Out of strict v1 perf scope, noted because it is reachable by unauthenticated input. +**Fix:** Add periodic eviction of entries whose `lockedUntil` is far in the past. + +### IN-04: `me.ts` link-oidc nonce is generated but never persisted/verified + +**File:** `apps/api/src/routes/me.ts:312-320` and `apps/api/src/index.ts:60-74` +**Issue:** The signed state carries a `nonce` "to prevent replay," but the `/callback` +handler never records or checks the nonce — it only verifies the JWT signature and reads +`linkUserId`. A captured state JWT is fully replayable within its 10-minute window (the +signature stays valid), so the nonce provides no actual replay protection. This underlies +the takeover concern in BL-03. +**Fix:** Persist issued nonces (or a single-use jti) and reject a state whose nonce was +already consumed, or shorten the window and bind the state to the initiating session cookie. + +--- + +_Reviewed: 2026-06-17_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: deep_