diff --git a/.planning/phases/04-shared-lists-live-sync/04-SECURITY.md b/.planning/phases/04-shared-lists-live-sync/04-SECURITY.md index 87edc54..cc21af2 100644 --- a/.planning/phases/04-shared-lists-live-sync/04-SECURITY.md +++ b/.planning/phases/04-shared-lists-live-sync/04-SECURITY.md @@ -1,10 +1,11 @@ --- phase: 4 slug: shared-lists-live-sync -status: issues_found -threats_open: 1 +status: verified +threats_open: 0 asvs_level: 1 created: 2026-06-09 +closed: 2026-06-09 --- # Phase 4 — Security @@ -31,15 +32,15 @@ created: 2026-06-09 | Threat ID | Category | Component | Disposition | Mitigation | Status | |-----------|----------|-----------|-------------|------------|--------| -| T-04-01 | Tampering | drizzle-kit push truncating populated tables | mitigate | generate+migrate only; `0001_lists_schema.sql` is additive (CREATE TABLE / ADD CONSTRAINT / CREATE INDEX), no DROP/TRUNCATE | closed | +| T-04-01 | Tampering | drizzle-kit push truncating populated tables | mitigate | generate+migrate only; `0001_lists_schema.sql` and `0002_yielding_mattie_franklin.sql` are additive (CREATE TABLE / ALTER TABLE MODIFY), no DROP/TRUNCATE | closed | | T-04-01b | Spoofing/AuthZ | unauthenticated SSE subscription | mitigate | `resolveUserId → 401`; endpoint behind OIDC middleware; client `withCredentials` | closed | | T-04-02 | Information Disclosure | scoped fan-out leak (D-04) — load-bearing | mitigate | per-list channel `list:${listId}` + `getAccessibleListIds`; GET /api/lists scoped | closed | | T-04-03 | Information Disclosure | getAccessibleListIds over-returning ids | mitigate | scoped to owner_id OR list_shares.userId; deduped via Set | closed | | T-04-04 | Denial of Service | EventEmitter max-listeners | accept | `setMaxListeners(200)` headroom | closed (accepted) | -| T-04-05 | Elevation of Privilege | accessing/mutating another member's list via direct id | mitigate | `checkListAccess` on every list + item handler; DELETE list owner-only; 403 otherwise — **but `isShared` reconciliation in PATCH is not owner-gated (see T-04-08)** | open | +| T-04-05 | Elevation of Privilege | accessing/mutating another member's list via direct id | mitigate | `checkListAccess` on every list + item handler; DELETE list owner-only; 403 otherwise; `isShared` reconciliation now owner-gated at `lists.ts:336` (plan 04-07) | closed | | T-04-06 | Tampering | XSS via list name / item text | mitigate | plain-text JSX children only; no `dangerouslySetInnerHTML` in ListCard/ItemRow | closed | | T-04-07 | Tampering | overposting on PATCH | mitigate | zod `patchListSchema` (name/isShared) + `patchItemSchema` exactly-one-of(checked/text/position) | closed | -| T-04-08 | Elevation of Privilege | self-adding to / manipulating list_shares | mitigate | claim: "shares server-managed only; no client-writable shares endpoint" — **DEFEATED: PATCH isShared toggle mutates list_shares for any sharee** | open | +| T-04-08 | Elevation of Privilege | self-adding to / manipulating list_shares | mitigate | Owner-only guard at `lists.ts:336`: `if (patch.isShared !== undefined && !access.isOwner) return 403`. A non-owner sharee can no longer delete or insert `list_shares` via PATCH `{ isShared }`. Verified by two negative tests (`lists.test.ts:452`, `lists.test.ts:477`): sharee → 403 + `list_shares` unchanged. (plan 04-07) | closed | | T-04-09 | Tampering | resurrecting a deleted item via in-flight edit (D-09) | mitigate | DELETE final; PATCH fetches row first, 404 if missing; no upsert path | closed | | T-04-10 | Denial of Service | pathological zipper inserts growing rank | accept | VARCHAR(255) headroom; fractional-indexing graceful degradation | closed (accepted) | | T-04-11 | Denial of Service | EventSource reconnect storm | mitigate | `es.close()` before setTimeout; bounded backoff; give up after `MAX_ATTEMPTS=6` | closed | @@ -51,45 +52,31 @@ created: 2026-06-09 --- -## Open Threat Detail +## Closed Threat Detail (Plan 04-07) -### T-04-08 (BLOCKER) — Sharee can rewrite list_shares via PATCH `isShared` +### T-04-08 — Sharee can rewrite list_shares via PATCH `isShared` — CLOSED -**File:** `apps/api/src/routes/lists.ts:319-393` +**Closed by:** plan 04-07 (`c0bd6d7`) +**File:** `apps/api/src/routes/lists.ts:334-338` -The declared mitigation for T-04-08 is "shares are server-managed only — no client-writable -shares endpoint." Verification of the PATCH handler shows this is **defeated**: - -- The handler gates only on `checkListAccess` (`lists.ts:327-332`), which returns `allowed:true` - for an owner **OR** any sharee. -- The `isShared` reconciliation block (`lists.ts:344-369`) runs **unconditionally for any - allowed user** — there is no `access.isOwner` guard. The inline comment at line 344 - ("owner only affects shares") asserts a guard that does not exist in code. -- A non-owner sharee sending `{ isShared: false }` reaches `lists.ts:365-367` → - `db.delete(listShares).where(eq(listShares.listId, listId))` — deleting **all** share rows - for the list, revoking every other member's access (an availability + integrity attack on - the owner's sharing state). -- A non-owner sharee sending `{ isShared: true }` reaches `lists.ts:348-362` → inserts a - `list_shares` row for **every other user** in the DB without the owner's consent. - -This is a client-writable path that mutates `list_shares`, contradicting the T-04-08 claim, -and is an Elevation-of-Privilege gap also touching T-04-05 (a sharee performs an owner-only -sharing mutation). It is independently documented as CR-01 in `04-REVIEW.md`. - -**Required fix (implementation — not applied by this audit):** add an owner-only guard before -the `isShared` write/reconciliation, e.g. after the access check at `lists.ts:327-332`: +The owner-only guard was added immediately after the `checkListAccess` block and before any `updateValues` construction: ```ts +// T-04-08 / T-04-05: owner-only guard for isShared mutations. +// A sharee may rename a list (patch.name) but must never mutate list_shares. if (patch.isShared !== undefined && !access.isOwner) { return c.json({ error: 'Only the list owner can change sharing settings' }, 403) } ``` -Add the corresponding negative test (`tests/routes/lists.test.ts`) asserting a sharee -receives 403 when toggling `isShared` (currently uncovered — WR-04). +**Test coverage (WR-04 now covered):** +- `lists.test.ts:452` — sharee sends `{ isShared: false }` → 403; `list_shares` row still exists (length 1). Proves the `db.delete(listShares)` path is unreachable for non-owners. +- `lists.test.ts:477` — sharee sends `{ isShared: true }` → 403; share count unchanged. Proves the `db.insert(listShares)` path is unreachable for non-owners. +- Pre-existing owner-toggle tests (false→true, true→false) and sharee-rename test continue to pass. -T-04-05 is marked `open` only because of this shared root cause; the direct-id access path for -GET/POST/PATCH-text/DELETE on lists and items is correctly gated and tested. +### T-04-05 — isShared reconciliation runs for any allowed user — CLOSED + +The shared root cause with T-04-08 (no `access.isOwner` guard on the reconciliation block) is resolved by the same guard. All other T-04-05 paths (GET/POST-item/PATCH-text/DELETE gated via `checkListAccess`) were already correct and remain so. --- @@ -132,6 +119,7 @@ any threat disposition under `block_on: high`. | Audit Date | Threats Total | Closed | Open | Run By | |------------|---------------|--------|------|--------| | 2026-06-09 | 14 | 13 | 1 | gsd-security-auditor | +| 2026-06-09 | 14 | 14 | 0 | gsd-verifier (re-verification after plan 04-07) | --- @@ -139,7 +127,7 @@ any threat disposition under `block_on: high`. - [x] All threats have a disposition (mitigate / accept / transfer) - [x] Accepted risks documented in Accepted Risks Log -- [ ] `threats_open: 0` confirmed — **1 open (T-04-08 / T-04-05 root cause)** -- [ ] `status: verified` set in frontmatter +- [x] `threats_open: 0` confirmed +- [x] `status: verified` set in frontmatter -**Approval:** pending — blocked on T-04-08 (CR-01) implementation fix + negative test +**Approval:** APPROVED — all 14 threats closed; T-04-08 and T-04-05 closed by plan 04-07 owner guard + negative tests. diff --git a/.planning/phases/04-shared-lists-live-sync/04-VERIFICATION.md b/.planning/phases/04-shared-lists-live-sync/04-VERIFICATION.md index 4dd3e19..1fd034d 100644 --- a/.planning/phases/04-shared-lists-live-sync/04-VERIFICATION.md +++ b/.planning/phases/04-shared-lists-live-sync/04-VERIFICATION.md @@ -1,31 +1,25 @@ --- phase: 04-shared-lists-live-sync -verified: 2026-06-09T14:00:00Z -status: gaps_found -score: 3/4 must-haves verified +verified: 2026-06-09T18:30:00Z +status: passed +score: 4/4 must-haves verified overrides_applied: 0 -gaps: - - truth: "A member can drag an active item to a new position and the order persists (reorder via drag-to-top)" - status: partial - reason: "rank column uses utf8mb4_uca1400_ai_ci (case-insensitive) collation. fractional-indexing generates uppercase-prefixed keys (e.g. 'Zz') when inserting before the first active item (drag-to-top). In the DB, 'Zz' < 'a0' evaluates FALSE (measured: db_less=0), but in JS it is TRUE. After a drag-to-top the PATCH writes a rank the DB ORDER BY rank sorts to the bottom — the dragged item lands at the bottom on next fetch, contradicting the user's action. Tests at line 931 of lists.test.ts explicitly work around this by restricting to lowercase ranks (a0-a5) and noting 'avoids collation issues with uppercase ranks'. No regression test for an uppercase-prefixed rank exists." - artifacts: - - path: "apps/api/src/db/schema.ts" - issue: "listItems.rank column defined as varchar(255) with no explicit collation — inherits table/DB default utf8mb4_uca1400_ai_ci instead of utf8mb4_bin" - - path: "apps/api/src/db/migrations/0001_lists_schema.sql" - issue: "rank column DDL does not specify COLLATE utf8mb4_bin — column gets case-insensitive collation from DB default" - - path: "apps/api/tests/routes/lists.test.ts" - issue: "reorder test at line 931 explicitly avoids uppercase ranks to sidestep the bug; no regression test that inserts an uppercase-prefixed rank and asserts ORDER BY returns JS string order" - missing: - - "Migrate rank column to COLLATE utf8mb4_bin via drizzle-kit generate+migrate (NEVER db:push on populated MariaDB)" - - "Add a regression test that seeds an uppercase-prefixed rank (e.g. 'Zz') as first item, PATCHes it to top, and asserts GET returns items in JS string order" +re_verification: + previous_status: gaps_found + previous_score: 3/4 + gaps_closed: + - "LIST-03 drag-to-top: list_items.rank migrated to COLLATE utf8mb4_bin (migration 0002); uppercase-prefixed rank 'Zz' now sorts before 'a0' in DB ORDER BY, matching JS string order; regression test added" + - "T-04-08 / T-04-05: owner-only guard added at lists.ts:336; sharee sending { isShared } receives 403; list_shares never mutated by non-owner; negative tests added and passing" + gaps_remaining: [] + regressions: [] --- # Phase 4: Shared Lists + Live Sync Verification Report **Phase Goal:** Both members can create and manage shared named lists with real-time co-edit sync — edits by one member appear for the other without any manual refresh -**Verified:** 2026-06-09T14:00:00Z -**Status:** gaps_found -**Re-verification:** No — initial verification +**Verified:** 2026-06-09T18:30:00Z +**Status:** passed +**Re-verification:** Yes — after gap-closure plan 04-07 --- @@ -35,31 +29,51 @@ gaps: | # | Truth | Status | Evidence | |---|-------|--------|----------| -| 1 | Either member can create a named list and delete a list they no longer need | VERIFIED | `POST /api/lists` with auto-share wired in `lists.ts:249`; `DELETE /api/lists/:id` owner-only at `lists.ts:403`; `CreateListSheet.tsx` (336 lines, real form); `ListDeleteDialog.tsx` (196 lines); 181 API tests pass including scoped-access and cascade-delete tests | -| 2 | Either member can add items to a list, check items off, reorder them by drag-and-drop, and delete individual items | VERIFIED | `POST /:id/items` + `PATCH /list-items/:itemId` (exact-one-field LWW) + `DELETE /list-items/:itemId`; `ListDetail.tsx` (501 lines) with `DndContext`/`SortableContext`; `ItemRow.tsx` (273 lines) with `useSortable`; fractional rank assigned on create; optimistic mutations wired for all four operations; 181 API tests pass | +| 1 | Either member can create a named list and delete a list they no longer need | VERIFIED | `POST /api/lists` with auto-share wired in `lists.ts:249`; `DELETE /api/lists/:id` owner-only at `lists.ts:403`; `CreateListSheet.tsx` (336 lines, real form); `ListDeleteDialog.tsx` (196 lines); 184 API tests pass | +| 2 | Either member can add items to a list, check items off, reorder them by drag-and-drop, and delete individual items | VERIFIED | `POST /:id/items` + `PATCH /list-items/:itemId` (exact-one-field LWW) + `DELETE /list-items/:itemId`; `ListDetail.tsx` (501 lines) with `DndContext`/`SortableContext`; `ItemRow.tsx` (273 lines) with `useSortable`; fractional rank assigned on create; optimistic mutations wired for all four operations; 184 API tests pass | | 3 | When one member adds or checks off an item, the other member sees the change appear without refreshing — even after a brief network gap | VERIFIED | `publishListEvent` called after every write in `lists.ts`; scoped `GET /api/sse/lists` in `sse.ts:85` subscribes per accessible list via `getAccessibleListIds`; `useListSSE.ts` (114 lines) implements bounded-backoff EventSource (D-11); `refetchInterval:30000` polling fallback active (D-12); D-04 no-leak invariant tested in `lists.test.ts:856` | +| 4 | A member can drag an active item to a new position and the order persists (reorder via drag-to-top) | VERIFIED | `list_items.rank` column migrated to `COLLATE utf8mb4_bin` via migration `0002_yielding_mattie_franklin.sql`; upstream uppercase-prefixed rank `'Zz'` (produced by `generateKeyBetween(null, 'a0')`) now sorts BEFORE lowercase ranks in DB `ORDER BY rank`, matching JS string order; drag-to-top persists across refetch; regression test at `lists.test.ts:1091` passes against real DB | -**Score (roadmap truths):** 3/3 met, but Truth 2 has a partial defect at drag-to-top ordering — see gap below. +**Score:** 4/4 truths verified — phase goal fully achieved. -### Partial Defect: LIST-03 Drag-to-Top (Rank Collation) +--- -LIST-03 reorder works correctly for moves within the lowercase rank space (a0–a5 range) but fails for drag-to-top because `fractional-indexing` generates an uppercase-prefixed key (e.g. `Zz`) when prepending before the first item. +### Gap Closure Detail: LIST-03 Rank Collation -**Measured divergence (live DB query):** -``` -SELECT ('Zz' < 'a0') AS db_less, ('Zz' < 'a0' COLLATE utf8mb4_bin) AS bin_less -→ db_less: 0 bin_less: 1 -``` +**Root cause (previously):** `list_items.rank` inherited the DB default `utf8mb4_uca1400_ai_ci` (case-insensitive), causing `ORDER BY rank` to place uppercase-prefixed keys (`Zz`) AFTER lowercase keys (`a0`), contradicting JS string order. -The DB returns `FALSE` for `'Zz' < 'a0'` under `utf8mb4_uca1400_ai_ci` (case-insensitive), but JS string comparison returns `true`. So: -- User drags item to the top. -- Client computes `generateKeyBetween(null, firstActiveRank)` → `'Zz'` (uppercase prefix). -- PATCH writes `rank = 'Zz'` to DB. -- On next refetch, DB `ORDER BY rank` sorts `'Zz'` AFTER all `'a...'` ranks → the dragged item surfaces at the bottom. +**Fix applied (plan 04-07):** +- `apps/api/src/db/schema.ts`: `varcharBin` `customType` factory emits `varchar(255) COLLATE utf8mb4_bin`; `listItems.rank` switched from bare `varchar` to `varcharBin('rank').notNull()`. +- `apps/api/src/db/migrations/0002_yielding_mattie_franklin.sql`: Single-line `ALTER TABLE list_items MODIFY COLUMN rank varchar(255) COLLATE utf8mb4_bin NOT NULL` — no DROP, no TRUNCATE, no length or nullability change. Applied via `db:migrate` (never `db:push`). +- `apps/api/tests/routes/lists.test.ts:1091`: Regression test seeds item with rank `a0`, drags second item to top via PATCH `{ position: 'Zz' }`, then GETs items and asserts `Zz`-ranked item is at index 0. Exercises real DB `ORDER BY rank`. -The test suite was written to avoid this: `lists.test.ts:931` comment reads _"This stays within the normal rank space and avoids collation issues with uppercase ranks."_ No test covers the upstream failure path. +**Verification:** +- Migration body: `grep -ciE 'drop|truncate' 0002_yielding_mattie_franklin.sql` → `0` (confirmed) +- Schema: `grep -c 'utf8mb4_bin' schema.ts` → `4` (factory definition + 3 doc comments) +- Guard line: `lists.ts:336` confirmed +- Test suite: 184/184 pass against live MariaDB (`DB_HOST=127.0.0.1`) -**Requirement mapping:** LIST-03 is PARTIAL (moves within lowercase range work; drag-to-top breaks on refetch). +### Gap Closure Detail: T-04-08 / T-04-05 Owner Guard + +**Root cause (previously):** PATCH `/:id` `isShared` reconciliation block ran for any allowed user (owner OR sharee). A sharee sending `{ isShared: false }` deleted all `list_shares` rows; sending `{ isShared: true }` injected shares for every user without owner consent. + +**Fix applied (plan 04-07):** +- `apps/api/src/routes/lists.ts:334-338`: Owner-only guard inserted after `checkListAccess` and before `updateValues` construction: + ``` + if (patch.isShared !== undefined && !access.isOwner) { + return c.json({ error: 'Only the list owner can change sharing settings' }, 403) + } + ``` + Stale inline comment at line 350 updated to reflect the now-real guard. +- `apps/api/tests/routes/lists.test.ts:452-500`: Two new negative tests (T-04-08): + 1. Sharee sends `{ isShared: false }` → asserts 403 + `list_shares` unchanged (sharee row still present, length `1`). + 2. Sharee sends `{ isShared: true }` on private list → asserts 403 + no new shares inserted (count unchanged). + +**Verification:** +- Guard present: `grep -n "patch.isShared !== undefined && !access.isOwner" lists.ts` → line 336 (confirmed) +- Sharee rename test (pre-existing, `lists.test.ts:438`) still passes — rename allowed for sharees, only `isShared` is owner-gated. +- All owner-path `isShared` toggle tests (`false→true`, `true→false`) still pass (no regression). +- 184/184 API tests pass. --- @@ -67,11 +81,12 @@ The test suite was written to avoid this: `lists.test.ts:931` comment reads _"Th | Artifact | Expected | Status | Details | |----------|----------|--------|---------| -| `apps/api/src/db/schema.ts` | lists, listShares, listItems Drizzle tables | VERIFIED | All three tables present at lines 174–241; correct FK refs, indexes, D-01/D-02/D-13 column shapes | -| `apps/api/src/db/migrations/0001_lists_schema.sql` | Additive CREATE TABLE migration | VERIFIED | File exists; CREATE TABLE for lists/list_items/list_shares; no DROP/TRUNCATE of existing tables; FKs correct | +| `apps/api/src/db/schema.ts` | lists, listShares, listItems Drizzle tables; listItems.rank with COLLATE utf8mb4_bin | VERIFIED | All three tables present; `varcharBin` customType factory at lines 22-25 emits `varchar(255) COLLATE utf8mb4_bin`; `rank` column uses `varcharBin` | +| `apps/api/src/db/migrations/0001_lists_schema.sql` | Additive CREATE TABLE migration | VERIFIED | File exists; CREATE TABLE for lists/list_items/list_shares; no DROP/TRUNCATE; FKs correct | +| `apps/api/src/db/migrations/0002_yielding_mattie_franklin.sql` | Non-destructive ALTER TABLE for rank collation | VERIFIED | Single-line `ALTER TABLE list_items MODIFY COLUMN rank varchar(255) COLLATE utf8mb4_bin NOT NULL`; 0 DROP/TRUNCATE occurrences; applied via db:migrate | | `apps/pwa/src/components/BottomTabBar.tsx` | Calendar/Lists bottom tab navigation | VERIFIED | 88 lines; NavLink to `/calendar` and `/lists`; 44px+ touch targets; active-state accent via isActive callback | | `apps/pwa/src/routes/ListsIndex.tsx` | Lists surface with empty state | VERIFIED | Full implementation; useQuery(['lists']); ListsEmptyState; ListCard; CreateListSheet; ListDeleteDialog wired | -| `apps/api/src/routes/lists.ts` | POST/GET/PATCH/DELETE /api/lists with scoped access | VERIFIED | 693 lines; all four verbs; scoped GET (owner + shares); auto-share on create; publishListEvent fan-out on every write | +| `apps/api/src/routes/lists.ts` | POST/GET/PATCH/DELETE /api/lists with scoped access; owner-only isShared guard | VERIFIED | 693+ lines; all four verbs; scoped GET; auto-share on create; owner guard at line 336; publishListEvent fan-out on every write | | `apps/pwa/src/components/CreateListSheet.tsx` | New-list form with shared/private toggle | VERIFIED | 336 lines; default shared=true; form validation; useMutation wired | | `apps/pwa/src/components/ListCard.tsx` | List summary card navigating to /lists/:id | VERIFIED | 176 lines; item counts; navigate to /lists/:id | | `apps/pwa/src/components/ListDeleteDialog.tsx` | List-delete confirmation (D-06) | VERIFIED | 196 lines; reuses dialog pattern; owner-only delete path | @@ -84,6 +99,7 @@ The test suite was written to avoid this: `lists.test.ts:931` comment reads _"Th | `apps/pwa/src/components/LiveSyncIndicator.tsx` | Connected/reconnecting/disconnected indicator | VERIFIED | 119 lines; three distinct render branches; role="status"/"alert" | | `apps/api/src/lib/listEmitter.ts` | In-memory scoped event emitter | VERIFIED | 55 lines; module-level singleton; per-list channels `list:${listId}`; publish/subscribe/unsubscribe | | `apps/api/src/lib/listAccess.ts` | getAccessibleListIds access-scope query | VERIFIED | 43 lines; owned UNION shared; deduplicated; used by SSE endpoint | +| `apps/api/tests/routes/lists.test.ts` | Regression test (LIST-03 collation) + T-04-08 negative tests | VERIFIED | Three new tests: collation regression at line 1091, T-04-08 false→403 at line 452, T-04-08 true→403 at line 477; all pass | ### Key Link Verification @@ -92,29 +108,32 @@ The test suite was written to avoid this: `lists.test.ts:931` comment reads _"Th | `apps/pwa/src/App.tsx` | `/lists` route | `BrowserRouter` + `BottomTabBar` NavLink | WIRED | `App.tsx:31`; `} />`; `} />` | | `apps/api/src/index.ts` | `listsRouter` + `listItemsRouter` | `app.route('/api/lists', listsRouter)` + `app.route('/api/list-items', listItemsRouter)` | WIRED | `index.ts:65-66`; both routers mounted | | `apps/api/src/routes/lists.ts` | `list_shares` | Auto-insert shares on create + scoped GET | WIRED | `lists.ts:264-277` (auto-share POST); `lists.ts:169-183` (scoped GET via owned+shared IDs) | +| `apps/api/src/routes/lists.ts PATCH /:id` | `list_shares` reconciliation block | owner-only guard at line 336 returning 403 for non-owner isShared writes | WIRED | Guard at `lists.ts:336`; reconciliation block at lines 351-374 unreachable for non-owners when `patch.isShared` present | | `apps/pwa/src/routes/ListsIndex.tsx` | `/api/lists` | `useQuery + useMutation` in `listsClient` | WIRED | `ListsIndex.tsx:43` (`useQuery(['lists'], fetchLists)`); `useMutation(deleteList)` wired | | `apps/pwa/src/routes/ListDetail.tsx` | `/api/lists/:id/items` + `/api/list-items/:id` | `useQuery + optimistic mutations` | WIRED | `ListDetail.tsx:138-186` (fetch + mutations using `fetchListItems`/`addItem`/`patchListItem`/`deleteItem`) | | `apps/api/src/routes/lists.ts` | `publishListEvent` | Fan-out trigger after every successful write | WIRED | Calls present after POST list (`lists.ts:287`), PATCH list (`lists.ts:379`), DELETE list (`lists.ts:425`), POST item (`lists.ts:491`), PATCH item (`lists.ts:634`), DELETE item (`lists.ts:685`) | | `apps/api/src/routes/sse.ts` | `subscribeListEvents + getAccessibleListIds` | Scoped per-list subscription inside `streamSSE` | WIRED | `sse.ts:89` (`getAccessibleListIds`); `sse.ts:96` (`subscribeListEvents` per listId in loop) | | `apps/pwa/src/hooks/useListSSE.ts` | `/api/sse/lists` | `new EventSource(withCredentials) → invalidateQueries` | WIRED | `useListSSE.ts:65` (`new EventSource('/api/sse/lists', { withCredentials: true })`); event listeners call `handleListChange` which invalidates `['list', listId]` | | `apps/pwa/src/routes/ListDetail.tsx` | `useListSSE` | Mounted in `ListDetail` render; `setSyncState` passed to hook | WIRED | `ListDetail.tsx:116` (`useListSSE({ listId: parsedListId, onStateChange: setSyncState })`); `` at line 385 | +| `apps/api/src/db/schema.ts listItems.rank` | `MariaDB list_items.rank` column | `varcharBin` customType → `ALTER TABLE ... MODIFY rank ... COLLATE utf8mb4_bin` | WIRED | `schema.ts:22-25` defines `varcharBin`; `schema.ts:243` applies to `rank`; `0002_yielding_mattie_franklin.sql` applies the ALTER TABLE; migration applied via `db:migrate` | ### Data-Flow Trace (Level 4) | Artifact | Data Variable | Source | Produces Real Data | Status | |----------|---------------|--------|--------------------|--------| | `ListsIndex.tsx` | `data?.lists` | `useQuery(['lists'], fetchLists)` → `GET /api/lists` → DB query (`lists` + `listShares` + `listItems` COUNT) | Yes — DB query with scoped WHERE clause | FLOWING | -| `ListDetail.tsx` | `data?.items` | `useQuery(['list', listId], fetchListItems)` → `GET /api/lists/:id/items` → DB `SELECT ... ORDER BY rank ASC` | Yes — DB query returning real items | FLOWING | +| `ListDetail.tsx` | `data?.items` | `useQuery(['list', listId], fetchListItems)` → `GET /api/lists/:id/items` → DB `SELECT ... ORDER BY rank ASC` using `utf8mb4_bin`-collated `rank` column | Yes — DB query returning real items in correct order | FLOWING | | `sse.ts /lists` | SSE events | `subscribeListEvents` ← `publishListEvent` triggered by route writes → real DB mutations | Yes — events fire only after confirmed DB writes | FLOWING | ### Behavioral Spot-Checks | Behavior | Command | Result | Status | |----------|---------|--------|--------| -| API test suite (181 tests) | `pnpm --filter @familysync/api exec vitest run` | 181 passed, 0 failed (17 test files) | PASS | -| PWA test suite (160 tests) | `pnpm --filter @familysync/pwa test` | 160 passed, 0 failed (14 test files) | PASS | -| Production build | `pnpm build` | Built in 413ms; SW generated; no TypeScript errors | PASS | -| Rank column collation in DB | `SELECT ('Zz' < 'a0') AS db_less` | `db_less: 0` (FALSE) vs JS `'Zz' < 'a0'` = true | FAIL — confirms rank gap | +| API test suite (184 tests) | `set -a; . .env; set +a; export DB_HOST=127.0.0.1 DB_PORT=3306; pnpm --filter @familysync/api exec vitest run` | 17 test files, 184 passed, 0 failed | PASS | +| TypeScript typecheck | `pnpm --filter @familysync/api typecheck` | Clean (no errors) | PASS | +| Migration non-destructive | `grep -ciE 'drop\|truncate' apps/api/src/db/migrations/0002_yielding_mattie_franklin.sql` | 0 | PASS | +| rank collation in schema | `grep -c 'utf8mb4_bin' apps/api/src/db/schema.ts` | 4 | PASS | +| Owner guard in lists.ts | `grep -n "patch.isShared !== undefined && !access.isOwner" apps/api/src/routes/lists.ts` | Line 336 | PASS | ### Probe Execution @@ -126,38 +145,36 @@ No `scripts/*/tests/probe-*.sh` probes declared or found for this phase. |-------------|------------|-------------|--------|----------| | LIST-01 | 04-03-PLAN.md | User can create and delete named lists | SATISFIED | `POST /DELETE /api/lists`; auto-share; cascade delete; API tests pass | | LIST-02 | 04-04-PLAN.md | User can add items, check off, delete | SATISFIED | `POST/PATCH/DELETE /api/list-items`; D-05 checked-sink; D-08 single-field PATCH; API tests pass | -| LIST-03 | 04-05-PLAN.md | User can reorder items within a list | PARTIAL | dnd-kit + fractional rank wired and works for lowercase rank space; drag-to-top broken due to rank column collation (utf8mb4_uca1400_ai_ci vs required utf8mb4_bin) | +| LIST-03 | 04-05-PLAN.md | User can reorder items within a list | SATISFIED | dnd-kit + fractional rank wired; rank column carries `COLLATE utf8mb4_bin` via migration 0002; drag-to-top persists — collation regression test passes against real DB | | LIST-04 | 04-06-PLAN.md | Both members' list edits appear live without manual refresh | SATISFIED | scoped SSE endpoint; publishListEvent on every write; useListSSE bounded-backoff hook; D-04 no-leak tested; polling fallback active | ### Anti-Patterns Found | File | Line | Pattern | Severity | Impact | |------|------|---------|----------|--------| -| `apps/pwa/src/routes/ListsIndex.tsx` | 66 | `TODO: surface "Couldn't delete. Try again." toast (Plan 06 / notification layer)` | Info | Error feedback on delete failure absent; rollback still happens (cache restored); functional correctness unaffected | +| `apps/pwa/src/routes/ListsIndex.tsx` | 66 | `TODO: surface "Couldn't delete. Try again." toast (Plan 06 / notification layer)` | Info | Error feedback on delete failure absent; rollback still happens (cache restored); functional correctness unaffected; deferred to Phase 6 notification layer | | `apps/pwa/src/routes/ListDetail.tsx` | 379-381 | List detail header shows literal "List" instead of the list name | Info | UX limitation (no extra fetch for name in detail view); noted as future improvement; all item operations work correctly | -| `apps/api/src/db/schema.ts` | 231 | `rank: varchar(255)` with no COLLATE override — inherits utf8mb4_uca1400_ai_ci | BLOCKER | Drag-to-top produces uppercase-prefixed rank that DB sorts incorrectly vs JS; see gap | -**Debt markers:** Zero `TBD`, `FIXME`, or `XXX` markers found in any Phase 4 source file. +**Debt markers:** Zero `TBD`, `FIXME`, or `XXX` markers found in any Phase 4 source file (including 04-07 additions). ### Human Verification Required -None — all functional behaviors are verifiable via automated tests or direct code inspection. The drag-to-top ordering failure is confirmed by live DB query rather than requiring visual testing. +None — all functional behaviors verified via automated tests or direct code inspection. The drag-to-top fix is confirmed by passing DB-backed regression test. The T-04-08 guard is confirmed by passing negative tests that assert both the 403 response and the `list_shares` table state. --- ### Gaps Summary -**One open gap** blocks full LIST-03 goal achievement: +No open gaps. All four LIST-01..LIST-04 requirements are satisfied. Phase 04 is complete. -**Root cause:** The `list_items.rank` column was created without an explicit collation and inherited the database/table default (`utf8mb4_uca1400_ai_ci`), which is case-insensitive. The `fractional-indexing` library uses a base-62 character set that generates uppercase-prefixed keys (e.g. `Zz`) when inserting before the first item. Under case-insensitive collation, `'Zz' < 'a0'` is FALSE in MariaDB (uppercase letters sort after lowercase), so `ORDER BY rank ASC` returns rows in the wrong order after a drag-to-top. +**Previous gap now closed:** +- LIST-03 drag-to-top: `rank` column carries `COLLATE utf8mb4_bin` in schema and the live DB (applied via additive `ALTER TABLE` migration, no destructive operations). Regression test passes. +- T-04-08 / T-04-05: Owner-only guard at `lists.ts:336` blocks non-owner `isShared` mutations. Negative tests confirm 403 and unchanged `list_shares` for both `false→` and `true→` paths. -**Impact:** Drag-to-top is silently broken for the end user — the item appears to move but snaps back to the bottom on the next refetch. Moves within the lowercase range (e.g. `a0`→`a5`) are unaffected because both JS and MariaDB order them correctly. - -**Required fix:** Migrate the `rank` column to `COLLATE utf8mb4_bin` via `drizzle-kit generate+migrate`. Never use `db:push` on this populated DB. Add a regression test that seeds an uppercase-prefixed rank, reads items back, and asserts the returned order matches JS string order. - -**All other LIST-01, LIST-02, and LIST-04 behaviors are fully implemented and tested.** 181 API tests and 160 PWA tests pass. The production build is clean. +**All LIST-01, LIST-02, LIST-03, LIST-04 behaviors are fully implemented and tested.** 184 API tests pass (up from 181 before gap-closure). TypeScript typecheck clean. Production build passes. --- -_Verified: 2026-06-09T14:00:00Z_ +_Initial verification: 2026-06-09T14:00:00Z_ +_Re-verification (gap-closure 04-07): 2026-06-09T18:30:00Z_ _Verifier: Claude (gsd-verifier)_