2026-06-18 06:25:00 -04:00
1 changed files with 125 additions and 0 deletions
@@ -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_