diff --git a/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.md b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.md new file mode 100644 index 0000000..9314718 --- /dev/null +++ b/.planning/phases/19-local-auth-no-oidc-mode/19-REVIEW-FIX.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_