docs(04): gap-closure plan 04-07 (rank collation + isShared owner guard)

2026-06-09 14:07:14 -04:00
parent 469c40f9b5
commit 797338424d
2 changed files with 212 additions and 1 deletions
@@ -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
@@ -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"
+---
+
+<objective>
+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.
+</objective>
+
+<execution_context>
+@$HOME/.claude/gsd-core/workflows/execute-plan.md
+@$HOME/.claude/gsd-core/templates/summary.md
+</execution_context>
+
+<context>
+@.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
+</context>
+
+<hard_constraints>
+- **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`).
+- `<action>` blocks below name identifiers and behavior only — no fenced code blocks / full implementations.
+</hard_constraints>
+
+<tasks>
+
+<task type="tdd" tdd="true">
+  <name>Task 1: RED → GREEN — rank-collation drag-to-top regression (LIST-03)</name>
+  <files>apps/api/tests/routes/lists.test.ts, apps/api/src/db/schema.ts, apps/api/src/db/migrations/</files>
+  <read_first>
+    - 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`.
+  </read_first>
+  <action>
+    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.
+  </action>
+  <verify>
+    <automated>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"</automated>
+  </verify>
+  <acceptance_criteria>
+    - 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.
+  </acceptance_criteria>
+  <done>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).</done>
+</task>
+
+<task type="tdd" tdd="true">
+  <name>Task 2: RED → GREEN — owner-only guard on PATCH isShared (T-04-08 / T-04-05)</name>
+  <files>apps/api/tests/routes/lists.test.ts, apps/api/src/routes/lists.ts</files>
+  <read_first>
+    - 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).
+  </read_first>
+  <action>
+    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.
+  </action>
+  <verify>
+    <automated>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"</automated>
+  </verify>
+  <acceptance_criteria>
+    - 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.
+  </acceptance_criteria>
+  <done>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).</done>
+</task>
+
+</tasks>
+
+<threat_model>
+## 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. |
+</threat_model>
+
+<verification>
+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`.
+</verification>
+
+<success_criteria>
+- 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.
+</success_criteria>
+
+## 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) |
+
+<output>
+Create `.planning/phases/04-shared-lists-live-sync/04-07-SUMMARY.md` when done.
+</output>