diff --git a/.planning/phases/10-admin-role-settings/10-REVIEW.md b/.planning/phases/10-admin-role-settings/10-REVIEW.md new file mode 100644 index 0000000..4d85290 --- /dev/null +++ b/.planning/phases/10-admin-role-settings/10-REVIEW.md @@ -0,0 +1,197 @@ +--- +phase: 10-admin-role-settings +reviewed: 2026-06-13T00:00:00Z +depth: standard +files_reviewed: 21 +files_reviewed_list: + - apps/api/src/auth/user.ts + - apps/api/src/broker/outboxWorker.ts + - apps/api/src/db/migrations/0001_famous_mad_thinker.sql + - apps/api/src/db/schema.ts + - apps/api/src/index.ts + - apps/api/src/lib/requireAdmin.ts + - apps/api/src/routes/admin.ts + - apps/api/src/routes/me.ts + - apps/api/tests/auth/user.test.ts + - apps/api/tests/lib/requireAdmin.test.ts + - apps/api/tests/routes/admin.test.ts + - apps/api/tests/routes/me.test.ts + - apps/pwa/e2e/admin.spec.ts + - apps/pwa/e2e/global-setup.ts + - apps/pwa/src/App.tsx + - apps/pwa/src/api/client.ts + - apps/pwa/src/components/AppNav.tsx + - apps/pwa/src/components/BottomTabBar.tsx + - apps/pwa/src/components/SetupBanner.tsx + - apps/pwa/src/routes/AdminPage.tsx +findings: + critical: 1 + warning: 7 + info: 5 + total: 13 +status: issues_found +--- + +# Phase 10: Code Review Report + +**Reviewed:** 2026-06-13 +**Depth:** standard +**Files Reviewed:** 21 +**Status:** issues_found + +## Summary + +Phase 10 adds admin-role primitives (`users.is_admin`, first-login-wins bootstrap), a DB-backed `requireAdmin` guard, an admin/self-service credential surface, and an exclusive shared-calendar designator. The core security contracts hold up well: `requireAdmin` reads `is_admin` from the DB (not the context user), all `/api/admin/*` routes are gated by `adminRouter.use('*', requireAdmin)` as the first statement, the no-echo hook is applied to both credential routes, and `/api/me/credential` resolves `currentUserId` from the session and ignores any body `userId`. The migration is additive (no DROP/TRUNCATE). + +The defects found are concentrated in two areas: (1) the first-login-wins admin bootstrap and shared-calendar designation are non-atomic multi-statement operations with no transaction or row-count guard, and (2) several routes/inputs lack existence/identity validation that lets the system silently enter a wrong state. The single BLOCKER is the shared-calendar PUT, which can leave the household with **zero** shared calendars while returning `{ ok: true }`. + +**Scope limitation:** Two in-scope files could not be read — `apps/api/src/broker/credentialSync.ts` (the central validate/encrypt/store helper) and `apps/pwa/src/components/CredentialSheet.tsx` (the credential input form) — both are in directories denied by the sandbox. Their behavior was reviewed indirectly via call sites (`admin.ts`, `me.ts`) and the integration tests (`admin.test.ts`), which confirm encryption-at-rest and no-echo on the wire. The crypto implementation itself (IV reuse, auth-tag handling, key derivation) and the sheet's client-side handling of the password (e.g. whether it is held in state longer than the request, autocomplete attributes) were **not** directly inspected and should be re-reviewed separately. + +## Critical Issues + +### CR-01: Shared-calendar PUT can clear the only shared calendar and report success + +**File:** `apps/api/src/routes/admin.ts:150-163` +**Issue:** `PUT /api/admin/calendars/:id/shared` runs two independent UPDATEs: + +```js +await db.update(calendars).set({ isShared: false }).where(eq(calendars.isShared, true)); // clear +await db.update(calendars).set({ isShared: true }).where(eq(calendars.id, targetId)); // set +``` + +`targetId` is only checked for `isNaN`, never for existence. If the id does not match any row (deleted calendar, stale client cache, hand-crafted request, off-by-one from a re-sync that re-keyed calendar ids), step 1 still clears the previously-shared calendar and step 2 updates **0 rows**. The handler then returns `{ ok: true }`. Result: the household silently ends up with **no** shared calendar — the shared family lane disappears for every member, and the UI's `Currently shared` indicator shows nothing, with no error surfaced. This is a data-state-loss / correctness defect in the core ADMIN-02 flow. The two statements are also non-transactional, so a crash between them leaves zero shared calendars even for a valid id. + +**Fix:** Validate the target exists and make the swap atomic. Check the affected-row count of the set, and roll back / 404 if it is zero: + +```js +adminRouter.put('/calendars/:id/shared', async (c) => { + const targetId = parseInt(c.req.param('id'), 10); + if (Number.isNaN(targetId)) { + return c.json({ error: 'Invalid calendar id' }, 400); + } + + // Confirm the target exists BEFORE clearing the current selection. + const [target] = await db + .select({ id: calendars.id }) + .from(calendars) + .where(eq(calendars.id, targetId)) + .limit(1); + if (!target) { + return c.json({ error: 'Calendar not found' }, 404); + } + + // Wrap both writes in a transaction so a crash cannot strand zero shared calendars. + await db.transaction(async (tx) => { + await tx.update(calendars).set({ isShared: false }).where(eq(calendars.isShared, true)); + await tx.update(calendars).set({ isShared: true }).where(eq(calendars.id, targetId)); + }); + + return c.json({ ok: true }, 200); +}); +``` + +## Warnings + +### WR-01: First-login-wins admin bootstrap is a non-atomic check-then-insert (TOCTOU) + +**File:** `apps/api/src/auth/user.ts:118-136` +**Issue:** The zero-admin `COUNT(*)` and the subsequent `INSERT ... isAdmin: shouldBeAdmin` are separate statements with no transaction or locking. Two genuinely-concurrent first logins (two different OIDC identities hitting `/api/me` at the same time on a cold DB) can both read `count === 0` and both insert with `isAdmin: true`, producing two admins instead of one. The comment claims first-login-wins, but the implementation does not enforce a single winner. For a two-person household this is low-probability, but it is a privilege-escalation-adjacent correctness gap in exactly the bootstrap the phase is meant to harden, and Phase 12 is documented to build on this hook. + +**Fix:** Perform the count and insert inside a single transaction with a row lock (e.g. `SELECT ... FOR UPDATE` on the users table or an advisory lock), or gate admin assignment on a `UNIQUE` partial constraint / `app_config` flag set atomically. Minimum viable fix: wrap steps 2-5 in `db.transaction` and re-read the admin count inside it with `FOR UPDATE`. + +### WR-02: `resolveUserId` / `/api/me` will upsert a user with empty-string iss or sub + +**File:** `apps/api/src/routes/me.ts:81-85` and `:115-123` +**Issue:** Both the `resolveUserId` helper and the main `/api/me` handler coalesce missing claims to empty strings: `const sub = auth.sub ?? ''` and `const iss = (auth.iss as string | undefined) ?? ''`. If a malformed/partial token ever reaches here with a missing `sub` (the OIDC middleware is mocked as a passthrough in tests, and real-world token edge cases exist), `upsertUser('', '', ...)` creates a bogus identity row keyed on `('', '')`. Because identity is the composite `(oidc_iss, oidc_sub)` unique key, the first such request claims that row and — if it is the first user — becomes the bootstrap **admin**. Subsequent empty-claim requests from any user would then resolve to that same row, conflating distinct sessions into one admin identity. + +**Fix:** Reject empty identity instead of inventing one: + +```js +const iss = typeof auth.iss === 'string' ? auth.iss : ''; +const sub = typeof auth.sub === 'string' ? auth.sub : ''; +if (!iss || !sub) { + return c.json({ error: 'Unauthorized' }, 401); +} +``` + +Apply the same guard in `resolveUserId` (return `null`). + +### WR-03: New `UNIQUE(user_id)` on a populated `member_credentials` table will fail the migration if duplicates exist + +**File:** `apps/api/src/db/migrations/0001_famous_mad_thinker.sql:11` +**Issue:** `ALTER TABLE member_credentials ADD CONSTRAINT uniq_member_credential_user UNIQUE(user_id)` is additive (good — no data destroyed), but if any user already has more than one credential row in a deployed environment, MariaDB rejects the `ALTER` with error 1062 and the entire migration fails partway. The earlier `ADD COLUMN` statements in the same file may have already committed (MariaDB DDL is non-transactional), leaving the schema in a half-applied state that is awkward to recover. Schema design intends one-credential-per-member, but nothing earlier in the project enforced it, so existing prod data may violate it. + +**Fix:** Before adding the constraint, de-duplicate. Either ship a pre-migration cleanup (`DELETE` keeping the newest row per `user_id`) or verify in the deploy runbook that no duplicates exist. At minimum, document the failure mode in the migration so an operator hitting 1062 knows to clean up and re-run, rather than assuming corruption. + +### WR-04: `GET /api/admin/members` exposes every member's id/displayName/color to any admin — no per-row credential value, but unbounded result set + +**File:** `apps/api/src/routes/admin.ts:73-92` +**Issue:** The query `leftJoin`s `member_credentials` and maps `hasCredential: row.credentialId !== null`. This is correct and does **not** leak the encrypted password (good). However: (a) there is no `limit`, so the endpoint returns the full users table — fine for two members, but the "N-member expansion intent" recorded in project memory means this should be paginated or at least bounded before it ships to a larger household; and (b) the `leftJoin` would emit duplicate member rows (and a misleading member count) if the new `UNIQUE(user_id)` constraint were ever absent or dropped — the correctness of `hasCredential` silently depends on that constraint holding. Defense-in-depth: either aggregate (`MAX(credentialId)` / `EXISTS`) or document the hard dependency. + +**Fix:** Use an existence subquery instead of a join so the result is one row per user regardless of credential cardinality: + +```js +const rows = await db.select({ + id: users.id, displayName: users.displayName, color: users.color, + hasCredential: sql`EXISTS (SELECT 1 FROM member_credentials mc WHERE mc.user_id = ${users.id})`, +}).from(users); +``` + +### WR-05: `/api/admin/calendars` returns `displayName` typed as non-null, but the column is nullable + +**File:** `apps/api/src/routes/admin.ts:130-140`, contract `apps/pwa/src/api/client.ts:364-368` +**Issue:** `calendars.displayName` is `varchar('display_name', { length: 256 })` — **nullable** (schema.ts:95). The admin endpoint selects it raw and the client type `AdminCalendar.displayName: string` (client.ts:366) declares it non-null. `CalendarRadioRow` renders `{calendar.displayName}` directly (AdminPage.tsx:492). A calendar synced without a `DISPLAYNAME` prop (possible from CalDAV) yields a radio row with an empty/blank label that the admin cannot distinguish from others, making the exclusive-select picker ambiguous. The type also lies, so downstream `.length`/string ops on it are unguarded. + +**Fix:** Type it as `string | null` in `AdminCalendar` and render a fallback (e.g. the calendar URL tail or "Untitled calendar") in `CalendarRadioRow`. + +### WR-06: Admin nav/route gating depends on a client-mutable `isAdmin` with no server re-check on the data routes' shape + +**File:** `apps/pwa/src/App.tsx:75,141-152`, `apps/pwa/src/routes/AdminPage.tsx` +**Issue:** This is correctly documented as "UX only" and the server enforces 403 on `/api/admin/*` — that boundary is sound. The warning is narrower: the `/admin` route element renders `meQuery.isLoading ?
: isAdmin ? : `. `retry: false` plus an error state (`meQuery.isError`, not `isLoading`) makes `isAdmin` fall to `false` and redirect — acceptable. But on a **stale** cached `['me']` (staleTime 5min) where the admin was demoted server-side, the PWA keeps showing the Admin surface and firing admin queries until the cache refreshes; those queries 403 and surface as "Could not load members." This is a confusing-but-safe degradation, worth noting because the AdminPage has no explicit handling that distinguishes a 403 (you are no longer admin) from a transient error. + +**Fix:** In `AdminPage`, treat a 403 from `fetchAdminMembers`/`fetchAdminCalendars` as an authority revocation — invalidate `['me']` and redirect to `/calendar` rather than rendering the generic error. + +### WR-07: `triggerTargetedResync` client cache holds decrypted Fastmail credentials in a Map for the whole drain cycle + +**File:** `apps/api/src/broker/outboxWorker.ts:687-689, 302-313` +**Issue:** IN-01's per-cycle `clientCache: Map` was added to decrypt each member's app password at most once per drain. The tradeoff: a decrypted-credential-bearing client object now lives for the duration of the entire drain loop (up to 10 rows plus bounded 10s re-syncs each), and the Map is captured by the closures passed to `syncCalendar`. The code comment frames this as a security improvement, but it also widens the lifetime of the decrypted secret in memory versus decrypt-per-row. Not a leak per se (the Map is local and GC'd at function return), but it is the opposite of the stated T-03-13 "narrow the window" goal and deserves an explicit note that the cache must never be hoisted to module scope. + +**Fix:** Acceptable as-is for the single-process two-user deployment, but add an assertion/comment that `clientCache` is function-local and consider clearing it (`clientCache.clear()`) in a `finally` so the references drop before the function's lexical scope is collected. Re-review once `credentialSync.ts`/`crypto.ts` are inspectable to confirm the `FastmailClient` does not retain the plaintext password as a field. + +## Info + +### IN-01: Two in-scope files were not reviewable (sandbox denial) + +**File:** `apps/api/src/broker/credentialSync.ts`, `apps/pwa/src/components/CredentialSheet.tsx` +**Issue:** Both are in directories denied by the review sandbox and could not be read. `credentialSync.ts` is the single most security-relevant file in the phase (it owns encrypt + validate + store of the Fastmail app password). Its contract was inferred from call sites and the green integration tests (encryption-at-rest and no-echo verified on the wire), but the crypto internals were not audited. +**Fix:** Re-run this review with read access to `apps/api/src/broker/` and `apps/pwa/src/components/`, or have a reviewer with access audit AES-GCM IV uniqueness, auth-tag verification on decrypt, key sourcing from `APP_PASSWORD_ENCRYPTION_KEY`, and the sheet's password-state lifetime / `autoComplete="off"`. + +### IN-02: `noEchoHook` / `meNoEchoHook` are byte-identical duplicates + +**File:** `apps/api/src/routes/admin.ts:60-64`, `apps/api/src/routes/me.ts:164-168` +**Issue:** The two no-echo Zod hooks are identical (`{ error: 'Invalid request' }` 400). Duplicating the security-critical no-echo contract in two files risks the two copies drifting (one gets "improved" to include details). The project convention is to duplicate auth helpers per-router, so this is allowed, but a shared `noEchoHook` constant would make the no-echo guarantee single-sourced. +**Fix:** Optional — extract to a shared `lib/noEchoHook.ts` so the T-10-09 contract has one definition. + +### IN-03: `resolveAdminAndSetupStatus` issues two sequential round-trips per `/api/me` + +**File:** `apps/api/src/routes/me.ts:50-67` +**Issue:** Each `/api/me` does an `isAdmin` select then a `memberCredentials` existence select, serially. Functionally correct; minor. (Performance is out of v1 scope — noted only as a code-quality observation, not flagged as a perf defect.) +**Fix:** Could be a single join, but not required. + +### IN-04: `parseInt` without explicit radix appears once; the shared-cal route correctly passes radix 10 + +**File:** `apps/api/src/routes/admin.ts:151` +**Issue:** `parseInt(c.req.param('id'), 10)` correctly passes the radix — good. Noting for completeness that this is the only numeric parse in the admin surface and it is done correctly; no leading-zero/octal hazard. +**Fix:** None. + +### IN-05: AdminPage error copy collapses all mutation failures to "Something went wrong" + +**File:** `apps/pwa/src/routes/AdminPage.tsx:274-285` +**Issue:** `sharedCalMutation.isError` renders a generic message. Combined with CR-01 (the server can return `{ ok: true }` even when it set nothing), the user has no signal that a save no-op'd. Once CR-01 is fixed to return 404, this generic toast will at least fire on the not-found path, but a specific "That calendar no longer exists — refresh" message would be clearer. +**Fix:** Distinguish 404 from transient errors in the mutation's `onError`. + +--- + +_Reviewed: 2026-06-13_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_