Lucas BergerandClaude Opus 4.8
c7955a46b9
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
198 lines
16 KiB
Markdown
198 lines
16 KiB
Markdown
---
|
|
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<boolean>`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 ? <div/> : isAdmin ? <AdminPage/> : <Navigate/>`. `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<number, FastmailClient>` 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_
|