familysync/.planning/milestones/v1.0-phases/04-shared-lists-live-sync/04-07-PLAN.md

---
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>