[REVIEW] P3 backlog — 29 latent/hardening findings (tracking)

[serenakeyitan]

Severity: P3 (latent / low-reachability / hardening)

Batched tracking issue for 29 confirmed P3 findings from the 2026-06 engineering review (docs/ENGINEERING-REVIEW-2026-06.md). Each is real but low-impact.

---

✅ Resolved (verified in current code as of v0.7.2)

The security-relevant subset is done — live XSS test (4 attack vectors: comment text, comment id, author login, avatar_url) all neutralized in a real browser; corrupt-KV resilience covered by test/p3-hardening.test.js (14 assertions).

• identity.avatar_url XSS — now escapeHtml(identity.avatar_url || '') (overlay.js:763). Same for the identity chip.
• avatar_url in renderAuthor — escapeHtml(author.avatar_url) (overlay.js:1142).
• indexHtml / catalog escaping — title/slug/login now go through escapeHtml (worker.js:671/672/692), not bare replace(/</).
• JSON.parse(KV) uncaught 500s — all comment reads now go through safeParseList() (try/catch → []); a corrupt value self-heals instead of bricking the slug.
• non-array parse crashes ensureMigrated — safeParseList returns [] for non-array JSON.
• Debug console.log in auth/upload paths — converted to gated debug(env, ...); raw production logging removed.
• Inconsistent HTML escaping (index vs overlay) — index/catalog now use the same escapeHtml.

🔄 Superseded by #34 (Durable Object migration)

Comments now live in input-gated DO state.storage (not KV), so the KV-race-adjacent items are structurally changed:

• Reaction toggle non-idempotent under retry/double-click — reaction RMW now serialized through the DO transaction(); concurrent toggles no longer interleave.
• Mutating endpoints' RMW lost-update risk — eliminated by [REVIEW] Migrate comment writes to a Durable Object for true per-slug serialization #34 (25-way concurrency test: 0 lost).

☐ Still open (genuinely cosmetic / low-priority)

• Reaction POSTs ignore response status, then unconditionally refresh — overlay.js (no r.ok/401 handling on reaction paths)
• tdoc-doctor dead code from abandoned rewrite — bin/tdoc-doctor
• Reaction-cleanup + reply-shape duplicated in server.js
• Dead duplicate CSS + article-ratio fallback copy-pasted 3-4× — overlay.js
• comments.json two-schema split (local flat vs worker event-log) — latent; no local→worker re-import path
• GitHub OAuth token stored 30d with no consumer — store only {login, avatar_url, name}
• Soft-delete magic sentinel 999999 vs Infinity — still at worker.js:1718 (dead path; overlay always sends finite version)
• Local server IDs c_${Date.now()} no entropy — single-user localhost, negligible
• mtime-gated worker redeploy (not content hash) — bin/tdoc-publish
• Inconsistent 401/failure handling across 6 overlay mutation fetch sites
• Non-constant-time !== on TDOC_UPLOAD_TOKEN — worker.js:1102 (256-bit secret over RTT; not practically exploitable)
• Dead status==='resolved' filter — model never emits 'resolved' (worker.js + overlay.js)
• Device-flow start lacks r.ok/network handling
• published.json fields read with no validation — bin/tdoc-publish
• Onboarding 'everything missing' scenario asserts almost nothing — test/onboarding.test.js
• Mutations don't verify target alive at requested version — unreachable via UI (overlay sends current version)
• aid content-hash collision for duplicate artifacts — muted (identical artifacts render identically)
• Device-poll proxies device_code with no rate limit
• anchor.aid into querySelector without CSS.escape — overlay.js:1065/2217 (unguarded throw → render-loop abort; authenticated-commenter only)
• Pull merge can resurrect a worker-deleted comment — tdoc-pull (local-only impact)

---

Updated 2026-06 after v0.6/v0.7 hardening + #34 DO migration. Remaining items are cosmetic/maintainability or negligible-reachability; pick off as backlog.

[serenakeyitan]

P3 hardening batch landed in v0.7.2 (commit 29904c5, Codex-reviewed 3 rounds, live-deployed).

Fixed:

• ✅ avatar_url DOM-XSS sink (identity chip) — escaped
• ✅ Catalog/index escaping (worker /me + local server) — full escapeHtml, slug encodeURIComponent; was <-only
• ✅ Comment-card attribute injection (comment/reply/target ids) — escaped; agent_status whitelisted
• ✅ Corrupt/non-array stored comments value 500ing a slug forever (+ ensureMigrated non-array crash) — safeParseList() + DO/server non-array self-heal at all read sites
• ✅ Duplicate CSS rule removed

Already fixed by earlier work (verified):

• ✅ Reaction-toggle non-idempotency → [REVIEW] Migrate comment writes to a Durable Object for true per-slug serialization #34 (toggle moved into applyCommentOp, DO-serialized)
• ✅ tdoc-doctor dead code → already removed

Left intentionally (cosmetic / negligible — rationale):

• 999999 sentinel vs Infinity (unreachable: version machine-generated from 1)
• status==='resolved' dead filter (harmless no-op)
• non-constant-time TDOC_UPLOAD_TOKEN compare (256-bit secret over internet RTT)
• mtime-gated redeploy; local-server c_${Date.now()} ID collision (single-user); debug console.logs (owner-only wrangler tail, no secret values)
• 'mutating endpoints don't verify target alive at version' (unreachable via UI — overlay always sends current version)
• aid content-hash collision (identical artifacts render identically)

Keeping this issue OPEN as the tracker for the remaining intentionally-deferred items.

test/p3-hardening.test.js (7/7).