familysync/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW.iter2.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
19-local-auth-no-oidc-mode	2026-06-17T00:00:00Z	deep	39	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	critical blocker warning info total 4 4 7 4 15	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:

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:

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:

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:

// 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:

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