.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','<n>')`. 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_