---
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_