docs(phase-04): add security threat verification (13/14 closed, T-04-08 open)
This commit is contained in:
Lucas Berger
@@ -0,0 +1,145 @@
|
||||
---
|
||||
phase: 4
|
||||
slug: shared-lists-live-sync
|
||||
status: issues_found
|
||||
threats_open: 1
|
||||
asvs_level: 1
|
||||
created: 2026-06-09
|
||||
---
|
||||
|
||||
# Phase 4 — Security
|
||||
|
||||
> Per-phase security contract: threat register, accepted risks, and audit trail.
|
||||
> Register authored at plan time (`register_authored_at_plan_time: true`); this audit
|
||||
> VERIFIES each declared mitigation against implemented code — it does not scan for new
|
||||
> threat classes.
|
||||
|
||||
---
|
||||
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description | Data Crossing |
|
||||
|----------|-------------|---------------|
|
||||
| Browser → API (`/api/*`) | OIDC session cookie (Authelia) or dev-bypass; all list/item routes gated | List names, item text, sharing state |
|
||||
| API → MariaDB | Drizzle parameterized queries (mysql2) | List/item/share rows |
|
||||
| API → SSE clients | `GET /api/sse/lists` per-list-channel fan-out, scoped by `getAccessibleListIds` | Minimal `{type, listId, payload}` event envelopes |
|
||||
| npm registry → build | New deps (react-router, dnd-kit, fractional-indexing) installed during phase | Third-party source |
|
||||
|
||||
---
|
||||
|
||||
## Threat Register
|
||||
|
||||
| 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-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-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-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 |
|
||||
| T-04-12 | Information Disclosure | over-broad SSE event payload | mitigate | payload is `{type, listId, payload:{id,...}}` minimal; per-channel scoped | closed |
|
||||
| T-04-SC | Tampering | npm supply chain (react-router, dnd-kit, fractional-indexing) | mitigate | RESEARCH legitimacy audit + blocking human checkpoint (04-01 Task 1) before install | closed |
|
||||
|
||||
*Status: open · closed*
|
||||
*Disposition: mitigate (implementation required) · accept (documented risk) · transfer (third-party)*
|
||||
|
||||
---
|
||||
|
||||
## Open Threat Detail
|
||||
|
||||
### T-04-08 (BLOCKER) — Sharee can rewrite list_shares via PATCH `isShared`
|
||||
|
||||
**File:** `apps/api/src/routes/lists.ts:319-393`
|
||||
|
||||
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`:
|
||||
|
||||
```ts
|
||||
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).
|
||||
|
||||
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.
|
||||
|
||||
---
|
||||
|
||||
## Audit Observations (non-blocking, from 04-REVIEW.md)
|
||||
|
||||
These are not declared threats in the register; recorded for traceability. They do not change
|
||||
any threat disposition under `block_on: high`.
|
||||
|
||||
- **CR-03 — `Number(c.req.param(...))` → NaN unguarded** (`lists.ts:323,407,459,521,569,663`).
|
||||
Invalid path params (`/api/lists/abc`) coerce to `WHERE id = NaN` (MariaDB → effectively 0)
|
||||
rather than returning 400. Behavior is benign-by-accident (empty/404 responses) and does not
|
||||
defeat any declared mitigation (access checks still run against a non-matching id), so it is
|
||||
not a BLOCKER here — but it is fragile input handling that should be hardened with an
|
||||
`Number.isInteger` guard. Does not open a new threat class.
|
||||
- **CR-02 — stale SSE subscription scope** (`sse.ts:85-121`): availability/UX gap (newly shared
|
||||
lists not delivered live until poll fallback), not a confidentiality leak — does not affect
|
||||
T-04-02 (scope is computed correctly, just not refreshed). Non-security.
|
||||
- **WR-01/WR-02 — async SSE listener + cleanup-after-loop** (`sse.ts:96-119`): listener-leak /
|
||||
unhandled-rejection robustness. Relevant to T-04-04 DoS posture but within the accepted
|
||||
`setMaxListeners(200)` envelope; not a register threat.
|
||||
- **WR-03 — `position` accepts any 1..255 string** (`lists.ts:113`): no fractional-indexing
|
||||
format validation. T-04-07 (overposting / field whitelist) is still satisfied — exactly-one-field
|
||||
refine holds. Malformed-rank robustness is an integrity hardening item, not the declared threat.
|
||||
|
||||
---
|
||||
|
||||
## Accepted Risks Log
|
||||
|
||||
| Risk ID | Threat Ref | Rationale | Accepted By | Date |
|
||||
|---------|------------|-----------|-------------|------|
|
||||
| AR-04-04 | T-04-04 | Single-process household app; `setMaxListeners(200)` (100 members × 2 devices) is generous headroom; Redis fan-out deferred (D-18) | Plan (04-02) | 2026-06-09 |
|
||||
| AR-04-10 | T-04-10 | VARCHAR(255) rank headroom; fractional-indexing degrades gracefully; rebalance via generateNKeysBetween available if ever needed (out of scope) | Plan (04-05) | 2026-06-09 |
|
||||
|
||||
*Accepted risks do not resurface in future audit runs.*
|
||||
|
||||
---
|
||||
|
||||
## Security Audit Trail
|
||||
|
||||
| Audit Date | Threats Total | Closed | Open | Run By |
|
||||
|------------|---------------|--------|------|--------|
|
||||
| 2026-06-09 | 14 | 13 | 1 | gsd-security-auditor |
|
||||
|
||||
---
|
||||
|
||||
## Sign-Off
|
||||
|
||||
- [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
|
||||
|
||||
**Approval:** pending — blocked on T-04-08 (CR-01) implementation fix + negative test
|
||||
Reference in New Issue
Block a user