190 lines
10 KiB
Markdown
190 lines
10 KiB
Markdown
---
|
||
phase: 19-local-auth-no-oidc-mode
|
||
reviewed: 2026-06-17T00:00:00Z
|
||
depth: deep
|
||
files_reviewed: 41
|
||
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: 0
|
||
blocker: 0
|
||
warning: 0
|
||
info: 2
|
||
total: 2
|
||
status: clean
|
||
---
|
||
|
||
# Phase 19: Code Review Report (Iteration-2 Re-Review)
|
||
|
||
**Reviewed:** 2026-06-17
|
||
**Depth:** deep
|
||
**Files Reviewed:** 41
|
||
**Status:** clean
|
||
|
||
## Summary
|
||
|
||
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.
|
||
|
||
**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.
|
||
|
||
### Confirmation of high-judgment fixes (verified by tracing, not just diff)
|
||
|
||
- **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.
|
||
|
||
### Cross-cutting checks
|
||
|
||
- 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: `fetchLinkOidc` declared return type is narrower than the cast it returns
|
||
|
||
**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: 429 rate-limit branch short-circuits before the dummy-hash work
|
||
|
||
**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.
|
||
|
||
---
|
||
|
||
_Reviewed: 2026-06-17_
|
||
_Reviewer: Claude (gsd-code-reviewer)_
|
||
_Depth: deep_
|