diff --git a/docs/improvement-plan.md b/docs/improvement-plan.md new file mode 100644 index 0000000..4275df7 --- /dev/null +++ b/docs/improvement-plan.md @@ -0,0 +1,153 @@ +# ytplayer — Improvement Plan (2026-09-29) + +Grounded in the current tree: `frontend/app.js` 9.8k lines / 30+ module objects, +`styles.css` 4.3k, `server/server.js` 1.9k with ~30 routes, 11 unit/server test +files (~110 cases), 5 Playwright specs, **no CI**, no lint/format config. +The feature set is already broad (lyrics, party, remote, presenter, stats, +transitions, uploads). The biggest returns now come from **hardening and +maintainability**, not more features. + +Priority key: **P0** do next · **P1** this quarter · **P2** when there's room. +Effort: S ≤ ½ day · M 1–3 days · L a week+. + +--- + +## Phase 1 — Safety net (P0) + +Nothing below is safe to refactor without it; the CLAUDE.md history (update +banner "fixed three times", the thumb-cache opaque trap, the toast-loop tab +crash) shows regressions have repeatedly shipped silently. + +| # | Item | Effort | Done when | +|---|------|--------|-----------| +| 1.1 | **GitHub Actions CI**: `node --test frontend/*.test.js`, `cd server && bun run test`, Playwright chromium smoke specs. Run on every push/PR. | S | Red CI blocks a merge; badge in README. | +| 1.2 | Fix root `npm test` — it runs `node --test frontend/`, which CLAUDE.md says fails on Node 22. Change to `node --test frontend/*.test.js` and add `test:server`, `test:e2e`, `test:all`. | S | `npm run test:all` green locally and in CI. | +| 1.3 | **Lint + format**: Biome (one binary, zero config sprawl) with `no-unused-vars`, `no-undef`, `useAwait`, `noFallthrough`. Start with `--diagnostic-level=error` so it lands without a mass reformat commit. | S | CI step; existing code passes. | +| 1.4 | **Regression tests for the known fragile paths**: (a) `/sw.js` BUILD_TAG regex injection survives a bumped fallback literal; (b) `maybeShowUpdateBanner` single-rule behaviour; (c) thumb cache accepts CORS-mode responses (status 0 opaque trap); (d) `toast()` cap with deferred removal terminates. | M | Each has a test that fails on the historical bug. | +| 1.5 | **Server route contract tests** for `/api/streams`, `/api/search`, `/api/user/sync`, `/api/profile/*` against a stubbed yt-dlp binary (a script that prints fixture JSON). Guards the "JSON shapes mirror the Tauri bridge" rule. | M | Shape snapshot per endpoint. | + +## Phase 2 — Security & abuse hardening (P0/P1) + +| # | Item | Pri | Effort | Notes | +|---|------|-----|--------|-------| +| 2.1 | **Rate-limit expensive routes.** `/api/search`, `/api/channel`, `/api/streams`, `/api/playlist/expand`, `/api/download` each spawn yt-dlp/ffmpeg with no per-IP limit — a trivial loop can exhaust CPU and get the homelab IP bot-gated by YouTube (which already degrades saves to 360p, see BACKLOG). Token bucket per first `X-Forwarded-For` hop, plus a global concurrency cap on yt-dlp spawns with a small queue. | P0 | M | Reuse the IP logic already written for `REMOTE_SAME_NETWORK`. | +| 2.2 | **Profile name enumeration.** Names are the only credential and may be 3 chars (`PROFILE_NAME_RE`), so `/api/profile/load` + `/api/playlist/inbox` are brute-forceable and nothing throttles them. Minimum: strict per-IP limit on 404s from those routes + raise the minimum for *new* profiles to ~10 chars or suggest a generated passphrase. Optional later: an opt-in per-profile PIN (hash stored) that `save` requires. | P0 | S–M | Keep "name = passkey" UX for existing profiles. | +| 2.3 | **Security headers** via `hono/secure-headers`: CSP (self + ytimg/ggpht images + the listed CDNs), `X-Content-Type-Options`, `Referrer-Policy`, `frame-ancestors 'self'`. Start CSP in `Report-Only` for a week — app.js builds a lot of `innerHTML` (124 sites). | P1 | M | Admin page gets a stricter policy. | +| 2.4 | **innerHTML audit**: lint rule / grep gate that every interpolation into `innerHTML` goes through `escapeHtml` (67 call sites today vs 124 innerHTML writes). Convert the risky ones (titles, channel names, chat, notes, profile names from URLs) to `textContent` or a tiny `html\`\`` tagged template that escapes by default. | P1 | M | Party chat + shared playlists carry attacker-controlled text. | +| 2.5 | Body-size limits on every JSON POST (profile already has `PROFILE_MAX_BYTES`; check `/api/user/sync`, notes, party/remote WS frames). | P1 | S | | +| 2.6 | Finish the **cookies jar** backlog item (mount `YTDLP_COOKIES` via a compose volume + an admin-page "cookies expire in N days" warning) — only if 360p fallback saves are noticed. | P2 | S | Already documented in BACKLOG. | + +## Phase 3 — Break up `app.js` (P1) + +9.8k lines in one file is the main drag on every change (and on AI-assisted +edits, which have to re-read huge ranges). The modules are already +IIFE-shaped (`Notes`, `Remote`, `Transition`, `Wave`, `Party`, `Presenter`, +`SectionRail`, `EQ`, …), so the split is mechanical. + +1. **Native ES modules, no bundler.** `