diff --git a/plans/260510-0232-fix-do-migration-followups/phase-01-do-storage-atomicity.md b/plans/260510-0232-fix-do-migration-followups/phase-01-do-storage-atomicity.md new file mode 100644 index 0000000..aaed33e --- /dev/null +++ b/plans/260510-0232-fix-do-migration-followups/phase-01-do-storage-atomicity.md @@ -0,0 +1,151 @@ +--- +phase: 1 +title: "DO storage atomicity & correctness" +status: pending +priority: P1 +effort: "4h" +dependencies: [] +--- + +# Phase 1: DO Storage Atomicity & Correctness + +## Overview +Fix the four correctness bugs in the Durable Object storage path: non-atomic multi-chunk writes, BLOB-grow data loss after canvas resize, cooldown burned on storage failure, and orphan-row crash on `/api/canvas`. Sanitize the 500 response body. Replace the bypassable `content-length` cap. + +## Context Links +- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (C3, H1, H2, H3, H5) +- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (C1, C2, M5) + +## Key Insights +- "No `await` between statements" ≠ atomic. Each `sql.exec` auto-commits unless wrapped in `state.storage.transactionSync(...)`. +- `new Uint8Array(typedArray)` copies but preserves the source's length, so growing a short BLOB requires explicit re-allocation against `chunkSize(chunkId)`. +- Cooldown UPDATE must roll back if the subsequent write throws — otherwise transient storage flakes silently halve image-uploader throughput. + +## Requirements + +**Functional** +- Multi-chunk batch writes atomic: all chunks commit, or none do. +- After `CANVAS_WIDTH`/`CANVAS_HEIGHT` grow + redeploy, new pixels in the formerly-last chunk persist correctly. +- Cooldown row reverted when `writePixels` throws. +- `GET /api/canvas` returns 200 even with orphan `chunk_id ≥ CHUNK_COUNT` rows present (post-shrink). +- `POST /api/place` rejects requests with missing or zero `content-length` (or reads body with hard byte cap). +- 500 responses do not echo raw error message. + +**Non-functional** +- No regression in happy-path latency (single-chunk batch). +- Static assertion `CHUNK_BYTES <= 2_000_000` near constants to prevent future BLOB-cell overflow. + +## Architecture + +``` +worker.js (edge) + ├─ guard: content-length present AND > 0 AND ≤ MAX_BODY_BYTES (else 413) + └─ forwards to DO + +CanvasRoom.placePixels (canvas-room.js) + ├─ tryAcquire(userId) → reserves cooldown + ├─ TRY: + │ state.storage.transactionSync(() => writePixels(sql, edits)) + │ broadcast(edits) + └─ CATCH: + sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId) // rollback + return Response.json({ error: 'storage_failed' }, { status: 500 }) + // no message field; full err logged separately +``` + +`chunk-storage.writePixels` rewritten so `next` is sized to `chunkSize(chunkId)`, with `buf.subarray(0, min(buf.length, expected))` copied into it. + +`chunk-storage.readAllChunks` filters `WHERE chunk_id < ?` bound to `CHUNK_COUNT`. + +## Related Code Files + +**Modify** +- `src/worker.js` — content-length validator (require > 0 OR switch to bounded `arrayBuffer`) +- `src/durable-objects/canvas-room.js` — wrap write in transactionSync, rollback cooldown on error, drop `message` from 500 +- `src/durable-objects/lib/chunk-storage.js` — fix `writePixels` BLOB-grow, bound `readAllChunks`, replace `new Uint8Array(buf)` aliasing comment with `.slice()` or explicit allocation +- `src/lib/constants.js` — add static assertion for `CHUNK_BYTES` +- `src/durable-objects/canvas-room.js:119` — fix the inverted "pre-2026-04-07" comment (M2 in code-review) + +**Create** — none + +**Delete** — none + +## Implementation Steps + +1. **`chunk-storage.writePixels` BLOB-grow fix** (review C3) + - In the per-chunk loop, compute `expected = chunkSize(chunkId)`. + - `const next = new Uint8Array(expected); next.set(buf.subarray(0, Math.min(buf.length, expected)));` + - Update inline comment: "size against chunkSize, never trust persisted blob length". + +2. **`chunk-storage.writePixels` atomicity** (debugger C2) + - Wrap the entire `for (const [chunkId, edits] of grouped)` block in `state.storage.transactionSync(() => { ... })`. + - Verify `transactionSync` exists at compat date `2025-04-01`; if not, fall back to async `state.storage.transaction(async () => {...})` and propagate rejection. + - Update the comment at L86-87 to describe the transactional invariant, not "no await". + +3. **`chunk-storage.readAllChunks` orphan guard** (review H3) + - Change SQL to `SELECT chunk_id, bytes FROM canvas_chunks WHERE chunk_id < ?` bound to `CHUNK_COUNT`. + - Optional secondary defense: skip rows where `chunkId * CHUNK_BYTES + view.length > out.length`. + +4. **`canvas-room.placePixels` cooldown rollback** (review H1, debugger C1) + - Add try/catch around the `writePixels` call. + - In catch: `sql.exec('DELETE FROM cooldowns WHERE user_id = ?', userId)`. Rethrow or return 500. + - Add a one-line comment: `// refund cooldown so transient storage errors don't soft-DOS user`. + +5. **500 response sanitization** (review H2) + - Drop `message: String(err)` from the response JSON. + - Keep the `console.error` call so debugging info stays in logs. + +6. **`worker.js` content-length guard** (review H5) + - Require `content-length` header present AND > 0 AND ≤ `MAX_BODY_BYTES`. Reject with 411 (length required) or 413 (too large). + - Alternative: read raw body via `c.req.arrayBuffer()`, check `byteLength`, then `JSON.parse(decoder.decode(buf))`. + +7. **Static `CHUNK_BYTES` cap assertion** (debugger M5) + - At top of `constants.js` (or in a small init): `if (CHUNK_BYTES > 2_000_000) throw new Error('CHUNK_BYTES exceeds DO SQLite cell limit');` + - Document compat-date-versioned cell limit in the comment. + +8. **WS-close comment correction** (review M2) + - `canvas-room.js:119` — replace misleading comment with: `// Required because compatibility_date 2025-04-01 predates the 2026-04-07 default-close cutoff. Remove if/when wrangler.json bumps past that date.` + - Add try/catch around the `ws.close(...)` call to handle already-closed sockets (debugger M7). + +9. **Compile + smoke** + - `npm run build` — must pass. + - Local `wrangler dev`, hit `POST /api/place` with a 2-pixel batch spanning 2 chunks; verify success. + - Force a write error (e.g. temporarily throw inside `writePixels`); verify cooldown row deleted, 500 returned without raw message. + - Hit `GET /api/canvas` after manually inserting an orphan row; verify 200. + +## Todo List + +- [ ] Implement BLOB-grow fix in `chunk-storage.writePixels` +- [ ] Wrap multi-chunk write in `transactionSync` (or async fallback) +- [ ] Bound `readAllChunks` query by `chunk_id < CHUNK_COUNT` +- [ ] Add cooldown rollback in `canvas-room.placePixels` catch path +- [ ] Drop `message` from 500 response +- [ ] Replace `content-length` guard with present+nonzero check or bounded body read +- [ ] Add `CHUNK_BYTES <= 2_000_000` static assertion +- [ ] Fix inverted WS-close comment + add try/catch +- [ ] `npm run build` passes +- [ ] Manual smoke: 2-chunk batch success +- [ ] Manual smoke: forced write error → cooldown refunded, no message leak +- [ ] Manual smoke: orphan row → `/api/canvas` 200 + +## Success Criteria + +- [ ] No regressions in existing 94-test suite +- [ ] `transactionSync` (or async transaction) confirmed available at compat date 2025-04-01 +- [ ] All bullets in Todo List checked +- [ ] PR includes file:line evidence linking each change to the originating finding + +## Risk Assessment + +- **Risk:** `transactionSync` not available at our compat date → fix back-compat. + **Mitigation:** Verify via `wrangler` runtime docs first; fall back to `state.storage.transaction(async () => {...})` and ensure `placePixels` is awaited end-to-end. +- **Risk:** Cooldown rollback DELETE race against concurrent insert from same user. + **Mitigation:** Single-DO is single-threaded per request; rollback runs synchronously before response. No lock needed. +- **Risk:** Bounded body read changes Hono request-handling shape; could break the existing tests. + **Mitigation:** Keep `c.req.json()` if guard is sufficient; only refactor to `arrayBuffer()` if guard alone leaves a gap. + +## Security Considerations + +- 500 sanitization closes a low-severity info-leak. +- content-length guard removes a DOS-amplification vector against the JSON parser. +- Atomicity fix prevents broadcast/persistence divergence that could be probed for state inference. diff --git a/plans/260510-0232-fix-do-migration-followups/phase-02-cookie-ip-identity.md b/plans/260510-0232-fix-do-migration-followups/phase-02-cookie-ip-identity.md new file mode 100644 index 0000000..2e1027f --- /dev/null +++ b/plans/260510-0232-fix-do-migration-followups/phase-02-cookie-ip-identity.md @@ -0,0 +1,153 @@ +--- +phase: 2 +title: "Cookie+IP identity & broadcast sequence numbers" +status: pending +priority: P1 +effort: "3h" +dependencies: [] +--- + +# Phase 2: Cookie+IP Identity & Broadcast Sequence Numbers + +## Overview +Replace IP-only rate-limit identity with cookie-preferred / IP-fallback to unblock NAT/CGNAT/mobile users. Add monotonically-increasing sequence numbers to WebSocket broadcast frames so clients can detect missed pixels and refetch. + +## Context Links +- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (C3 NAT, H2 dev bucket, H3 hibernation gap) +- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (M4 dev bucket) + +## Key Insights +- Cookie identity isn't a security boundary — it's a usability fix to break NAT collisions. Trivially defeated by clearing cookies; that's acceptable scope. +- Broadcast sequence numbers don't require persistence: a per-DO instance counter works because reconnects after hibernation refetch the canvas anyway. +- `cf-connecting-ip` missing in production is an alarm condition, not a fallback path. Fail-closed in prod, soft-fall in dev. + +## Requirements + +**Functional** +- First request without `rplace_id` cookie: server issues `Set-Cookie: rplace_id=; HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=31536000`. +- Subsequent requests with cookie: identity = `cookie:`. Without cookie but with valid `cf-connecting-ip`: identity = `ip:`. Neither in prod: 500 with `{ error: "no_identity" }`. Neither in dev: identity = `dev:`. +- WS broadcast frames carry monotonic `seq` field (uint32, wraps at 2^32). +- Client tracks last-seen `seq`; on gap (`seq != lastSeq + 1`) triggers `refetchCanvas()`. + +**Non-functional** +- Cookie issuance adds zero latency (Set-Cookie on existing 200 response). +- No new dependencies. + +## Architecture + +``` +src/lib/get-user-id.js + resolveIdentity(request, env): + if (cookie 'rplace_id' present) → "cookie:" + uuid + else if (cf-connecting-ip present) → "ip:" + sha256(ip).slice(0,16) + else if (env is production) → throw NoIdentityError → caller maps to 500 + else → "dev:" + + needsCookieIssue(request) → true if no rplace_id cookie + +src/worker.js + /api/canvas handler: + if needsCookieIssue → response.headers.append('Set-Cookie', issueCookie(uuid)) + +src/durable-objects/canvas-room.js + state #seq = 0 (in-memory, resets on hibernation rehydrate) + broadcast(edits): + #seq = (#seq + 1) >>> 0 + payload = { type: 'pixels', seq: #seq, edits } + +src/client/App.svelte + ws.onmessage: + if (msg.seq != null) { + if (lastSeq != null && msg.seq !== ((lastSeq + 1) >>> 0)) { + refetchCanvas() // gap detected + } + lastSeq = msg.seq + } +``` + +## Related Code Files + +**Modify** +- `src/lib/get-user-id.js` — cookie-first resolution + production fail-closed +- `src/worker.js` — issue cookie on `/api/canvas` when missing; pass identity into DO +- `src/durable-objects/canvas-room.js` — instance `#seq` counter; include in broadcast frames +- `src/client/App.svelte` — track `lastSeq`, refetch on gap +- `src/client/components/CanvasRenderer.svelte` — accept `seq`-aware events if needed (likely not) + +**Create** +- `src/lib/cookie.js` — minimal helpers: `parseCookie(header)`, `formatSetCookie(name, value, opts)`. ~30 lines, kebab-case per code standards. + +## Implementation Steps + +1. **Cookie helpers** (`src/lib/cookie.js`) + - `parseCookie(header) → Map` — handle missing header, malformed pairs. + - `formatSetCookie(name, value, { httpOnly, secure, sameSite, path, maxAge })` → string. + - Export both as named exports. + - Add tests in `test/lib/cookie.test.js` (Phase 4). + +2. **Identity resolution rewrite** (`get-user-id.js`) + - Import `parseCookie`. + - `resolveIdentity(request, env)` returns `{ id, issueCookie?: { name, value } }`. + - Production path: if no cookie AND no `cf-connecting-ip` → throw `NoIdentityError`. + - Dev path: stable `dev:` per-isolate (cache module-level). + - When identity comes from `ip:`, mark `issueCookie = { name: 'rplace_id', value: crypto.randomUUID() }` so the worker can attach Set-Cookie even before the user has one. + +3. **Worker integration** (`worker.js`) + - On `/api/canvas` and `/api/place`: call `resolveIdentity(c.req.raw, c.env)`. If it throws `NoIdentityError`, return 500 `{ error: "no_identity" }`. + - Pass `id` into the DO body / header. + - On `/api/canvas` only: if `issueCookie` is set, append `Set-Cookie` header to the response. + - On `/api/place`: don't issue cookies (POST shouldn't mutate cookies in this design). + +4. **DO sequence counter** (`canvas-room.js`) + - Add `#seq = 0` private field. + - In `#broadcastPixels(edits)`: `this.#seq = (this.#seq + 1) >>> 0; const message = JSON.stringify({ type: 'pixels', seq: this.#seq, edits });` + - Note: counter resets on DO hibernation rehydrate; that's fine — clients refetch on reconnect. + +5. **Client gap detection** (`App.svelte`) + - Track `let lastSeq = null;`. + - In WS message handler: if `msg.seq != null && lastSeq != null && msg.seq !== ((lastSeq + 1) >>> 0)` → call `canvasRenderer.refetchCanvas()`. Always update `lastSeq = msg.seq`. + - On reconnect: reset `lastSeq = null` (it's already a fresh canvas fetch). + +6. **Compile + smoke** + - `npm run build` passes. + - Cookie issuance: open `wrangler dev`, GET `/api/canvas`, confirm `Set-Cookie` present. Subsequent request shows cookie. + - Identity: 2 tabs, 1 cookie each → independent rate limits even on same IP. + - Sequence gap: temporarily log `seq` on client; place pixel, confirm seq increments. + +## Todo List + +- [ ] Create `src/lib/cookie.js` (parseCookie + formatSetCookie) +- [ ] Refactor `src/lib/get-user-id.js` to cookie-first / IP-fallback / fail-closed +- [ ] Worker issues `Set-Cookie` on `/api/canvas` when missing +- [ ] Worker forwards resolved identity to DO +- [ ] DO maintains `#seq` counter; broadcast includes `seq` +- [ ] Client tracks `lastSeq`, refetches on gap +- [ ] Client resets `lastSeq` on reconnect +- [ ] `npm run build` passes +- [ ] Manual smoke: cookie issued, identity changes per cookie +- [ ] Manual smoke: seq gap → client refetches + +## Success Criteria + +- [ ] Two browsers on the same IP can place pixels independently within their own 1Hz cooldown. +- [ ] In production env (`env.ENVIRONMENT === 'production'`), missing both cookie and `cf-connecting-ip` returns 500 with `no_identity`. +- [ ] Forced WS gap (drop a frame in dev console) triggers `refetchCanvas`. +- [ ] No regression in existing tests. + +## Risk Assessment + +- **Risk:** Browser cookie blocking (private mode, strict tracker prevention) → falls back to IP, NAT users still collide. + **Mitigation:** Acceptable per Q&A scope; document. Future plan can add HMAC-signed token for stricter envs. +- **Risk:** Cookie collision if two users share a stolen cookie value (e.g., copy-paste curl). + **Mitigation:** Cookies are HttpOnly — not exfiltrable from JS. The opaque UUID is high-entropy. +- **Risk:** Seq counter wraparound after 2^32 broadcasts. + **Mitigation:** ~136 years at 1 broadcast/sec. Wrap is deliberately handled via `>>> 0` so wrap doesn't trigger false gap. +- **Risk:** Production fail-closed behind a misconfigured proxy denies all traffic. + **Mitigation:** Add a `wrangler tail` check before deploy; if 500s spike, hotfix to fall back to a synthetic identity. + +## Security Considerations + +- Cookie is `HttpOnly`, `Secure`, `SameSite=Lax` — standard hygiene. +- Cookie is opaque (UUID v4) — no PII, no prediction. +- Identity is used only for rate-limiting; no privilege gating, so cookie theft doesn't escalate. +- No CSRF concern — POST `/api/place` doesn't depend on cookie identity for authorization, only for rate-limit bucketing. (Worth confirming in PR review.) diff --git a/plans/260510-0232-fix-do-migration-followups/phase-03-ws-hardening-client-race.md b/plans/260510-0232-fix-do-migration-followups/phase-03-ws-hardening-client-race.md new file mode 100644 index 0000000..5b870fe --- /dev/null +++ b/plans/260510-0232-fix-do-migration-followups/phase-03-ws-hardening-client-race.md @@ -0,0 +1,153 @@ +--- +phase: 3 +title: "WebSocket hardening & client race fix" +status: pending +priority: P1 +effort: "3h" +dependencies: [] +--- + +# Phase 3: WebSocket Hardening & Client Race Fix + +## Overview +Fix the WebSocket-during-initial-fetch race that silently drops pixels. Add Origin allowlist + per-identity connection cap on WS upgrade. Add a minimal heartbeat so dead connections fire `onclose` promptly. + +## Context Links +- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (C2 race, H4 conn cap, M3 heartbeat) +- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (H5 origin/amplification, L6 conn cap, M3 heartbeat — duplicate) + +## Key Insights +- The pre-allocated `committedColors` zero array gets overwritten by the post-fetch `new Uint8Array(indices)` — any WS edits between WS-open and fetch-resolve are lost. Fix: buffer WS edits, replay after replacement. +- CF DO `state.acceptWebSocket(ws, [tag1, tag2])` lets `getWebSockets(tag)` filter — perfect for per-identity caps. +- Hibernation API may auto-ping at TCP level, but app-level heartbeat is cheaper insurance and gives clients a way to detect zombies. + +## Requirements + +**Functional** +- WS messages received during the initial canvas fetch are applied (not dropped) once the fetch resolves. +- WS upgrade rejected (403) if `Origin` header is present and not in allowlist. +- WS upgrade rejected (429) if the requesting identity already has ≥ N (default 5) live sockets. +- Server accepts `ping` text message and responds `pong`. Client sends ping every 30s; if no pong in 60s, closes WS to trigger reconnect. + +**Non-functional** +- Allowlist configurable via `wrangler.json` env vars (`ALLOWED_ORIGINS`, comma-separated). +- Conn cap configurable (`MAX_WS_PER_IDENTITY`, default 5). + +## Architecture + +``` +Worker /api/ws + ├─ resolve identity (Phase 2 helper) + ├─ origin check: if Origin present AND not in env.ALLOWED_ORIGINS → 403 + └─ forward to DO with identity in header + +CanvasRoom #handleWsUpgrade + ├─ existing = state.getWebSockets(identity) + ├─ if existing.length >= MAX_WS_PER_IDENTITY → 429 + ├─ state.acceptWebSocket(server, [identity]) + └─ return 101 + +CanvasRoom webSocketMessage(ws, msg) + ├─ if msg === 'ping' → ws.send('pong'); return + └─ else → ws.close(1003, 'unsupported message') + +Client (CanvasRenderer.svelte loadCanvas) + let pendingWsEdits = []; + ws.onmessage during fetch → push to pendingWsEdits (don't apply) + fetch resolves: + committedColors = new Uint8Array(indices); + apply pendingWsEdits to committedColors + pendingWsEdits = null + flag "live mode" — onmessage now applies directly + +Client (App.svelte) + setInterval(() => ws.send('ping'), 30_000) + trackPongTimer; if no pong in 60s → ws.close() +``` + +## Related Code Files + +**Modify** +- `src/worker.js` — origin check before WS upgrade forwarding +- `src/durable-objects/canvas-room.js` — per-identity conn cap, ping handling, tag-aware acceptWebSocket +- `src/client/components/CanvasRenderer.svelte` — buffer-and-replay during initial fetch (around lines 465–484) +- `src/client/App.svelte` — ping interval, pong watchdog +- `src/lib/constants.js` — add `MAX_WS_PER_IDENTITY = 5` +- `wrangler.json` — add `vars: { ALLOWED_ORIGINS: "https://rplace.miti99.workers.dev" }` + +**Create** — none + +## Implementation Steps + +1. **Origin allowlist in worker** (debugger H5) + - Read `env.ALLOWED_ORIGINS` (comma-separated). Parse to Set at module top. + - In `/api/ws` handler: if `Origin` header present and not in allowlist, return `c.text('forbidden_origin', 403)`. Empty allowlist → allow all (dev default). + - Document in `wrangler.json` comment. + +2. **Per-identity WS cap in DO** (review H4, debugger L6) + - In `#handleWsUpgrade(request, identity)`: `const existing = this.state.getWebSockets(identity);` + - If `existing.length >= MAX_WS_PER_IDENTITY` → return `new Response('too_many_sockets', { status: 429 })`. + - Replace `state.acceptWebSocket(server)` with `state.acceptWebSocket(server, [identity])`. + +3. **Server-side heartbeat** (review M3, debugger M3) + - In `webSocketMessage(ws, message)`: if `message === 'ping'` → `ws.send('pong'); return;`. Else keep current close behavior. + - Note: this works under hibernation because messages auto-rehydrate the DO. + +4. **Client buffer-and-replay** (review C2) + - In `loadCanvas` (`CanvasRenderer.svelte:465-484`): + - Add `let pendingWsEdits = [];` and `let isLive = false;` at top of `loadCanvas`. + - Expose `pushWsEdit(edit)` from the component: if `!isLive` → `pendingWsEdits.push(edit)`; else apply directly. + - After fetch resolves and `committedColors = new Uint8Array(indices)`, replay: for each `edit` in `pendingWsEdits`, write to `committedColors[edit.idx] = edit.color` AND update `imageData`. Then `isLive = true; pendingWsEdits = null;`. + - In parent (`App.svelte`), route ws.onmessage pixel events to `canvasRenderer.pushWsEdit(...)` instead of applying directly when first connect. + +5. **Client heartbeat** (review M3) + - In `App.svelte` WS open handler: start `setInterval(() => ws.readyState === 1 && ws.send('ping'), 30_000)`. Track `lastPongAt = Date.now()`. + - On message `'pong'`: `lastPongAt = Date.now()`. + - Watchdog: if `Date.now() - lastPongAt > 60_000` → `ws.close()` to trigger reconnect logic. + - Clear interval/watchdog on `onclose`. + +6. **Compile + smoke** + - `npm run build` passes. + - Open dev console, throttle network to "Slow 3G", reload, place pixel from a second tab during fetch, confirm pixel appears in tab 1 once fetch completes. + - Try opening 6 WS connections from same browser → 6th gets 429. + - Try opening WS from a different origin (curl with `Origin: https://evil.example`) → 403. + - Confirm `ping`/`pong` round-trips in dev console. + +## Todo List + +- [ ] Origin allowlist parsing + worker check +- [ ] Per-identity WS cap (`MAX_WS_PER_IDENTITY`) with `acceptWebSocket(server, [identity])` +- [ ] Server `ping` handler returns `pong` +- [ ] `wrangler.json` `vars.ALLOWED_ORIGINS` +- [ ] Client buffer-and-replay for WS during initial fetch +- [ ] Client 30s ping / 60s pong watchdog +- [ ] `npm run build` passes +- [ ] Manual smoke: race-fix verified by slow network reload + remote pixel placement +- [ ] Manual smoke: 6th WS rejected with 429 +- [ ] Manual smoke: foreign-origin WS rejected with 403 +- [ ] Manual smoke: ping/pong visible in dev tools + +## Success Criteria + +- [ ] No pixel placed during the initial-fetch window is dropped (verified via instrumented log). +- [ ] WS upgrade from disallowed origin returns 403 in production. +- [ ] Per-identity cap enforced; logs show `too_many_sockets` when triggered. +- [ ] `onclose` fires within ~60s of network drop (verified via airplane-mode toggle). +- [ ] No regression in 94-test suite. + +## Risk Assessment + +- **Risk:** Origin allowlist set too tight → legitimate clients (preview deployments, custom domains) get 403. + **Mitigation:** Empty `ALLOWED_ORIGINS` allows all — start with empty in dev/preview, populate before production deploy. +- **Risk:** Cap on identity blocks tab-power-users (5 tabs is normal for some folks). + **Mitigation:** Cap is configurable; bump to 10 if support tickets appear. +- **Risk:** Heartbeat interval too aggressive → battery drain on mobile. + **Mitigation:** 30s ping is well below the typical mobile-radio-wakeup penalty; keeping interval >= 25s avoids extra wakes. +- **Risk:** Buffer-and-replay logic interacts oddly with the existing `imageData` invalidation in CanvasRenderer. + **Mitigation:** Replay loop must call the same path the live message handler does (write to both `committedColors` AND `imageData`); add a unit test in Phase 4. + +## Security Considerations + +- Origin check is a usability/cost barrier, not a security one — WS protocol allows non-browser clients to spoof Origin. Real defense is the per-identity cap + Worker request budget. +- Per-identity cap prevents broadcast amplification (debugger H5, L6). +- Heartbeat surface is a single text-equality check; no parser exposure. diff --git a/plans/260510-0232-fix-do-migration-followups/phase-04-do-surface-tests.md b/plans/260510-0232-fix-do-migration-followups/phase-04-do-surface-tests.md new file mode 100644 index 0000000..7fa2f87 --- /dev/null +++ b/plans/260510-0232-fix-do-migration-followups/phase-04-do-surface-tests.md @@ -0,0 +1,161 @@ +--- +phase: 4 +title: "Full DO surface test coverage" +status: pending +priority: P2 +effort: "6h" +dependencies: [1, 2, 3] +--- + +# Phase 4: Full DO Surface Test Coverage + +## Overview +Close the test gap: today only `test/worker-validation.test.js` exists. The DO surface (chunk-storage, cooldown-store, WS hub, identity resolution) has zero unit tests. Add a vitest harness that runs against `wrangler unstable_dev` so DO state is real, not mocked. + +## Context Links +- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (positive observations re tests; gap implicit) +- Reports: `plans/reports/debugger-260510-0211-rplace-edge-cases.md` (L7 — explicit DO test gap) + +## Key Insights +- Mocked DO unit tests (current style) won't catch the bugs Phase 1–3 fix. Need real DO state. +- `wrangler unstable_dev` lets vitest hit a live local Worker + DO via fetch. Heavier but accurate. +- For pure-function modules (`pixel-buffer`, `chunk-storage` math), keep classic vitest unit tests — fast loop. +- The 200-line file rule applies to test files too; split per concern (one test file per source file). + +## Requirements + +**Functional** +- Tests cover: write atomicity, BLOB-grow, orphan-row read, cooldown refund, cooldown TTL, WS broadcast frame format with `seq`, WS per-identity cap, ping/pong, identity cookie/IP/dev fallback, gap detection. +- All tests pass on `npm test`. + +**Non-functional** +- Total test runtime under 30s on a dev machine. +- No hard-coded sleeps > 100ms; use polling helpers where needed. +- Tests are deterministic — no flakes when run 10× consecutively. + +## Architecture + +``` +test/ +├── lib/ +│ ├── cookie.test.js # NEW — parseCookie/formatSetCookie +│ ├── get-user-id.test.js # EXTEND — cookie/ip/dev fallback paths +│ ├── pixel-buffer.test.js # EXISTING — keep +│ └── ... (existing image-* tests stay) +├── durable-objects/ +│ ├── chunk-storage.test.js # NEW — pure-function unit tests via mocked sql +│ ├── cooldown-store.test.js # NEW — TTL math, GC sample, race +│ └── canvas-room.integration.test.js # NEW — wrangler unstable_dev +├── worker-validation.test.js # EXISTING — keep +└── helpers/ + └── do-harness.js # NEW — boot wrangler unstable_dev once per file +``` + +`do-harness.js` exports `setupDO()` returning `{ worker, fetch, close }`. Suite uses `beforeAll` / `afterAll` to share the harness. + +## Related Code Files + +**Create** +- `test/helpers/do-harness.js` +- `test/lib/cookie.test.js` +- `test/durable-objects/chunk-storage.test.js` +- `test/durable-objects/cooldown-store.test.js` +- `test/durable-objects/canvas-room.integration.test.js` + +**Modify** +- `test/lib/get-user-id.test.js` — extend for new resolution logic +- `vitest.config.js` — add `testTimeout: 30_000`, include `test/durable-objects/**/*` +- `package.json` — no new deps; `wrangler` is already a devDependency + +**Delete** — none + +## Implementation Steps + +### Pure-function tests + +1. **`test/lib/cookie.test.js`** + - `parseCookie('a=1; b=2')` → Map `{a:'1', b:'2'}` + - `parseCookie('')` → empty Map + - `parseCookie(undefined)` → empty Map + - Malformed: `parseCookie('a; b=2')` → Map `{b:'2'}` + - `formatSetCookie('rplace_id', 'uuid', { httpOnly: true, secure: true, sameSite: 'Lax', maxAge: 31536000, path: '/' })` → exact string match. + +2. **`test/lib/get-user-id.test.js`** (extend) + - Cookie present → `cookie:` + - No cookie, IP present → `ip:` + - Production env, neither → throws `NoIdentityError` + - Dev env, neither → `dev:*` + - Cookie + IP both present → cookie wins + - Cookie issuance flag set when caller should `Set-Cookie` + +3. **`test/durable-objects/chunk-storage.test.js`** (mock sql) + - Build a fake `sql` with an in-memory `Map` backing `INSERT OR REPLACE` and `SELECT`. + - `writePixels` single chunk: write 3 pixels, read back identical bytes. + - `writePixels` 2-chunk batch: pixels at offset 0 and offset 65535+1, both persist. + - `writePixels` BLOB-grow: pre-seed last chunk with a short blob (8KB), grow via fake `chunkSize` returning 64KB, write a pixel at byte 30000, read back exactly 64KB blob with byte 30000 set. + - `readAllChunks` orphan: pre-seed `chunk_id = 999`, `CHUNK_COUNT = 256`, read returns valid `Uint8Array(TOTAL_PIXELS)` without throwing. + - `chunkSize(255)` for current dims = 65536; `chunkSize(0..254)` = 65536. + - Test the bound query: orphan row not selected. + +4. **`test/durable-objects/cooldown-store.test.js`** (mock sql) + - `tryAcquire(userId, now)` first call returns `{ ok: true }`. + - Second call within 1s returns `{ ok: false, retryAfter: }`. + - Second call after 1s returns `{ ok: true }`. + - GC sample with deterministic RNG: pre-seed expired rows, run `tryAcquire` with mocked `Math.random()` returning 0 (always GC) → expired rows deleted. + - INSERT cursor symmetry: ensure both branches drain (after fix in Phase 1's M1). + +### Integration tests (real DO) + +5. **`test/helpers/do-harness.js`** + - `import { unstable_dev } from 'wrangler'` + - `export async function setupDO() { const worker = await unstable_dev('src/worker.js', { config: 'wrangler.json', experimental: { disableExperimentalWarning: true } }); return { worker, fetch: worker.fetch.bind(worker), close: () => worker.stop() }; }` + - Add helper `placePixel(worker, { x, y, color, cookie? })` returning `{ status, body, setCookie }`. + +6. **`test/durable-objects/canvas-room.integration.test.js`** + - **Cookie issuance:** GET `/api/canvas` without cookie → response has `Set-Cookie: rplace_id=...`. Subsequent GET with cookie → no new Set-Cookie. + - **Cooldown isolation:** two cookies, same simulated IP → both place pixels in the same second. + - **Cooldown refund on error:** force a write error path (test-only env flag throws inside `writePixels`) → cooldown row deleted, second attempt succeeds immediately. + - **Multi-chunk atomicity:** force-error mid-batch on second chunk → `GET /api/canvas` shows none of the batch applied, broadcast not fired. + - **Orphan row:** seed orphan via DO debug endpoint (test-only) → `GET /api/canvas` returns 200. + - **content-length=0 rejected:** POST `/api/place` with `Content-Length: 0` → 411 or 413. + - **WS broadcast carries seq:** open WS, place a pixel, receive frame with `{ type:'pixels', seq:1, edits:[...] }`. Place another → `seq:2`. + - **WS per-identity cap:** open 5 WS with same cookie → all accepted. 6th → 429 / connection-close. + - **WS ping/pong:** send `'ping'`, receive `'pong'`. + - **WS gap → client refetch:** simulate dropped frame by manually skipping a `seq`; client-side gap detection is a unit-test concern (covered in `test/lib/seq-gap.test.js` if extracted; otherwise verify protocol shape only). + - **Origin allowlist:** WS upgrade with `Origin: https://evil.example` → 403; with no Origin → allowed. + +## Todo List + +- [ ] Decide test-only env flag mechanism (e.g., `env.TEST_FORCE_WRITE_ERROR === '1'`) and add gated branches +- [ ] Write `test/helpers/do-harness.js` +- [ ] Write `test/lib/cookie.test.js` +- [ ] Extend `test/lib/get-user-id.test.js` +- [ ] Write `test/durable-objects/chunk-storage.test.js` +- [ ] Write `test/durable-objects/cooldown-store.test.js` +- [ ] Write `test/durable-objects/canvas-room.integration.test.js` +- [ ] Update `vitest.config.js` (timeout + include patterns) +- [ ] `npm test` — all green +- [ ] Run 10× back-to-back; no flakes + +## Success Criteria + +- [ ] All Phase 1–3 fixes have at least one test that fails on the pre-fix code and passes after. +- [ ] Coverage report shows DO source files at ≥ 80% line coverage. +- [ ] Total test runtime under 30s on dev hardware. +- [ ] CI (if configured) passes; otherwise local 10× green. + +## Risk Assessment + +- **Risk:** `wrangler unstable_dev` is not stable across versions; tests may break on upgrade. + **Mitigation:** Pin `wrangler` version in `package.json` (already pinned to `^4.14.1`); update tests when intentionally bumping. +- **Risk:** Test-only env flags leak into production. + **Mitigation:** Gate behind `env.NODE_ENV === 'test'` AND require an explicit `env.TEST_HOOKS === 'enabled'` second flag. Document; reject in deploy script. +- **Risk:** Integration tests slow down dev loop, devs skip them. + **Mitigation:** Add `npm run test:unit` (excludes `**/integration.*`) for fast inner loop. +- **Risk:** WS testing in vitest is awkward (`unstable_dev` returns Worker, not Server). + **Mitigation:** Use `worker.fetch` for upgrade and treat the returned `WebSocket` directly. If tooling doesn't allow that, fall back to spawning `wrangler dev` in a child process. + +## Security Considerations + +- Test-only hooks must never ship to production. Add a build-time assertion (e.g., a fail-fast check at worker startup if `TEST_HOOKS` is enabled in production env). +- Tests must not commit any secrets — use `crypto.randomUUID()` for ephemeral test cookies. diff --git a/plans/260510-0232-fix-do-migration-followups/phase-05-docs-cleanup.md b/plans/260510-0232-fix-do-migration-followups/phase-05-docs-cleanup.md new file mode 100644 index 0000000..d17fae9 --- /dev/null +++ b/plans/260510-0232-fix-do-migration-followups/phase-05-docs-cleanup.md @@ -0,0 +1,121 @@ +--- +phase: 5 +title: "Docs cleanup & legacy plan archival" +status: pending +priority: P2 +effort: "30m" +dependencies: [] +--- + +# Phase 5: Docs Cleanup & Legacy Plan Archival + +## Overview +Strike stale Upstash references from `README.md` and `.env.example`. Verify the `docs/` directory matches current code (the docs-manager and code-reviewer reports disagree — verify file by file). Mark the migration plan as `completed` after the 7-day rollback observation window. + +## Context Links +- Reports: `plans/reports/docs-manager-260510-0211-rplace-docs-drift.md` (primary) +- Reports: `plans/reports/code-reviewer-260510-0211-rplace-do-migration.md` (C1 — claims docs/*.md still mention Upstash; conflicts with docs-manager finding) + +## Key Insights +- Docs-manager said `docs/system-architecture.md`, `deployment-guide.md`, `code-standards.md` are accurate. Code-reviewer C1 said they still reference Upstash. Verify directly before deciding. +- Migration endpoint already removed from worker; documenting it as "transitional" misleads onboarding. +- `.env.example` placeholders for deleted services cause new-developer confusion. + +## Requirements + +**Functional** +- README's Project Structure block lists only files that exist in `src/`. +- README does not advertise `/admin/migrate-from-upstash` as transitional. +- `.env.example` either deleted or replaced with a comment explaining no external secrets. +- All `docs/*.md` files verified accurate; any Upstash references that describe current state (not history) removed. +- `plans/260509-2309-canvas-on-do-storage/plan.md` frontmatter `status: completed` (after 2026-05-17, the rollback window cutoff). + +**Non-functional** +- Edits target only the lines in question; no unrelated reformatting. +- Git history preserves clear "docs:" commit type per `.claude` convention (drop `docs:` from `.claude/` paths only — not relevant here). + +## Architecture +N/A — text edits. + +## Related Code Files + +**Modify** +- `README.md` — Project Structure tree (lines 79–111), API section (lines 146–151) +- `.env.example` — content +- `docs/system-architecture.md` — only if verification finds Upstash-as-current text +- `docs/deployment-guide.md` — only if verification finds Upstash-as-current text +- `docs/code-standards.md` — only if verification finds Upstash-as-current text +- `plans/260509-2309-canvas-on-do-storage/plan.md` — frontmatter `status` field (after rollback window) + +**Create** — none + +**Delete** — possibly `.env.example` (Option A in docs-manager report) + +## Implementation Steps + +1. **Verify docs/ accuracy** + - `grep -ni 'upstash\|redis\|@upstash' docs/*.md` + - For each match: read context, classify as (a) historical mention OK, (b) presented as current — needs edit. + - Decide whether to keep migration narrative as historical or scrub entirely. + +2. **README.md Project Structure** (docs-manager STALE finding 1) + - Replace lines 79–111 with the corrected tree from docs-manager report (no `src/admin/`, no legacy lib files; include image pipeline files). + - Use the actual `find src -type f` output as ground truth. + +3. **README.md API Section** (docs-manager MISLEADING finding 2) + - Delete the `### POST /admin/migrate-from-upstash (transitional)` block (lines 146–151). + - Verify no other doc references this section. + +4. **.env.example** (docs-manager STALE finding 3) + - Choose Option B (keep as documentation): replace contents with a single comment block: + ``` + # No external secrets required. + # Canvas + cooldown state live inside CanvasRoom Durable Object (SQLite). + # All configuration is in src/lib/constants.js. + ``` + - Rationale: discoverability. New devs find `.env.example` and learn there's nothing to set. + +5. **docs/ targeted edits** (only if step 1 found drift) + - Edit each flagged section. Keep migration mentioned in `deployment-guide.md` "Optional One-Shot Migration from Upstash" as historical — do not delete the narrative if it's already framed as past. + +6. **Migration plan archival** (docs-manager finding 4) + - Today is 2026-05-10; rollback window per migration-plan ends ~2026-05-17. + - Add a TODO note in this phase saying "after 2026-05-17, change `plans/260509-2309-canvas-on-do-storage/plan.md` frontmatter `status` to `completed`". + - For now (before 2026-05-17), leave `status: in-progress` BUT add `deployment.cleanupAt: 2026-05-10` (already present per existing frontmatter) and verify it's accurate. + +7. **Sanity sweep** + - `grep -rni 'upstash\|@upstash\|redis-client\|rate-limiter\.js\|migrate-from-upstash' README.md docs/ .env.example src/` — should return only intentional historical refs. + - `npm run build` — must still pass (sanity check that a doc edit didn't break a `