From 73dd6a238396e78969227e1f6b813e3b8392f4b6 Mon Sep 17 00:00:00 2001
From: Lucas Berger <me@lucasberger.ca>
Date: Wed, 17 Jun 2026 20:50:25 -0400
Subject: [PATCH] =?UTF-8?q?docs(19):=20re-review=20after=20fixes=20?=
 =?UTF-8?q?=E2=80=94=20status=20clean=20(0=20critical/warning,=202=20info)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../19-REVIEW-FIX.iter2.md                    | 125 +++++
 .../19-REVIEW.iter2.md                        | 387 ++++++++++++++++
 .../19-local-auth-no-oidc-mode/19-REVIEW.md   | 434 +++++-------------
 3 files changed, 630 insertions(+), 316 deletions(-)
 create mode 100644 .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.iter2.md
 create mode 100644 .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.iter2.md

diff --git a/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.iter2.md b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.iter2.md
new file mode 100644
index 0000000..9314718
--- /dev/null
+++ b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.iter2.md
@@ -0,0 +1,125 @@
+---
+phase: 19-local-auth-no-oidc-mode
+fixed_at: 2026-06-17T20:39:00Z
+review_path: .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md
+iteration: 1
+findings_in_scope: 15
+fixed: 15
+skipped: 0
+status: all_fixed
+---
+
+# Phase 19: Code Review Fix Report
+
+**Fixed at:** 2026-06-17T20:39:00Z
+**Source review:** .planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md
+**Iteration:** 1
+
+**Summary:**
+- Findings in scope: 15 (4 critical, 4 blocker, 7 warning, 4 info — fix_scope: all)
+- Fixed: 15
+- Skipped: 0
+
+**Verification:** Full API suite **452/452** (34 files, live MariaDB) and full PWA suite **266/266** (22 files) pass; both `tsc --noEmit` clean. Findings classified as security/availability logic (CR-04, BL-03, WR-06, IN-04) are flagged "requires human verification" below — syntax/tests pass but a human should confirm the threat-model intent.
+
+## Fixed Issues
+
+### CR-01: OIDC-link flow broken — client/server response-shape mismatch
+**Files modified:** `apps/pwa/src/api/client.ts`, `apps/pwa/src/components/SettingsSheet.tsx`
+**Commit:** 1688f22
+**Applied fix:** Changed `fetchLinkOidc` to return `{ authorizationUrl: string | null }` matching the server's `{ signedState, authorizationUrl }` contract, and updated `LinkOidcSheet` to navigate to `authorizationUrl` (handling the `null`/unconfigured case by surfacing an error instead of navigating to `undefined`).
+
+### CR-02: Admin "create member" always fails — request field-name mismatch
+**Files modified:** `apps/pwa/src/api/client.ts`
+**Commit:** 93c47b3
+**Applied fix:** `fetchCreateMember` now sends `initialPassword` (the field `createMemberSchema` requires) instead of `password`, and maps HTTP 409 to `Error('conflict')` so the AdminPage's existing conflict branch renders the right banner.
+
+### CR-03: Self-service password change logs user out on wrong current password
+**Files modified:** `apps/api/src/routes/me.ts`, `apps/pwa/src/api/client.ts`, `apps/api/tests/routes/me.test.ts`
+**Commit:** 6ef8e03
+**Applied fix:** Server returns **403** (not 401) for an incorrect current password; client `fetchChangePassword` branches on 403 → `Error('wrong-current')` before the 401→`SessionExpiredError` path, so a mistyped password no longer triggers the global session-expiry logout. Updated me.test Test 2 to expect 403.
+
+### CR-04: Login lockout per-IP, global, permanent, unrecoverable — *requires human verification*
+**Files modified:** `apps/api/src/routes/localAuth.ts`, `apps/api/src/routes/admin.ts`, `apps/pwa/src/routes/LoginPage.tsx`, `apps/api/tests/routes/localAuth.test.ts`
+**Commit:** b083cb7
+**Applied fix:** Re-scoped the rate limiter from client IP to the **validated username**; added a **15-minute TTL** so a 423 lockout auto-expires (self-healing, no restart); wired admin password-reset to call `resetLoginAttempts(username)` for an immediate unlock; corrected the LoginPage banner copy. Added Test 5b asserting TTL auto-expiry. *Human verification: confirm the username-scoping + TTL behaviour matches the intended threat model for the tunnel deployment.*
+
+### BL-01: devSessionCookieMiddleware issues a session without verifying secret strength
+**Files modified:** `apps/api/src/auth/devBypass.ts`
+**Commit:** 3674b25
+**Applied fix:** Apply the same `secret.length >= 32` floor used by the boot guard inside `devSessionCookieMiddleware` before minting the dev cookie (degrade to no-op if too short), and emit a loud warning when the secret is the well-known dev placeholder.
+
+### BL-02: Logout cannot clear the cookie in non-production (Secure attribute mismatch)
+**Files modified:** `apps/api/src/auth/localSession.ts`
+**Commit:** cd095e5
+**Applied fix:** `clearLocalSessionCookie` now mirrors the issue-time `secure: process.env.NODE_ENV === 'production'` logic instead of hard-coding `secure: true`, so the deletion cookie is accepted over plain HTTP and logout actually clears the session in HTTP-only self-hosts.
+
+### BL-03: OIDC-link binding swallows failure / binds on stale/blank identity — *requires human verification*
+**Files modified:** `apps/api/src/index.ts`
+**Commit:** 7153760
+**Applied fix:** In the `/callback` link path, reject the bind unless the current local session (`verifyLocalSessionCookie`) matches `linkUserId` (account-takeover guard), and reject when `iss`/`sub` are empty (never call `linkOidcToUser` with blank identity, which would corrupt identity and delete the user's local credential). *Human verification: confirm the session cross-check closes the replay-takeover path described in the review.*
+
+### BL-04: localAuthMiddleware fabricates oidcSub collisions for local users
+**Files modified:** `apps/api/src/auth/devBypass.ts`, `apps/api/src/auth/localAuthMiddleware.ts`, `apps/api/tests/auth/localAuthMiddleware.test.ts`
+**Commit:** 40666e1
+**Applied fix:** Introduced a `ContextUser` interface with nullable `oidcIss`/`oidcSub`; the middleware now stores `null` for local users instead of the `'local'`/`String(id)` sentinels that shared the `uniq_oidc_identity` uniqueness domain. Added Test 1c asserting null context for a null-OIDC local user.
+
+### WR-01: reset-admin.ts arg parsing trusts `--password ''` and echoes username
+**Files modified:** `apps/api/scripts/reset-admin.ts`
+**Commit:** c4d8d76
+**Applied fix:** Rewrote `parseArgs` to support `--key=value` and to treat `--username`/`--password` as value-taking (consuming the next token verbatim, so a `--`-prefixed or empty password is preserved) and `--dry-run` as boolean; removed username interpolation from log lines.
+
+### WR-02 + WR-04: hardcoded Authelia auth path / OIDC-config detection divergence
+**Files modified:** `apps/api/src/auth/oidcConfig.ts` (new), `apps/api/src/routes/me.ts`
+**Commit:** 322929a
+**Applied fix:** New `oidcConfig.ts` centralizes the env-OR-app_config resolution (`resolveOidcConfig`) and discovers the `authorization_endpoint` from the provider's `/.well-known/openid-configuration` (`discoverAuthorizationEndpoint`). me.ts link-oidc now uses both, so a wizard-configured instance no longer reports `oidcEnabled:true` while returning `authorizationUrl:null`, and the URL is no longer Authelia-path-specific. (Two findings fixed in one commit — they share the same handler/lines and are inseparable.)
+
+### WR-03: scryptSync blocks the event loop on the login hot path
+**Files modified:** `apps/api/src/auth/localCredentials.ts`, `apps/api/src/routes/localAuth.ts`, `apps/api/src/routes/me.ts`, `apps/api/src/routes/admin.ts`, plus their tests
+**Commit:** 30ad25c
+**Applied fix:** Converted `hashPassword`/`verifyPassword` to async (threadpool scrypt via a typed Promise wrapper), awaited at all call sites, made the login DUMMY_HASH a module-level promise, and moved create-member hashing outside the DB transaction. Updated all test call sites to await. Preserves the timing-defense property while keeping the event loop responsive.
+
+### WR-05: noEchoHook pass-through is correct-but-untested
+**Files modified:** `apps/api/tests/routes/admin.test.ts`, `apps/api/tests/routes/me.test.ts`
+**Commit:** 4bd6b2c
+**Applied fix:** Added focused no-echo tests for the admin create-member and me password hook sites asserting a malformed body never includes the submitted password or Zod's `received`/`issues`. `@hono/zod-validator` is already pinned to exact `0.8.0` in package.json.
+
+### WR-06: rate-limit lockedUntil refreshed on every blocked attempt — *requires human verification*
+**Files modified:** `apps/api/src/routes/localAuth.ts`
+**Commit:** 4cf2ad4
+**Applied fix:** The 429 (already-rejected) branch no longer re-arms `lockedUntil`; the cooldown window stays anchored to when it was first armed, so sustained attacker traffic can no longer slide the window forward indefinitely. *Human verification: confirm the window now expires on schedule for a legitimate user behind the same identity.*
+
+### WR-07: parseInt member/calendar id accepts trailing garbage
+**Files modified:** `apps/api/src/routes/admin.ts`, `apps/api/tests/routes/admin.test.ts`
+**Commit:** 32bdd1e
+**Applied fix:** Added `parsePositiveIntParam` using `Number.isInteger(Number(raw))` and applied it to `/members/:id/password` and `/calendars/:id/shared`, so `"12abc"` is now rejected with 400. Added a test for the calendar route.
+
+### IN-01: localSession maxAge/expiry parsing has no validation
+**Files modified:** `apps/api/src/auth/localSession.ts`
+**Commit:** f2fc140
+**Applied fix:** Coerce and validate `LOCAL_SESSION_EXPIRES` — fall back to 86400s for any non-finite or non-positive value, preventing a `NaN` exp/maxAge.
+
+### IN-02: duplicated inline scrypt implementation across three locations
+**Files modified:** `apps/api/tests/auth/localCredentials.test.ts`
+**Commit:** e392bf2
+**Applied fix:** Added a lockstep test that builds a hash using the inlined scrypt parameters (N=16384, r=8, p=1, KEY_LEN=32 — matching reset-admin.ts and ci.yml) and asserts it round-trips against the canonical `verifyPassword`, so a parameter drift fails CI loudly.
+
+### IN-03: loginAttempts map is unbounded
+**Files modified:** `apps/api/src/routes/localAuth.ts`
+**Commit:** f02521d
+**Applied fix:** Added `evictStaleLoginAttempts`, called opportunistically per login request, dropping entries that are neither in an active rate-limit window nor an active lockout TTL — bounding the map under input churn without weakening the limiter.
+
+### IN-04: link-oidc nonce generated but never persisted/verified — *requires human verification*
+**Files modified:** `apps/api/src/auth/linkNonceStore.ts` (new), `apps/api/src/routes/me.ts`, `apps/api/src/index.ts`
+**Commit:** 2691dd0
+**Applied fix:** New in-memory single-use nonce store: me.ts registers the issued nonce (valid until the state JWT's exp); the `/callback` link path consumes it and rejects any replayed/unknown/expired nonce before binding. Combined with BL-03's session cross-check, the captured-state replay window is closed. *Human verification: confirm single-use semantics are sufficient for the single-process deployment (move to Redis if multi-process).*
+
+## Skipped Issues
+
+None — all 15 in-scope findings were fixed.
+
+---
+
+_Fixed: 2026-06-17T20:39:00Z_
+_Fixer: Claude (gsd-code-fixer)_
+_Iteration: 1_
diff --git a/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.iter2.md b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.iter2.md
new file mode 100644
index 0000000..eb38370
--- /dev/null
+++ b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.iter2.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_
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
index eb38370..1f4827f 100644
--- a/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md
+++ b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.md
@@ -2,7 +2,7 @@
 phase: 19-local-auth-no-oidc-mode
 reviewed: 2026-06-17T00:00:00Z
 depth: deep
-files_reviewed: 39
+files_reviewed: 41
 files_reviewed_list:
   - apps/api/scripts/reset-admin.ts
   - apps/api/src/auth/devBypass.ts
@@ -45,340 +45,142 @@ files_reviewed_list:
   - .gitea/workflows/ci.yml
   - scripts/generate-secrets.mjs
 findings:
-  critical: 4
-  blocker: 4
-  warning: 7
-  info: 4
-  total: 15
-status: issues_found
+  critical: 0
+  blocker: 0
+  warning: 0
+  info: 2
+  total: 2
+status: clean
 ---
 
-# Phase 19: Code Review Report
+# Phase 19: Code Review Report (Iteration-2 Re-Review)
 
 **Reviewed:** 2026-06-17
 **Depth:** deep
-**Files Reviewed:** 39 (auth source + routes + PWA + CI)
-**Status:** issues_found
+**Files Reviewed:** 41
+**Status:** clean
 
 ## 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.
+This is the iteration-2 re-review confirming the fixer correctly applied all 15
+findings from the prior review (4 critical, 4 blocker, 7 warning, 4 info). I read
+every listed source file, traced the high-judgment fixes through their full call
+chains across module boundaries, ran `tsc --noEmit` on both `@familysync/api` and
+`@familysync/pwa` (both clean, exit 0), and confirmed the CI seed parameters and
+inlined scrypt copies all agree. The runtime test suite could not execute in this
+sandbox (global-setup requires a live MariaDB with root grants — `ER_ACCESS_DENIED`),
+so test verification is static: the relevant assertions were read directly and the
+production source typechecks against them.
 
-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.
+**Verdict: all 15 prior findings are correctly and completely resolved. No
+regressions, no re-occurrence at other call sites, and no new critical/blocker/warning
+issues.** Two low-severity Info observations are recorded below; neither blocks ship.
 
-## Critical Issues
+### Confirmation of high-judgment fixes (verified by tracing, not just diff)
 
-### CR-01: OIDC-link flow is broken — client/server response-shape mismatch
+- **CR-01/02/03 (client↔server contracts):**
+  - CR-01 shared-calendar: `admin.ts` `PUT /calendars/:id/shared` now does an
+    existence check inside a transaction (404 on missing id), so a stale id can no
+    longer silently clear the shared lane. Client `setSharedCalendar` agrees.
+  - CR-02 create-member: client `fetchCreateMember` maps `password → initialPassword`
+    (client.ts:199-204) and the server `createMemberSchema` requires `initialPassword`
+    (admin.ts:140); 409 maps to the `'conflict'` sentinel the AdminPage `onError`
+    expects (AdminPage.tsx:229). Server returns 409 on `ER_DUP_ENTRY` (admin.ts:202).
+    Both ends agree; admin.test.ts Tests 1–2 cover the round-trip + 409 rollback.
+  - CR-03 wrong-current-password: server returns **403** (`me.ts:267`), client checks
+    403 **before** the 401 session-expiry branch (client.ts:166) and maps it to
+    `'wrong-current'`, which `ChangePasswordSheet.onError` surfaces without dropping
+    the session (SettingsSheet.tsx:569). me.test.ts Test 2 asserts 403 + update-not-called.
+- **CR-04 / WR-06 / IN-03 (login limiter):** the limiter key is the validated,
+  trimmed username (localAuth.ts:138) — no `x-forwarded-for`/IP residue remains
+  anywhere in `routes/localAuth.ts` or `src/auth/*` (grep clean). 423 lockout
+  auto-expires after `LOCKOUT_TTL_MS` (15 min, localAuth.ts:149-155); admin reset
+  calls `resetLoginAttempts(credRow.username)` for an instant unlock (admin.ts:262).
+  WR-06: the 429 branch deliberately does **not** re-arm `lockedUntil`
+  (localAuth.ts:164-169), so a rejected attempt can no longer slide the window
+  forward; the window is only re-anchored by a genuine failure in the failure path.
+  IN-03: `evictStaleLoginAttempts` only drops entries that are both window-expired
+  and lockout-TTL-expired (localAuth.ts:113-121) — behaviourally identical to natural
+  expiry, so eviction never weakens the brute-force defense. Tests 4, 5, 5b cover it.
+- **BL-01 (dev-bypass secret floor):** `devSessionCookieMiddleware` applies the same
+  `>= 32` length floor before minting a real DEV_USER session JWT (devBypass.ts:144-151)
+  and warns on the well-known placeholder. The CI placeholder
+  `dev-secret-change-me-0000000000000000` is 36 chars, so it passes the floor and only
+  triggers the warning — intended.
+- **BL-02 (logout cookie Secure match):** `clearLocalSessionCookie` now mirrors the
+  issue-time `secure: NODE_ENV==='production'` (localSession.ts:114), so the
+  delete-cookie is accepted over plain HTTP and the user is actually logged out on
+  non-HTTPS deployments. `sameSite`/`path`/`httpOnly` also match issue-time.
+- **BL-03 (OIDC-link takeover guard):** `/callback` (index.ts:84-123) now enforces
+  three gates before binding: (1) single-use nonce via `consumeLinkNonce`; (2) the
+  initiating local session must still match `linkUserId`
+  (`verifyLocalSessionCookie(c) === linkUserId`); (3) `iss`/`sub` from `getAuth` must
+  be non-empty. `/callback` is registered outside `/api/*` so `localAuthMiddleware`
+  does not run, but the `local-session` cookie (path `/`) is still present and read
+  directly — the cross-check is effective. All three gates fail-closed to
+  `/?error=oidc-link-conflict`. The preflight conflict check in `linkOidcToUser`
+  (linkOidc.ts:65-74) remains the backstop.
+- **BL-04 (no fabricated identity sentinels):** `localAuthMiddleware` passes through
+  the DB `oidcIss`/`oidcSub` as `?? null` (localAuthMiddleware.ts:91-97); `ContextUser`
+  widens both to `string | null` (devBypass.ts:56-62). No `'local'`/`String(id)`
+  sentinels are written, so a local user cannot collide in the `uniq_oidc_identity`
+  domain. localAuthMiddleware.test.ts Test 1c pins null.
+- **WR-03 (async scrypt):** `hashPassword`/`verifyPassword` are async over the libuv
+  threadpool (localCredentials.ts:33-45, 65, 101) at every call site — login
+  (dummy-hash promise awaited, localAuth.ts:128/199), create-member (hash before the
+  transaction, admin.ts:159), admin reset, and self-change. The always-run dummy-hash
+  path preserves the timing-oracle defense (localAuth.ts:197-199). reset-admin.ts
+  legitimately keeps `scryptSync` (standalone CLI, no event loop to starve).
+- **IN-04 (single-use nonce):** `linkNonceStore.ts` records the nonce at issue
+  (me.ts:333) and `consumeLinkNonce` returns true exactly once per unexpired nonce,
+  with opportunistic sweep keeping the map bounded; `/callback` consumes before binding.
 
-**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;
-```
+### Cross-cutting checks
 
-### 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.
+- Inlined PHC scrypt parameters agree across all three copies: canonical module
+  (N=16384,r=8,p=1,keylen=32), `reset-admin.ts`, and `.gitea/workflows/ci.yml`
+  (seed step lines 309-310). localCredentials.test.ts Test 6 pins this round-trip.
+- `oidcConfig.ts` (`resolveOidcConfig` + `discoverAuthorizationEndpoint`) is the single
+  env-OR-app_config source now shared by `/api/auth/mode`, the fallback middleware, and
+  `me.ts` link-oidc, closing the WR-04 divergence where link-oidc could return
+  `authorizationUrl:null` while `/mode` reported `oidcEnabled:true`.
+- `LOCAL_SESSION_EXPIRES` NaN-coercion guard (localSession.ts:35-38) and boot guards
+  (`assertLocalSessionSecretSet` >= 32, exempt under bypass) are correct and wired
+  first in the `isMainModule()` block (index.ts:262-265).
+- Migration `0003_warm_deathstrike.sql` matches the `localCredentials` Drizzle schema
+  (unique on user_id and username, FK cascade, varchar(256) hash).
 
 ## Info
 
-### IN-01: `localSession` `maxAge`/expiry parsing has no validation
+### IN-01: `fetchLinkOidc` declared return type is narrower than the cast it returns
 
-**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;`
+**File:** `apps/pwa/src/api/client.ts:250,261`
+**Issue:** The function signature declares `Promise<{ authorizationUrl: string | null }>`
+but the body returns `res.json() as Promise<{ signedState: string; authorizationUrl: string | null }>`.
+The widening cast is harmless (the only consumer, `SettingsSheet.tsx` `LinkOidcSheet`,
+reads `data.authorizationUrl` only and never `signedState`), and `tsc` is clean. It is a
+minor contract-doc inconsistency: the declared type drops a field the server actually
+sends. Not a defect — recorded only so the next editor does not "fix" the cast and
+accidentally start relying on the absent field.
+**Fix:** Align the declared return type with the cast for clarity:
+```ts
+export async function fetchLinkOidc(): Promise<{ signedState: string; authorizationUrl: string | null }> {
+```
 
-### IN-02: Duplicated inline scrypt implementation across three locations
+### IN-02: 429 rate-limit branch short-circuits before the dummy-hash work
 
-**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.
+**File:** `apps/api/src/routes/localAuth.ts:162-177`
+**Issue:** Once an identity is in the 429 window, the handler returns before the DB
+lookup and the always-run `verifyPassword`/dummy-hash. This is a deliberate and correct
+DoS/throughput tradeoff (a rate-limited identity should not pay scrypt cost), and it does
+NOT leak username existence because the 429 path is reached identically for valid and
+invalid usernames (the limiter is keyed on the submitted username regardless of whether a
+credential row exists). The timing-oracle defense is only required on the *credential-check*
+path, which still always runs the dummy hash. Recorded for completeness; no change needed.
+**Fix:** None required. If a future reviewer wants strict constant-time even under
+rate-limiting, the dummy-hash could be awaited before the 429 return — but that would
+re-introduce the exact event-loop-starvation cost WR-03 removed, so leaving it as-is is
+the right call.
 
 ---