From 797338424dea10bf7ca3b32148d10c24d87fac15 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Tue, 9 Jun 2026 14:07:14 -0400 Subject: [PATCH] docs(04): gap-closure plan 04-07 (rank collation + isShared owner guard) --- .planning/ROADMAP.md | 6 +- .../04-shared-lists-live-sync/04-07-PLAN.md | 207 ++++++++++++++++++ 2 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 .planning/phases/04-shared-lists-live-sync/04-07-PLAN.md diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index cd76425..a49c432 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -143,7 +143,7 @@ Plans: **Entry gate status (2026-06-08):** CLEARED — SSE-over-Pangolin smoke test PASSED (35 heartbeats over ~6 min, buffering off, no cut). Live sync may be built directly on SSE; polling fallback (D-12) retained as belt-and-suspenders. -**Plans**: 6 plans +**Plans**: 7 plans (6 + 1 gap-closure) Plans: **Wave 1** @@ -166,6 +166,10 @@ Plans: - [x] 04-06-PLAN.md — Live-sync slice: scoped /api/sse/lists + fan-out triggers + useListSSE bounded-backoff hook + LiveSyncIndicator + polling fallback (LIST-04, D-04/D-10/D-11/D-12) +**Wave 6** *(gap closure — blocked on Waves 2 + 4)* + +- [ ] 04-07-PLAN.md — Gap closure: migrate list_items.rank to COLLATE utf8mb4_bin (LIST-03 drag-to-top) + owner-only guard on PATCH isShared (T-04-08/T-04-05) — two TDD features (LIST-03) + **UI hint**: yes ### Phase 5: Web Push Notifications diff --git a/.planning/phases/04-shared-lists-live-sync/04-07-PLAN.md b/.planning/phases/04-shared-lists-live-sync/04-07-PLAN.md new file mode 100644 index 0000000..bd9c203 --- /dev/null +++ b/.planning/phases/04-shared-lists-live-sync/04-07-PLAN.md @@ -0,0 +1,207 @@ +--- +phase: 04-shared-lists-live-sync +plan: 07 +type: tdd +wave: 6 +depends_on: ["04-03", "04-05"] +files_modified: + - apps/api/src/db/schema.ts + - apps/api/src/db/migrations + - apps/api/src/routes/lists.ts + - apps/api/tests/routes/lists.test.ts +autonomous: true +gap_closure: true +requirements: [LIST-03] +must_haves: + truths: + - "A member can drag an active item to a new position and the order persists (reorder via drag-to-top) — closes LIST-03 gap" + - "T-04-08 closed: a non-owner sharee sending { isShared } to PATCH /api/lists/:id receives 403; list_shares is never mutated by a sharee" + - "T-04-05 closed: the isShared reconciliation block runs only for the list owner (access.isOwner === true)" + artifacts: + - path: "apps/api/src/db/schema.ts" + provides: "listItems.rank column with explicit COLLATE utf8mb4_bin" + contains: "utf8mb4_bin" + - path: "apps/api/src/routes/lists.ts" + provides: "owner-only guard before isShared reconciliation in PATCH /:id" + contains: "access.isOwner" + - path: "apps/api/tests/routes/lists.test.ts" + provides: "rank-collation regression test + sharee-403 negative test" + key_links: + - from: "apps/api/src/routes/lists.ts PATCH /:id" + to: "list_shares reconciliation block" + via: "owner-only guard returning 403 for non-owner isShared writes" + pattern: "access\\.isOwner" + - from: "apps/api/src/db/schema.ts listItems.rank" + to: "MariaDB list_items.rank column" + via: "generate+migrate ALTER TABLE ... MODIFY rank ... COLLATE utf8mb4_bin" + pattern: "utf8mb4_bin" +--- + + +Close the two open gaps blocking Phase 4 sign-off: + +1. **LIST-03 drag-to-top (rank collation)** — `list_items.rank` inherited the case-insensitive DB default collation (`utf8mb4_uca1400_ai_ci`). `fractional-indexing` emits uppercase-prefixed keys (e.g. `Zz`) on drag-to-top, which MariaDB sorts AFTER lowercase `a…` ranks even though JS sorts it BEFORE. The dragged item snaps to the bottom on refetch. Fix: migrate the column to `COLLATE utf8mb4_bin` so DB `ORDER BY rank` matches JS string order. + +2. **T-04-08 / T-04-05 (security BLOCKER)** — The PATCH `/:id` `isShared` reconciliation block runs for ANY allowed user, including sharees. A non-owner sharee can delete every share row (`isShared:false`) or inject shares for all users (`isShared:true`). Fix: add an owner-only guard returning 403 when a non-owner sends `isShared`. + +Both gaps are TDD: known-failing behavior with a defined assertion. Each feature follows RED → GREEN. + +Purpose: Achieve `threats_open: 0` in 04-SECURITY.md and full LIST-03 satisfaction in 04-VERIFICATION.md. +Output: One additive migration SQL file, one schema collation edit, one owner-only guard, two new test cases. + +DO NOT modify or replan 04-01 through 04-06 — they are VERIFIED. This plan adds NEW behavior and tests only. + + + +@$HOME/.claude/gsd-core/workflows/execute-plan.md +@$HOME/.claude/gsd-core/templates/summary.md + + + +@.planning/STATE.md +@.planning/ROADMAP.md +@.planning/REQUIREMENTS.md +@.planning/phases/04-shared-lists-live-sync/04-VERIFICATION.md +@.planning/phases/04-shared-lists-live-sync/04-SECURITY.md +@.planning/phases/04-shared-lists-live-sync/04-CONTEXT.md +@apps/api/src/db/schema.ts +@apps/api/src/db/migrations/0001_lists_schema.sql +@apps/api/src/routes/lists.ts +@apps/api/tests/routes/lists.test.ts +@apps/api/drizzle.config.ts +@apps/api/package.json + + + +- **MariaDB only. NEVER `drizzle-kit push` (`pnpm db:push`).** `push` emits a false destructive diff that truncates populated tables. Use `pnpm --filter @familysync/api db:generate` to emit the migration SQL, then `pnpm --filter @familysync/api db:migrate` to apply it. The schema-push gate's default push task is OVERRIDDEN for this phase. +- The new migration MUST be a non-destructive `ALTER TABLE ... MODIFY` — NO DROP, NO TRUNCATE. Preserve `varchar(255)`, `NOT NULL`, and existing default/index semantics exactly. +- API integration tests live in `apps/api/tests/` (NEVER `src/`) and run against the real dev MariaDB. The regression test MUST exercise the real DB so it observes the column's actual collation, not JS comparison. +- Test run prelude (matches the file header at `lists.test.ts:7-10`): `set -a; . ./apps/api/.env 2>/dev/null; set +a; export DB_HOST=127.0.0.1 DB_PORT=3306`. Drizzle-kit reads the same `DB_*` env vars (see `drizzle.config.ts`). +- `` blocks below name identifiers and behavior only — no fenced code blocks / full implementations. + + + + + + Task 1: RED → GREEN — rank-collation drag-to-top regression (LIST-03) + apps/api/tests/routes/lists.test.ts, apps/api/src/db/schema.ts, apps/api/src/db/migrations/ + + - apps/api/tests/routes/lists.test.ts:919-982 — existing reorder describe block + seed helpers (`seedUser`, `seedList`, `seedItem`, `getApp`, `jsonRequest`, `currentDevUserId`). The test at line 930-932 explicitly sidesteps this bug with the comment "avoids collation issues with uppercase ranks". + - apps/api/src/db/schema.ts:222-241 — `listItems` table; `rank` is `varchar('rank', { length: 255 }).notNull()` at line 231 with no `.$type`/collation. + - apps/api/src/db/migrations/0001_lists_schema.sql:26-35 — existing additive CREATE TABLE style; the new migration must follow the same `--> statement-breakpoint` format drizzle-kit emits. + - apps/api/drizzle.config.ts — `out: './src/db/migrations'`, `dialect: 'mysql'`; confirms generate writes here and reads `DB_*` env. + - apps/api/package.json:14-15 — `db:generate` and `db:migrate` scripts. + - 04-VERIFICATION.md gap (frontmatter `gaps:` + "Measured divergence"): `SELECT ('Zz' < 'a0')` returns `0` under the current collation but `('Zz' < 'a0' COLLATE utf8mb4_bin)` returns `1`. + + + RED — Add a regression test inside the existing `describe('PATCH /api/list-items/:id { position } — reorder ordering (LIST-03, D-13)')` block in `lists.test.ts`. Title it to name the bug (e.g. "drag-to-top: uppercase-prefixed rank sorts above lowercase ranks (LIST-03 collation regression)"). The test must: + - seed an owner, set `currentDevUserId`, seed a private list; + - seed two active items where the FIRST has a lowercase rank (e.g. `a0`) and a SECOND item; + - simulate drag-to-top of the second item by PATCHing `/api/list-items/:id` with `{ position: 'Zz' }` (the uppercase-prefixed key `fractional-indexing`'s `generateKeyBetween(null, 'a0')` produces when prepending before the first item — assert `'Zz' < 'a0'` is `true` in JS first to document intent); + - GET `/api/lists/:listId/items` and assert the dragged item (`rank: 'Zz'`) is returned FIRST (index 0), matching JS string order. + Run the test BEFORE the schema change and confirm it FAILS (the item lands last) — this is the RED proof. Do not weaken the assertion to make it pass in JS; it must hit the real DB `ORDER BY rank`. + + GREEN (schema) — In `schema.ts`, change the `listItems.rank` column so it carries an explicit binary collation. Preserve `varchar` length `255` and `.notNull()` exactly; add the `utf8mb4_bin` collation via drizzle's column collation option for the mysql varchar type. Do NOT touch any other column, index, or table. + + GREEN (migrate — [BLOCKING], must run before the test passes) — From repo root, with the env prelude loaded, run `pnpm --filter @familysync/api db:generate`. Inspect the newly emitted SQL file under `apps/api/src/db/migrations/` (next sequential number, e.g. `0002_*.sql`): it MUST be a single non-destructive `ALTER TABLE list_items MODIFY ... rank varchar(255) ... COLLATE utf8mb4_bin NOT NULL` (or drizzle's equivalent MODIFY/CHANGE form) with NO DROP/TRUNCATE and NO change to length or nullability. If generate emits anything destructive, STOP and report — do not edit the SQL by hand to hide it. Then apply with `pnpm --filter @familysync/api db:migrate`. NEVER run `db:push`. + + After migrate, re-run the regression test — it now passes because DB `ORDER BY rank` under `utf8mb4_bin` matches JS order. + + + set -a; . ./apps/api/.env 2>/dev/null; set +a; export DB_HOST=127.0.0.1 DB_PORT=3306; pnpm --filter @familysync/api exec vitest run tests/routes/lists.test.ts -t "collation regression" + + + - The new test exists in the LIST-03 reorder describe block and asserts the `'Zz'`-ranked item is returned at index 0 from GET items. + - A new migration file exists under `apps/api/src/db/migrations/` whose body is an `ALTER TABLE list_items` MODIFY/CHANGE statement containing `utf8mb4_bin`, with zero occurrences of `DROP` or `TRUNCATE` (verify: `grep -ciE 'drop|truncate' apps/api/src/db/migrations/0002_*.sql` returns `0`). + - `apps/api/src/db/schema.ts` line for `rank` contains `utf8mb4_bin` (verify: `grep -c 'utf8mb4_bin' apps/api/src/db/schema.ts` returns `>= 1`). + - Live DB confirms the fix: a query of `information_schema.columns` for `list_items.rank` reports collation `utf8mb4_bin`. + - The full reorder describe block (including the pre-existing a0–a5 tests) still passes — no regression. + + Drag-to-top persists: an uppercase-prefixed rank now sorts above lowercase ranks in the DB, matching JS order. LIST-03 gap closed; migration is additive (generate+migrate, no push). + + + + Task 2: RED → GREEN — owner-only guard on PATCH isShared (T-04-08 / T-04-05) + apps/api/tests/routes/lists.test.ts, apps/api/src/routes/lists.ts + + - apps/api/src/routes/lists.ts:319-393 — PATCH `/:id` handler. `checkListAccess` (line 327) returns `{ allowed: true, isOwner: boolean, listRow }` for owner OR sharee. The `isShared` reconciliation block (lines 344-369) runs unconditionally for any allowed user. The DELETE handler at line 418 already uses `if (!access.isOwner)` as the exact guard idiom to mirror. + - apps/api/src/routes/lists.ts:121-152 — `checkListAccess` return shape; `isOwner` is the authoritative owner flag (true only when `listRow.ownerId === currentUserId`). + - apps/api/tests/routes/lists.test.ts:364-406, 438-450 — existing isShared toggle tests (all run as OWNER) and the "sharee can rename" test. There is NO test where a sharee toggles `isShared` — that path (WR-04) is uncovered; the existing 403-patch test (397-406) uses a non-sharee, caught earlier by `checkListAccess`. + - 04-SECURITY.md "Open Threat Detail" — the exact required guard and its placement (after the access check at lines 327-332, before the reconciliation). + + + RED — Add a negative test in the PATCH describe block of `lists.test.ts`. Title it for the threat (e.g. "T-04-08: sharee sending { isShared } gets 403 and list_shares is unchanged"). It must: + - seed an owner and a sharee, seed a SHARED list (`isShared: true`), `shareList(listId, shareeId)`; + - set `currentDevUserId = shareeId`; + - PATCH `/api/lists/:id` with `{ isShared: false }` and assert status `403`; + - assert the response body error mentions owner/sharing (the guard's message); + - assert `list_shares` for the list is UNCHANGED — the sharee row still exists (query `listShares` where `listId` and `userId = shareeId`, expect length `1`). This proves the destructive delete did not run. + Add a second assertion path (same or sibling test): a sharee sending `{ isShared: true }` on a private-but-shared scenario likewise gets `403` and inserts no new shares. Run before the fix and confirm it FAILS (currently 200 + shares wiped) — RED proof. + + Preserve the existing owner-path tests at lines 364-395: they must still pass (owner toggling isShared continues to work). + + GREEN — In `lists.ts`, immediately after the access check (the `if (!access.allowed)` block ending ~line 332) and BEFORE any update/reconciliation, add an owner-only guard: when `patch.isShared !== undefined && !access.isOwner`, return `c.json({ error: 'Only the list owner can change sharing settings' }, 403)`. This blocks both the `updateValues.isShared` write and the reconciliation block for non-owners. A sharee may still PATCH `{ name }` (the rename test at 438-450 must stay green). Update the stale inline comment at line 344 ("owner only affects shares") so it reflects the now-real guard rather than asserting a guard that didn't exist. + + + set -a; . ./apps/api/.env 2>/dev/null; set +a; export DB_HOST=127.0.0.1 DB_PORT=3306; pnpm --filter @familysync/api exec vitest run tests/routes/lists.test.ts -t "isShared" + + + - New test asserts a sharee PATCHing `{ isShared: false }` receives HTTP `403` AND the sharee's `list_shares` row still exists afterward (length `1`). + - New test asserts a sharee PATCHing `{ isShared: true }` receives `403` and no new shares are inserted. + - `apps/api/src/routes/lists.ts` PATCH handler contains a guard referencing `access.isOwner` and `patch.isShared` that returns 403 (verify: `grep -n "patch.isShared !== undefined && !access.isOwner" apps/api/src/routes/lists.ts` returns a match before line 342). + - Existing owner-path isShared toggle tests (false→true, true→false) and the sharee-rename test still pass. + - Full API suite green: `pnpm --filter @familysync/api exec vitest run` reports 0 failures. + + A non-owner sharee can no longer mutate list_shares via PATCH isShared; T-04-08 and T-04-05 are closed. The owner-only sharing-mutation invariant is enforced and regression-tested (WR-04 now covered). + + + + + +## Trust Boundaries + +| Boundary | Description | Data Crossing | +|----------|-------------|---------------| +| Browser → API (`PATCH /api/lists/:id`) | OIDC session cookie (Authelia) or dev-bypass; caller may be owner OR sharee | `{ name, isShared }` patch body | +| API → MariaDB | Drizzle parameterized queries (mysql2); `list_shares` mutated on visibility change | list_shares delete/insert rows | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-04-08 | Elevation of Privilege | `PATCH /api/lists/:id` isShared reconciliation (`lists.ts:344-369`) | mitigate | Owner-only guard after access check: `if (patch.isShared !== undefined && !access.isOwner) return 403`. A sharee can no longer delete/insert `list_shares`. Verified by negative test asserting 403 + unchanged shares. | +| T-04-05 | Elevation of Privilege | sharee performing owner-only sharing mutation via direct id | mitigate | Same owner-only guard closes the shared root cause; sharee retains read + name-edit + item-edit access (already gated/tested), but is blocked from the owner-only sharing mutation. | +| T-04-SC | Tampering | npm/pnpm installs during this plan | accept | This plan installs NO new packages (schema collation + route guard + tests only). No supply-chain surface added. | + + + +Phase-level checks after both tasks: + +1. **Full API suite (real DB):** `set -a; . ./apps/api/.env 2>/dev/null; set +a; export DB_HOST=127.0.0.1 DB_PORT=3306; pnpm --filter @familysync/api exec vitest run` → 0 failures (was 181 passing; now 183+ with two new cases). +2. **Migration is additive:** `grep -ciE 'drop|truncate' apps/api/src/db/migrations/0002_*.sql` → `0`. +3. **Collation applied in DB:** query `information_schema.columns` for `list_items.rank` → collation `utf8mb4_bin`. +4. **No push used:** confirm the change was applied via `db:migrate` (a new numbered SQL file exists in `apps/api/src/db/migrations/`), not `db:push`. +5. **Typecheck/build clean:** `pnpm --filter @familysync/api typecheck`. + + + +- LIST-03 drag-to-top persists across refetch (uppercase-prefixed rank sorts correctly) — verified by the collation regression test against the real DB. +- T-04-08 and T-04-05 closed: a non-owner sharee receives 403 on PATCH `{ isShared }` and `list_shares` is untouched — verified by the negative test. +- The rank column carries `COLLATE utf8mb4_bin` in both `schema.ts` and the live DB, applied via a non-destructive generate+migrate (no push, no DROP/TRUNCATE). +- All pre-existing Phase 4 tests still pass (181 prior API tests + new cases; no regression). +- 04-SECURITY.md can move to `threats_open: 0`; 04-VERIFICATION.md LIST-03 gap resolved. + + +## Artifacts this phase produces + +| Artifact | Type | Detail | +|----------|------|--------| +| `apps/api/src/db/migrations/0002_*.sql` (next sequential number) | NEW migration | `ALTER TABLE list_items` MODIFY `rank` to `COLLATE utf8mb4_bin`; additive, no DROP/TRUNCATE | +| `apps/api/src/db/schema.ts` — `listItems.rank` collation | EDIT | `varchar('rank', { length: 255 })` gains explicit `utf8mb4_bin` collation; length/notNull preserved | +| `apps/api/src/routes/lists.ts` — owner-only isShared guard | NEW guard | `if (patch.isShared !== undefined && !access.isOwner) return c.json({ error: 'Only the list owner can change sharing settings' }, 403)` after access check, before reconciliation | +| `lists.test.ts` — "collation regression" test (LIST-03) | NEW test | seeds `Zz` rank via drag-to-top PATCH; asserts GET returns it at index 0 | +| `lists.test.ts` — "T-04-08 sharee 403" test | NEW test | sharee PATCH `{ isShared }` → 403; `list_shares` unchanged (false→ and true→ paths) | + + +Create `.planning/phases/04-shared-lists-live-sync/04-07-SUMMARY.md` when done. +