diff --git a/.planning/phases/04-shared-lists-live-sync/04-REVIEW.md b/.planning/phases/04-shared-lists-live-sync/04-REVIEW.md new file mode 100644 index 0000000..db01f7b --- /dev/null +++ b/.planning/phases/04-shared-lists-live-sync/04-REVIEW.md @@ -0,0 +1,439 @@ +--- +phase: 04-shared-lists-live-sync +reviewed: 2026-06-09T15:30:00Z +depth: standard +files_reviewed: 33 +files_reviewed_list: + - apps/api/src/db/schema.ts + - apps/api/src/index.ts + - apps/api/src/lib/listAccess.ts + - apps/api/src/lib/listEmitter.ts + - apps/api/src/lib/rank.ts + - apps/api/src/routes/lists.ts + - apps/api/src/routes/sse.ts + - apps/api/test/setup.ts + - apps/api/tests/lib/listAccess.test.ts + - apps/api/tests/lib/listEmitter.test.ts + - apps/api/tests/lib/rank.test.ts + - apps/api/tests/routes/lists.test.ts + - apps/api/vitest.config.ts + - apps/pwa/src/App.tsx + - apps/pwa/src/api/listsClient.ts + - apps/pwa/src/components/AddItemInput.tsx + - apps/pwa/src/components/AppNav.tsx + - apps/pwa/src/components/BottomTabBar.tsx + - apps/pwa/src/components/CalendarShell.test.tsx + - apps/pwa/src/components/CreateListSheet.tsx + - apps/pwa/src/components/ItemRow.tsx + - apps/pwa/src/components/ListCard.tsx + - apps/pwa/src/components/ListDeleteDialog.tsx + - apps/pwa/src/components/ListsEmptyState.tsx + - apps/pwa/src/components/LiveSyncIndicator.tsx + - apps/pwa/src/hooks/useListSSE.test.ts + - apps/pwa/src/hooks/useListSSE.ts + - apps/pwa/src/routes/ListDetail.test.tsx + - apps/pwa/src/routes/ListDetail.tsx + - apps/pwa/src/routes/ListsIndex.tsx + - apps/pwa/src/store/listsStore.ts + - apps/pwa/package.json + - apps/api/package.json +findings: + critical: 3 + warning: 5 + info: 3 + total: 11 +status: issues_found +--- + +# Phase 4: Code Review Report + +**Reviewed:** 2026-06-09T15:30:00Z +**Depth:** standard +**Files Reviewed:** 33 +**Status:** issues_found + +## Summary + +Reviewed the full Phase 4 shared-lists + live-sync implementation: API routes, schema, +access-control helpers, SSE fan-out, fractional rank, and the React PWA layer (mutations, +SSE hook, drag-to-reorder, components). The previously-recorded rank collation bug +(uppercase fractional-indexing keys sort incorrectly under utf8mb4_uca1400_ai_ci) is +acknowledged but not re-litigated here per brief instructions. + +Three critical issues were found: a privilege-escalation hole that lets any list sharee +unilaterally de-share or re-share a list (purging or creating list_shares rows for ALL +household members), an SSE subscription scope that is computed once at connect time and +never refreshed (so a newly-shared list never reaches a live subscriber without a +disconnect/reconnect), and a missing NaN-guard on URL path parameters that causes DB +queries to execute with a filter of `id = NaN` instead of returning 400. + +--- + +## Critical Issues + +### CR-01: Sharee can de-share or re-share a list — privilege escalation on `isShared` toggle + +**File:** `apps/api/src/routes/lists.ts:319-393` + +**Issue:** `PATCH /api/lists/:id` gates on `checkListAccess` (owner OR sharee) but does +not restrict the `isShared` field to the owner. A sharee — any household member who was +granted access — can send `{ isShared: false }` and the handler will: + +1. Write `is_shared = false` to the `lists` row (changing the list's visibility state on + behalf of the owner without consent). +2. Delete ALL rows from `list_shares` for that list (line 366-368), immediately revoking + every other member's access including the owner's own sharee visibility. + +The inverse (a sharee escalating a private list to shared by sending `{ isShared: true }`) +is also possible, inserting `list_shares` rows for every user in the DB without the owner's +consent. + +The comment on line 344 reads "Reconcile list_shares on visibility change (owner only +affects shares)" but there is no `isOwner` guard anywhere in the PATCH handler — the +reconciliation runs unconditionally for any `allowed` user. + +The test at `lists.test.ts:438-450` explicitly tests and asserts that a sharee CAN rename +a list, which is correct, but there is no test asserting that a sharee CANNOT toggle +`isShared`. The gap is uncovered. + +**Fix:** Add an owner-only guard before the `isShared` reconciliation block (and before +writing `isShared` itself, since the DB field controls visibility semantics): + +```typescript +// In PATCH /:id, after the access check at line 327-332: +if (patch.isShared !== undefined && !access.isOwner) { + return c.json({ error: 'Only the list owner can change sharing settings' }, 403) +} +``` + +--- + +### CR-02: SSE subscription scope is stale — newly shared lists never delivered to live subscribers + +**File:** `apps/api/src/routes/sse.ts:85-121` + +**Issue:** `GET /api/sse/lists` calls `getAccessibleListIds(userId)` exactly once at +connection time (line 89), then subscribes only to those list channels. If the user's +access set changes while the SSE connection is open — for example, another member creates +a new shared list (which inserts a `list_shares` row for this user), or a PATCH toggles +`isShared` — the live subscriber never receives `list:updated` events for the new list +because no subscription was registered for its channel. + +From the client's perspective: member A creates "Groceries" (shared). The `publishListEvent` +fires on channel `list:${newId}`. Member B's open SSE stream has no subscriber on that +channel — it was computed before the list existed. B only learns about the list when the +30-second polling fallback fires (D-12). + +This means the "other member sees the change appear without refreshing" requirement (Truth +3) is not met for newly-created shared lists while both members are simultaneously connected. +The 30-second polling fallback (D-12) masks the failure but does not eliminate it. + +**Fix (two options):** + +Option A (minimal): When `POST /api/lists` creates a shared list, publish a special +`list:created` event to a well-known global channel (e.g. `global:lists`) that all +authenticated SSE connections also subscribe to. On receiving `list:created`, the client +invalidates `['lists']` and re-establishes (or the server issues a reconnect hint). + +Option B (structural, recommended): Store the SSE handler's `userId` and wire the +`list:created` event through a per-user "inbox" channel (`user:${userId}`) that the SSE +endpoint subscribes to in addition to the per-list channels. `POST /api/lists` fans out +to each sharee's inbox. The SSE handler then dynamically adds a new per-list subscription +when it receives the inbox event. + +At minimum, `POST /api/lists`, `PATCH /api/lists/:id` (when toggling `isShared`), and +the `list:deleted` flow all need to trigger re-subscription updates for affected users. + +--- + +### CR-03: `Number(c.req.param(...))` — NaN propagates silently into DB queries + +**File:** `apps/api/src/routes/lists.ts:323, 407, 459, 521, 569, 663` + +**Issue:** Every route that reads a URL path parameter converts it with bare `Number(...)`. +`Number('abc')` is `NaN`. All subsequent Drizzle `eq(lists.id, NaN)` calls emit SQL like +`WHERE id = NaN` which MariaDB coerces to `WHERE id = 0`. This returns "not found" for +most paths, but the behavior is implementation-defined and fragile: + +- A crafted request to `PATCH /api/lists/abc` skips the `checkListAccess` notFound→404 + branch and returns a 404, which is benign but by accident. +- A crafted request to `GET /api/lists/abc/items` proceeds past the access check with + `listId = 0`, queries `WHERE list_id = 0` (no rows), and returns `{ items: [] }` — a + 200 with empty data rather than a 400. +- The `listItemsRouter` `PATCH /:itemId` at line 569 fetches `WHERE id = 0` from + `list_items`, gets no row, and returns 404, which again masks rather than rejects. + +Silently treating invalid input as a DB query is incorrect behavior. Every route should +validate the path parameter before touching the DB. + +**Fix:** Add NaN validation immediately after each `Number(...)` conversion: + +```typescript +const listId = Number(c.req.param('id')) +if (!Number.isInteger(listId) || listId < 1) { + return c.json({ error: 'Invalid id' }, 400) +} +``` + +Apply the same pattern to `itemId` at lines 569 and 663. `ListDetail.tsx` already does +this check for its own `parsedListId` (line 315), confirming the pattern is known; it +just was not applied server-side. + +--- + +## Warnings + +### WR-01: SSE listener registered as `async` but errors inside it are silently dropped + +**File:** `apps/api/src/routes/sse.ts:96-103` + +**Issue:** The handler passed to `subscribeListEvents` is declared `async`: + +```typescript +const unsub = subscribeListEvents(listId, async (event) => { + if (stream.aborted) return + await stream.writeSSE(...) +}) +``` + +`EventEmitter.emit()` does not await Promises returned by listeners. If `stream.writeSSE` +rejects (e.g. the underlying socket was half-closed but `stream.aborted` has not been set +yet), the rejection is an unhandled Promise rejection. Under Node.js 18+ this can crash +the process depending on the `unhandledRejection` policy. In production behind Pangolin the +risk is a silent dropped write followed by an eventual crash. + +**Fix:** Wrap the async body in a try/catch: + +```typescript +subscribeListEvents(listId, (event) => { + if (stream.aborted) return + stream.writeSSE({ + data: JSON.stringify(event), + event: event.type, + id: `${listId}-${Date.now()}`, + }).catch((err) => { + console.error('[sse/lists] writeSSE failed:', err) + }) +}) +``` + +--- + +### WR-02: Unsubscribers run AFTER the heartbeat loop exits — they may never run if `writeSSE` throws + +**File:** `apps/api/src/routes/sse.ts:107-119` + +**Issue:** The cleanup block (`unsubscribers.forEach(...)` at line 119) is placed after the +`while (!stream.aborted)` loop. If `stream.writeSSE` inside the heartbeat loop throws +synchronously, the loop exits via exception propagation and the `unsubscribers.forEach` +line is never reached. This leaves orphaned listeners attached to the module-level emitter +for the lifetime of the process — a listener leak that accumulates with every aborted +connection. + +In the current implementation `streamSSE` from Hono likely catches the inner Promise, but +the placement creates a fragile dependency on that behavior. + +**Fix:** Use a try/finally block to guarantee cleanup: + +```typescript +return streamSSE(c, async (stream) => { + const unsubscribers: Array<() => void> = [] + + try { + for (const listId of accessibleListIds) { + const unsub = subscribeListEvents(listId, (event) => { ... }) + unsubscribers.push(unsub) + } + + let tick = 0 + while (!stream.aborted) { + await stream.writeSSE({ ... }) + await stream.sleep(30_000) + } + } finally { + unsubscribers.forEach((unsub) => unsub()) + } +}) +``` + +--- + +### WR-03: `position` field in `patchItemSchema` accepts any string — no fractional-indexing format validation + +**File:** `apps/api/src/routes/lists.ts:107-117` + +**Issue:** `patchItemSchema` validates `position` as `z.string().min(1).max(255)`. A client +can send any arbitrary string as a rank (e.g. `"aaaaa..."` 255 chars, or `"\x00"`). +`fractional-indexing` has specific format constraints: keys must match a particular +character set and structure. An invalid rank value written to the DB will permanently +corrupt the ordering for all items in the list, since subsequent `generateKeyBetween` +calls against a malformed neighbor will throw or produce unpredictable output. + +This is particularly relevant because malformed ranks survive server-side silently — the +DB stores whatever string is written and returns it in ORDER BY, but `generateKeyBetween` +on the PWA side will throw when encountering an out-of-spec rank as a neighbor. + +**Fix:** Add a regex validator matching the fractional-indexing key format. The library +produces keys in `[A-Za-z0-9]` with specific leading-character rules. At minimum, restrict +to the documented safe character set: + +```typescript +position: z.string() + .min(1) + .max(255) + .regex(/^[A-Za-z0-9]+$/, 'Invalid fractional rank format'), +``` + +Or call `validateOrderKey` from the `fractional-indexing` package inside a `.refine()`. + +--- + +### WR-04: `PATCH /api/lists/:id` does not guard `isShared` changes against non-owner callers in test coverage + +**File:** `apps/api/tests/routes/lists.test.ts:438-450` + +**Issue:** The test `"sharee can rename a shared list they have access to"` asserts the +correct behaviour (sharees can rename), but there is no corresponding negative test +asserting that a sharee CANNOT change `isShared`. Given CR-01 above is a confirmed bug, +the absence of this test means the regression will go undetected after the fix unless a +test is added simultaneously. + +**Fix:** Add a test case in the `PATCH /api/lists/:id` describe block: + +```typescript +it('returns 403 when a sharee attempts to change isShared (owner-only)', async () => { + const ownerId = await seedUser('patch-isshared-owner') + const shareeId = await seedUser('patch-isshared-sharee') + const listId = await seedList(ownerId, 'Shared List', true) + await shareList(listId, shareeId) + + currentDevUserId = shareeId + const app = await getApp() + const res = await app.request( + jsonRequest('PATCH', `/api/lists/${listId}`, { isShared: false }), + ) + expect(res.status).toBe(403) +}) +``` + +--- + +### WR-05: Optimistic rank computation in `addMutation` can produce a duplicate rank when a concurrent add is in-flight + +**File:** `apps/pwa/src/routes/ListDetail.tsx:159-162` + +**Issue:** `addMutation.onMutate` computes the optimistic rank using: + +```typescript +const lastRank = activeItems.at(-1)?.rank ?? null +const optimisticRank = generateKeyBetween(lastRank, null) +``` + +`activeItems` is the locally-computed split of the React Query cache at the time the +mutation fires. If two concurrent adds are initiated in quick succession (e.g. rapid Enter +key taps), the second `onMutate` reads the cache that already contains the first optimistic +item (with `id: -Date.now()`). However, the first optimistic item's rank was computed from +the same `lastRank`, so `generateKeyBetween(lastRank, null)` is called twice with the +same `lastRank`, producing the same rank string for both optimistic items. + +Both items render visually without issue, but on settlement the first item gets rank R1 +from the server and the second gets rank R2 > R1. The transient duplicate rank in the cache +can cause a visible re-ordering flash during the `onSettled` invalidation. + +This is a cosmetic issue only (server round-trips produce correct ordering), but it violates +the "no accidental reorder flash" UX expectation. + +**Fix:** After the first optimistic insert, re-read the cache to get the updated last rank +for the second add. Since `onMutate` is async, read the updated cache state after +`cancelQueries` completes: + +```typescript +onMutate: async (text: string) => { + await queryClient.cancelQueries({ queryKey: ['list', parsedListId] }) + // Read AFTER cancel so concurrent in-flight optimistic updates are visible + const previous = queryClient.getQueryData(['list', parsedListId]) + const currentActiveItems = (previous?.items ?? []) + .filter((i) => !i.checked) + .sort((a, b) => (a.rank < b.rank ? -1 : 1)) + const lastRank = currentActiveItems.at(-1)?.rank ?? null + ... +} +``` + +--- + +## Info + +### IN-01: `useListSSE` connects to `/api/sse/lists` — not scoped to the current `listId` + +**File:** `apps/pwa/src/hooks/useListSSE.ts:65` + +**Issue:** The hook is parameterized on `listId` and invalidates `['list', listId]` on +events, but the SSE connection it opens is `/api/sse/lists` — the server-side global +fan-out stream for ALL lists the user can access. Events for other lists the user owns or +shares (e.g. a grocery list while viewing a gift list) also trigger `handleListChange`, +which only invalidates the currently-viewed list's query key. Events for other lists are +received and ignored, which is harmless but slightly wasteful. + +The `listId` parameter to the hook is used only for cache invalidation, not for scoping +the server subscription. This is by design per D-10, but the hook's name (`useListSSE`) +and the `listId` parameter imply it is scoped to one list, which may confuse future +maintainers. + +**Fix (documentation):** Add a comment clarifying that the connection is intentionally +global and `listId` is only the invalidation target. Alternatively, rename the parameter +to `activeListId` to signal its limited scope. + +--- + +### IN-02: `getAccessibleListIds` issues two sequential DB round-trips that could be one query + +**File:** `apps/api/src/lib/listAccess.ts:29-43` + +**Issue:** The function issues two separate `SELECT` queries — one for owned lists, one +for shared lists — then unions the results in JavaScript. This is two DB round-trips +where one `UNION` or a single query with an `OR` would suffice. In a two-member household +the cost is negligible; it is called at SSE connection time and can be called on every +request to `GET /api/lists` in the future. As the call count grows this becomes a latency +doubling point. + +**Fix:** Not urgent, but a single query avoids the double round-trip: + +```typescript +// Single query with OR +const rows = await db + .selectDistinct({ id: lists.id }) + .from(lists) + .leftJoin(listShares, eq(listShares.listId, lists.id)) + .where( + or(eq(lists.ownerId, userId), eq(listShares.userId, userId)), + ) +return rows.map((r) => r.id) +``` + +--- + +### IN-03: The 401 test in `lists.test.ts` is a no-op assertion + +**File:** `apps/api/tests/routes/lists.test.ts:263-287` + +**Issue:** The test `"returns 401 when no session is set"` contains the assertion +`expect(true).toBe(true)` with a comment explaining why the 401 path is not actually +exercised. The test body documents a known gap in test coverage (the dev-bypass path makes +it impossible to test 401 via the same app instance without module-level re-mocking). This +is a real gap — the 401 enforcement path is never exercised in the automated suite. + +The test gives false confidence by appearing in the describe block as a passing test while +asserting nothing about the code under review. + +**Fix:** Either remove the test (if it cannot be implemented), or implement it properly by +using `vi.doMock` before a fresh `import()` of `app` to override `devAuthBypass` to a +no-op in that test only, then assert `res.status === 401`. The pattern is already used in +the `@hono/oidc-auth` mock above it. Keeping a passing test that asserts `true === true` +is misleading. + +--- + +_Reviewed: 2026-06-09T15:30:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_