luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(04): re-verify phase 04 after gap-closure — 4/4 verified, 0 threats open

Browse Source 
LIST-03 collation gap closed, T-04-05 + T-04-08 closed by plan 04-07.
Phase 04 sign-off complete.
This commit is contained in:
Lucas Berger
2026-06-09 14:32:54 -04:00
parent ae115c65ef
commit fa71cf1a30
2 changed files with 99 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/04-shared-lists-live-sync/04-SECURITY.md
+24 -36
View File
@@ -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.