From 4de1364f74990faaf65840fe694f26f32870fb0d Mon Sep 17 00:00:00 2001 From: Slop Master Flex Date: Fri, 24 Jul 2026 23:14:56 +0000 Subject: [PATCH] rsgain frontend --- .gitignore | 3 + AGENTS.md | 4 + db.ts | 34 ++++ public/auth.js | 6 + public/channelSync.js | 1 + public/controls.js | 1 + public/core.js | 3 + public/index.html | 15 ++ public/init.js | 1 + public/replayGain.js | 185 +++++++++++++++++++ public/styles.css | 35 ++++ public/trackContainer.js | 2 + public/visualizer.js | 30 ++- routes/auth.ts | 42 +++++ routes/index.ts | 4 + server.ts | 1 + todo/README.md | 36 ++++ todo/channel-get-auth/overview.md | 74 ++++++++ todo/client-cache-correctness/overview.md | 155 ++++++++++++++++ todo/client-store-and-windowing/overview.md | 154 +++++++++++++++ todo/duplicate-download-race/overview.md | 111 +++++++++++ todo/fetch-response-validation/overview.md | 87 +++++++++ todo/websocket-robustness/overview.md | 121 ++++++++++++ todo/ws-guest-control-permission/overview.md | 74 ++++++++ todo/xss-fixes/overview.md | 98 ++++++++++ 25 files changed, 1273 insertions(+), 4 deletions(-) create mode 100644 public/replayGain.js create mode 100644 todo/README.md create mode 100644 todo/channel-get-auth/overview.md create mode 100644 todo/client-cache-correctness/overview.md create mode 100644 todo/client-store-and-windowing/overview.md create mode 100644 todo/duplicate-download-race/overview.md create mode 100644 todo/fetch-response-validation/overview.md create mode 100644 todo/websocket-robustness/overview.md create mode 100644 todo/ws-guest-control-permission/overview.md create mode 100644 todo/xss-fixes/overview.md diff --git a/.gitignore b/.gitignore index 4ecb99e..6237271 100644 --- a/.gitignore +++ b/.gitignore @@ -60,3 +60,6 @@ blastoise.db config.json *.db-shm *.db-wal + +# machine-local ops notes (see AGENTS.md) +HiddenAgents.md diff --git a/AGENTS.md b/AGENTS.md index 7d93d7a..9a5002b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,6 +2,10 @@ Synchronized music streaming server built with Bun. Manages "channels" (virtual radio stations) that play through queues sequentially. Clients connect, receive now-playing state, download audio, and sync playback locally. +## HiddenAgents.md + +If a `HiddenAgents.md` file exists in this repository, read it as well. It is git-ignored and contains machine-local operational details about how this server is deployed and managed on this host. + ## Architecture ### Server diff --git a/db.ts b/db.ts index 2634d55..48ad563 100644 --- a/db.ts +++ b/db.ts @@ -55,6 +55,17 @@ db.run(` ) `); +db.run(` + CREATE TABLE IF NOT EXISTS user_preferences ( + user_id INTEGER NOT NULL, + key TEXT NOT NULL, + value TEXT NOT NULL, + updated_at INTEGER DEFAULT (unixepoch()), + PRIMARY KEY (user_id, key), + FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE + ) +`); + // Types export interface User { id: number; @@ -249,6 +260,29 @@ export function getUserPermissions(userId: number): Permission[] { return db.query("SELECT * FROM permissions WHERE user_id = ?").all(userId) as Permission[]; } +// User preferences functions (key-value store, per account) +export function getUserPreference(userId: number, key: string): string | null { + const row = db.query("SELECT value FROM user_preferences WHERE user_id = ? AND key = ?").get(userId, key) as { value: string } | null; + return row ? row.value : null; +} + +export function getAllUserPreferences(userId: number): Record { + const rows = db.query("SELECT key, value FROM user_preferences WHERE user_id = ?").all(userId) as { key: string; value: string }[]; + const prefs: Record = {}; + for (const row of rows) prefs[row.key] = row.value; + return prefs; +} + +export function setUserPreference(userId: number, key: string, value: string): void { + db.query(` + INSERT INTO user_preferences (user_id, key, value, updated_at) + VALUES (?, ?, ?, unixepoch()) + ON CONFLICT(user_id, key) DO UPDATE SET + value = excluded.value, + updated_at = excluded.updated_at + `).run(userId, key, value); +} + export function getAllUsers(): Omit[] { const users = db.query("SELECT id, username, is_admin, is_guest, created_at FROM users WHERE is_guest = 0").all() as any[]; return users.map(u => ({ ...u, is_admin: !!u.is_admin, is_guest: false })); diff --git a/public/auth.js b/public/auth.js index dda2df3..4b22024 100644 --- a/public/auth.js +++ b/public/auth.js @@ -13,6 +13,9 @@ if (M.currentUser && data.permissions) { M.currentUser.permissions = data.permissions; } + if (data.preferences) { + M.loadReplayGainPrefs && M.loadReplayGainPrefs(data.preferences); + } M.updateAuthUI(); // Start slow queue polling if logged in if (M.currentUser && !M.currentUser.isGuest && M.startSlowQueuePoll) { @@ -100,6 +103,9 @@ if (M.currentUser && data.permissions) { M.currentUser.permissions = data.permissions; } + if (data.preferences) { + M.loadReplayGainPrefs && M.loadReplayGainPrefs(data.preferences); + } M.updateAuthUI(); if (M.currentUser) M.loadStreams(); } catch (e) { diff --git a/public/channelSync.js b/public/channelSync.js index 8024cd8..340eff5 100644 --- a/public/channelSync.js +++ b/public/channelSync.js @@ -499,6 +499,7 @@ if (isNewTrack) { M.currentTrackId = trackId; M.setTrackTitle(data.track.title); + M.applyReplayGain && M.applyReplayGain(data.track); M.loadingSegments.clear(); // Auto-scroll queue to current track diff --git a/public/controls.js b/public/controls.js index 571b69b..3ccf140 100644 --- a/public/controls.js +++ b/public/controls.js @@ -72,6 +72,7 @@ M.currentTrackId = trackId; M.serverTrackDuration = track.duration; M.setTrackTitle(track.title?.trim() || track.filename?.replace(/\.[^.]+$/, "") || "Unknown"); + M.applyReplayGain && M.applyReplayGain(track); M.loadingSegments.clear(); const cachedUrl = await M.loadTrackBlob(trackId); M.audio.src = cachedUrl || M.getTrackUrl(trackId); diff --git a/public/core.js b/public/core.js index 3978363..af1fb61 100644 --- a/public/core.js +++ b/public/core.js @@ -14,6 +14,9 @@ window.MusicRoom = { serverTrackDuration: 0, lastServerUpdate: 0, serverPaused: true, + + // Current track cache (for ReplayGain re-application across graph init) + currentTrack: null, // Channels list channels: [], diff --git a/public/index.html b/public/index.html index 57b2b7e..da89a29 100644 --- a/public/index.html +++ b/public/index.html @@ -207,6 +207,20 @@
stream +
+ rg + +
πŸ”Š
@@ -228,6 +242,7 @@ + diff --git a/public/init.js b/public/init.js index c6acbca..bc62084 100644 --- a/public/init.js +++ b/public/init.js @@ -38,6 +38,7 @@ M.currentTrackId = trackId; M.serverTrackDuration = track.duration; M.setTrackTitle(track.title || track.filename); + M.applyReplayGain && M.applyReplayGain(track); M.loadingSegments.clear(); const cachedUrl = await M.loadTrackBlob(trackId); diff --git a/public/replayGain.js b/public/replayGain.js new file mode 100644 index 0000000..bc37b36 --- /dev/null +++ b/public/replayGain.js @@ -0,0 +1,185 @@ +// MusicRoom - ReplayGain module +// Applies per-track loudness normalization using server-provided rsgain metadata. +// Gain is applied client-side via the shared Web Audio gain node (M.gainNode), +// so it only affects this listener's playback. Preferences are persisted +// per-account via /api/auth/me/preferences (localStorage fallback). + +(function() { + const M = window.MusicRoom; + + const LS_ENABLED = "blastoise_replaygain_enabled"; + const LS_PREAMP = "blastoise_replaygain_preamp"; + const PREAMP_MIN = -12; + const PREAMP_MAX = 12; + const PREAMP_STEP = 0.5; + const MAX_LINEAR = 10; // +20 dB hard cap to avoid extreme boosts + + M.replayGain = { + enabled: localStorage.getItem(LS_ENABLED) !== "false", // default true + preampDb: clampPreamp(Number.parseFloat(localStorage.getItem(LS_PREAMP)) || 0), + }; + + M.currentTrack = null; + + function clampPreamp(v) { + if (!Number.isFinite(v)) return 0; + return Math.max(PREAMP_MIN, Math.min(PREAMP_MAX, Math.round(v / PREAMP_STEP) * PREAMP_STEP)); + } + + // linear gain factor for a track, honoring enable flag, preamp, and peak clipping + function computeLinearGain(track) { + if (!M.replayGain.enabled) return 1.0; + const preamp = M.replayGain.preampDb || 0; + let gainDb = preamp; + const rgDb = track ? track.replayGainDb : null; + if (Number.isFinite(rgDb)) gainDb += rgDb; + let linear = Math.pow(10, gainDb / 20); + const peak = track ? track.replayPeak : null; + if (Number.isFinite(peak) && peak > 0 && peak * linear > 1) { + linear = 1 / peak; + } + return Math.min(linear, MAX_LINEAR); + } + + function setGain(node, linear) { + const ctx = node.context; + if (ctx.state === "running") { + const t = ctx.currentTime; + node.gain.cancelScheduledValues(t); + node.gain.setTargetAtTime(linear, t, 0.02); // ~20ms ramp, avoids clicks + } else { + node.gain.value = linear; // context suspended; apply directly + } + } + + // Apply gain for the current (or given) track. Safe to call before the graph + // exists β€” the value is re-applied when the graph initializes. + M.applyReplayGain = function(track) { + if (track) M.currentTrack = track; + const t = M.currentTrack; + const hasData = Number.isFinite(t && t.replayGainDb) || Number.isFinite(t && t.replayPeak); + // Only build the audio graph when there's actually something to apply (or it + // already exists), preserving default behavior for libraries without RG. + if (M.replayGain.enabled && (hasData || M.gainNode || M.replayGain.preampDb)) { + M.ensureAudioGraph && M.ensureAudioGraph(); + } + const node = M.gainNode; + if (!node) return; + setGain(node, computeLinearGain(t)); + }; + + M.setReplayGainEnabled = function(enabled) { + M.replayGain.enabled = !!enabled; + localStorage.setItem(LS_ENABLED, M.replayGain.enabled ? "true" : "false"); + M.applyReplayGain(); + M.updateReplayGainUI && M.updateReplayGainUI(); + M.saveReplayGainPrefs && M.saveReplayGainPrefs(); + }; + + M.setReplayGainPreamp = function(db) { + M.replayGain.preampDb = clampPreamp(db); + localStorage.setItem(LS_PREAMP, String(M.replayGain.preampDb)); + M.applyReplayGain(); + M.updateReplayGainUI && M.updateReplayGainUI(); + M.saveReplayGainPrefs && M.saveReplayGainPrefs(); + }; + + // Debounced persistence to the server (per-account). localStorage is the + // immediate fallback for guests/offline. + let saveTimer = null; + M.saveReplayGainPrefs = function() { + if (saveTimer) clearTimeout(saveTimer); + saveTimer = setTimeout(() => { + saveTimer = null; + const body = { + replaygain_enabled: M.replayGain.enabled ? "true" : "false", + replaygain_preamp: String(M.replayGain.preampDb), + }; + fetch("/api/auth/me/preferences", { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }).catch(() => {}); + }, 400); + }; + + // Hydrate from the /api/auth/me preferences object (called by auth.js). + M.loadReplayGainPrefs = function(prefs) { + if (!prefs) return; + if (prefs.replaygain_enabled != null) { + M.replayGain.enabled = prefs.replaygain_enabled === "true"; + localStorage.setItem(LS_ENABLED, M.replayGain.enabled ? "true" : "false"); + } + if (prefs.replaygain_preamp != null) { + const v = Number.parseFloat(prefs.replaygain_preamp); + if (Number.isFinite(v)) { + M.replayGain.preampDb = clampPreamp(v); + localStorage.setItem(LS_PREAMP, String(M.replayGain.preampDb)); + } + } + M.applyReplayGain(); + M.updateReplayGainUI && M.updateReplayGainUI(); + }; + + // ---- UI ---- + function initReplayGainUI() { + const btn = M.$("#btn-replaygain"); + const popover = M.$("#replaygain-popover"); + const enabledCheckbox = M.$("#rg-enabled"); + const preampSlider = M.$("#rg-preamp"); + const preampValue = M.$("#rg-preamp-value"); + if (!btn) return; + + function open() { + if (!popover) return; + popover.classList.remove("hidden"); + document.addEventListener("pointerdown", onOutside, true); + document.addEventListener("keydown", onKey); + } + function close() { + if (!popover) return; + popover.classList.add("hidden"); + document.removeEventListener("pointerdown", onOutside, true); + document.removeEventListener("keydown", onKey); + } + function toggle() { + if (popover && popover.classList.contains("hidden")) open(); + else close(); + } + function onOutside(e) { + if (popover && !popover.contains(e.target) && e.target !== btn) close(); + } + function onKey(e) { + if (e.key === "Escape") close(); + } + + btn.onclick = (e) => { + e.stopPropagation(); + toggle(); + }; + + if (enabledCheckbox) { + enabledCheckbox.onchange = () => M.setReplayGainEnabled(enabledCheckbox.checked); + } + if (preampSlider) { + preampSlider.min = String(PREAMP_MIN); + preampSlider.max = String(PREAMP_MAX); + preampSlider.step = String(PREAMP_STEP); + preampSlider.oninput = () => M.setReplayGainPreamp(Number.parseFloat(preampSlider.value)); + } + + M.updateReplayGainUI = function() { + if (btn) btn.classList.toggle("active", M.replayGain.enabled); + if (enabledCheckbox) enabledCheckbox.checked = M.replayGain.enabled; + if (preampSlider) preampSlider.value = String(M.replayGain.preampDb); + if (preampValue) { + const db = M.replayGain.preampDb; + preampValue.textContent = (db > 0 ? "+" : "") + (Number.isInteger(db) ? db : db.toFixed(1)) + " dB"; + } + }; + + M.updateReplayGainUI(); + } + + document.addEventListener("DOMContentLoaded", initReplayGainUI); +})(); diff --git a/public/styles.css b/public/styles.css index a32d209..1576f00 100644 --- a/public/styles.css +++ b/public/styles.css @@ -267,6 +267,33 @@ h3 { font-size: 0.8rem; color: #999; margin-bottom: 0.3rem; text-transform: uppe #btn-mute:hover { opacity: 1; } #volume { width: 120px; accent-color: #4e8; } +/* ReplayGain control */ +#replaygain-wrap { position: relative; } +#btn-replaygain { font-size: 0.7rem; cursor: pointer; color: #666; transition: color 0.2s, text-shadow 0.2s; letter-spacing: 0.05em; user-select: none; } +#btn-replaygain:hover { color: #888; } +#btn-replaygain.active { color: #4e8; text-shadow: 0 0 6px #4e8; } +#replaygain-popover { + position: absolute; + bottom: calc(100% + 0.5rem); + right: 0; + background: #1a1a1a; + border: 1px solid #333; + border-radius: 6px; + padding: 0.6rem 0.75rem; + display: flex; + flex-direction: column; + gap: 0.5rem; + min-width: 210px; + z-index: 50; + box-shadow: 0 4px 16px rgba(0,0,0,0.5); + font-size: 0.8rem; +} +.rg-row { display: flex; align-items: center; justify-content: space-between; gap: 0.5rem; } +.rg-row > span:first-child { color: #aaa; } +#rg-preamp { width: 90px; accent-color: #4e8; } +#rg-preamp-value { font-size: 0.7rem; color: #888; min-width: 3rem; text-align: right; } +#rg-enabled { accent-color: #4e8; width: 16px; height: 16px; } + /* Common */ button { background: #222; color: #eee; border: 1px solid #333; padding: 0.4rem 1rem; border-radius: 4px; cursor: pointer; font-size: 0.85rem; } button:hover { background: #333; } @@ -687,6 +714,14 @@ button:hover { background: #333; } justify-content: center; } #volume { width: min(160px, 55vw); height: 44px; } + #btn-replaygain { + min-width: 44px; + min-height: 44px; + display: flex; + align-items: center; + justify-content: center; + } + #replaygain-popover { right: auto; left: 50%; transform: translateX(-50%); } .track-actions .track-menu-btn { opacity: 1; width: 44px; diff --git a/public/trackContainer.js b/public/trackContainer.js index 3c219fc..691cce2 100644 --- a/public/trackContainer.js +++ b/public/trackContainer.js @@ -619,6 +619,7 @@ M.currentTrackId = trackId; M.serverTrackDuration = track.duration; M.setTrackTitle(title); + M.applyReplayGain && M.applyReplayGain(track); M.loadingSegments.clear(); const cachedUrl = await M.loadTrackBlob(trackId); M.audio.src = cachedUrl || M.getTrackUrl(trackId); @@ -637,6 +638,7 @@ M.currentTrackId = trackId; M.serverTrackDuration = track.duration; M.setTrackTitle(title); + M.applyReplayGain && M.applyReplayGain(track); M.loadingSegments.clear(); const cachedUrl = await M.loadTrackBlob(trackId); diff --git a/public/visualizer.js b/public/visualizer.js index 3e7e8d0..4def506 100644 --- a/public/visualizer.js +++ b/public/visualizer.js @@ -17,6 +17,7 @@ let fullscreenButton = null; let audioContext = null; let source = null; + let gainNode = null; let analyser = null; let frequencyData = null; let waveformData = null; @@ -156,11 +157,20 @@ analyser.smoothingTimeConstant = 0.68; source = audioContext.createMediaElementSource(M.audio); - source.connect(analyser); + // Topology: source -> gainNode -> analyser -> destination. + // gainNode is shared with ReplayGain (M.gainNode); analyser is a + // transparent pass-through, so visualizer-off is unaffected. + gainNode = audioContext.createGain(); + source.connect(gainNode); + gainNode.connect(analyser); analyser.connect(audioContext.destination); + M.gainNode = gainNode; frequencyData = new Uint8Array(analyser.frequencyBinCount); waveformData = new Uint8Array(analyser.fftSize); + + // Graph may be built after the first track arrives; re-apply current gain. + M.applyReplayGain?.(M.currentTrack); return true; } catch (error) { graphUnavailable = true; @@ -169,6 +179,16 @@ } } + // Build the audio graph (if not already) and resume the context if needed. + // Used by ReplayGain so the gain node exists even with the visualizer off. + M.ensureAudioGraph = function() { + if (!initAudioGraph()) return false; + if (audioContext && audioContext.state === "suspended") { + audioContext.resume().catch(() => {}); + } + return true; + }; + function startVisualizer() { if (animationId || mode === "off") return; animationId = requestAnimationFrame(draw); @@ -767,10 +787,12 @@ }); M.audio.addEventListener("play", () => { + // Resume context whenever the graph exists (visualizer or replaygain path), + // since routing through it requires a running context to produce sound. + if (audioContext && audioContext.state === "suspended") { + audioContext.resume().catch(() => {}); + } if (mode !== "off") { - if (audioContext && audioContext.state === "suspended") { - audioContext.resume().catch(() => {}); - } startVisualizer(); } }); diff --git a/routes/auth.ts b/routes/auth.ts index 17d7b48..b6376f9 100644 --- a/routes/auth.ts +++ b/routes/auth.ts @@ -9,6 +9,8 @@ import { getAllUsers, grantPermission, revokePermission, + getAllUserPreferences, + setUserPreference, } from "../db"; import { getUser, @@ -93,6 +95,17 @@ export function handleLogout(req: Request): Response { ); } +// Whitelisted per-account preference keys (key -> parser/normalizer). +// Add new keys here as features need per-account persistence. +const PREFERENCE_KEYS: Record string | null> = { + replaygain_enabled: (raw) => (raw === true || raw === "true" ? "true" : raw === false || raw === "false" ? "false" : null), + replaygain_preamp: (raw) => { + const n = typeof raw === "number" ? raw : Number.parseFloat(String(raw)); + if (!Number.isFinite(n)) return null; + return String(Math.max(-24, Math.min(24, n))); + }, +}; + // Auth: get current user export function handleGetMe(req: Request, server: any): Response { const { user, headers } = getOrCreateUser(req, server); @@ -116,9 +129,38 @@ export function handleGetMe(req: Request, server: any): Response { return Response.json({ user: { id: user.id, username: user.username, isAdmin: user.is_admin, isGuest: user.is_guest }, permissions: effectivePermissions, + preferences: getAllUserPreferences(user.id), }, { headers }); } +// Preferences: update per-account preferences (whitelisted keys only) +export async function handleUpdatePreferences(req: Request, server: any): Promise { + const { user } = getOrCreateUser(req, server); + if (!user) { + return Response.json({ error: "Not authenticated" }, { status: 401 }); + } + let body: any; + try { + body = await req.json(); + } catch { + return Response.json({ error: "Invalid JSON" }, { status: 400 }); + } + if (!body || typeof body !== "object" || Array.isArray(body)) { + return Response.json({ error: "Expected an object" }, { status: 400 }); + } + + const applied: Record = {}; + for (const [key, raw] of Object.entries(body)) { + const normalize = PREFERENCE_KEYS[key]; + if (!normalize) continue; // ignore unknown keys + const value = normalize(raw); + if (value == null) continue; // ignore invalid values + setUserPreference(user.id, key, value); + applied[key] = value; + } + return Response.json({ success: true, preferences: applied }); +} + // Kick all other clients for current user export function handleKickOthers(req: Request, server: any): Response { const { user } = getOrCreateUser(req, server); diff --git a/routes/index.ts b/routes/index.ts index 351e57d..1df5748 100644 --- a/routes/index.ts +++ b/routes/index.ts @@ -9,6 +9,7 @@ import { handleLogin, handleLogout, handleGetMe, + handleUpdatePreferences, handleKickOthers, handleListUsers, handleGrantPermission, @@ -212,6 +213,9 @@ export function createRouter() { if (path === "/api/auth/me") { return handleGetMe(req, server); } + if (path === "/api/auth/me/preferences" && req.method === "PUT") { + return handleUpdatePreferences(req, server); + } if (path === "/api/auth/kick-others" && req.method === "POST") { return handleKickOthers(req, server); } diff --git a/server.ts b/server.ts index d331bc1..e872b35 100644 --- a/server.ts +++ b/server.ts @@ -4,6 +4,7 @@ import { init } from "./init"; import { createRouter } from "./routes"; import { websocketHandlers } from "./websocket"; + await init(); serve({ diff --git a/todo/README.md b/todo/README.md new file mode 100644 index 0000000..d339474 --- /dev/null +++ b/todo/README.md @@ -0,0 +1,36 @@ +# TODO β€” Implementation Plans + +Eight plans derived from the architecture assessment, ordered by priority and with +effort/risk so you can sequence the work. Each lives in `todo//overview.md`. + +## Critical / High β€” do first (all low effort, low risk) + +| # | Topic | Priority | Effort | Depends on | +|---|-------|----------|--------|------------| +| 1 | [xss-fixes](xss-fixes/overview.md) β€” escape server data at `innerHTML`; add CSP | Critical | Low | β€” | +| 2 | [websocket-robustness](websocket-robustness/overview.md) β€” try/catch parse, reconnect backoff, dead-channel fallback | High | Low–Med | β€” | +| 3 | [ws-guest-control-permission](ws-guest-control-permission/overview.md) β€” route WS control through `userHasPermission` | High | Trivial | β€” | +| 4 | [channel-get-auth](channel-get-auth/overview.md) β€” add auth to `GET /api/channels/:id` | High | Trivial | β€” | + +Items 1–4 are independent and can be done in parallel. Each is a small, isolated change. + +## Medium β€” correctness and efficiency + +| # | Topic | Priority | Effort | Depends on | +|---|-------|----------|--------|------------| +| 5 | [fetch-response-validation](fetch-response-validation/overview.md) β€” `res.ok` checks before `res.json()` | Medium | Low | β€” | +| 6 | [duplicate-download-race](duplicate-download-race/overview.md) β€” synchronous check-and-claim for bulk downloads | Medium | Low | β€” | +| 7 | [client-cache-correctness](client-cache-correctness/overview.md) β€” revoke blob URLs, LRU prune, real `streamOnly` | Medium | Medium | #6 (shares `downloadAndCacheTrack`) | + +## Strategic β€” refactor, do last + +| # | Topic | Priority | Effort | Depends on | +|---|-------|----------|--------|------------| +| 8 | [client-store-and-windowing](client-store-and-windowing/overview.md) β€” virtualize list + minimal store + decompose god module | Strategic | High | #1, #6, #7 (inherit correct behavior first) | + +## Suggested order + +1, 3, 4 (one-line/server-side, ship immediately) β†’ 2 β†’ 5 β†’ 6 β†’ 7 β†’ 8. + +All plans include affected file:line references, a step-by-step implementation +sketch, validation steps, and rollback notes. diff --git a/todo/channel-get-auth/overview.md b/todo/channel-get-auth/overview.md new file mode 100644 index 0000000..7e9f54e --- /dev/null +++ b/todo/channel-get-auth/overview.md @@ -0,0 +1,74 @@ +# Add Authentication to `GET /api/channels/:id` + +Priority: **High** Β· Effort: **Trivial (3 lines)** Β· Risk: **Low** + +## Problem + +Every sibling channel endpoint calls `getOrCreateUser(req, server)` and returns 401 if there is no user. The GET-state endpoint does not: + +```ts +// routes/channels.ts:167-171 +export function handleGetChannel(channelId: string): Response { + const channel = state.channels.get(channelId); + if (!channel) return new Response("Not found", { status: 404 }); + return Response.json(channel.getState()); +} +``` + +`Channel.getState()` (`channel.ts:142-159`) returns the queue, `currentIndex`, listener count, the `isDefault` flag, and (when requested) the full track list. Any unauthenticated caller can enumerate and read the state of every channel. + +## Affected Location + +- `routes/channels.ts:167-171` β€” `handleGetChannel`, missing auth. + +Also note: this handler's signature differs from its siblings (it takes only `channelId`, not `req, server, channelId`). The router call at `routes/index.ts:111-114` reflects this. Both must be updated. + +## Implementation Plan + +### Step 1 β€” Update the handler signature and add the auth check + +```ts +// routes/channels.ts +import { getOrCreateUser } from "./helpers"; + +// GET /api/channels/:id - get channel state +export function handleGetChannel(req: Request, server: any, channelId: string): Response { + const { user } = getOrCreateUser(req, server); + if (!user) { + return Response.json({ error: "Authentication required" }, { status: 401 }); + } + const channel = state.channels.get(channelId); + if (!channel) return new Response("Not found", { status: 404 }); + return Response.json(channel.getState()); +} +``` + +### Step 2 β€” Update the router call site + +`routes/index.ts:111-114`: + +```ts +const channelGetMatch = path.match(/^\/api\/channels\/([^/]+)$/); +if (channelGetMatch && req.method === "GET") { + return handleGetChannel(req, server, channelGetMatch[1]); +} +``` + +(Pass `req` and `server` through, matching the DELETE/PATCH handlers above.) + +### Step 3 β€” Consider whether state should be filtered by listener + +Today `getState()` does not return the `listeners` array (only `listenerCount`) β€” good. Confirm this remains true; if `listeners` is ever added to `getState()`, also gate it behind the same ownership/permission rules used by `getListInfo()` (`channel.ts:377-389`), which *does* expose usernames. (Out of scope for this fix, but worth a note: the channel *list* endpoint exposes listener usernames to any authenticated user β€” acceptable for a listen-along app, but verify it matches the product intent.) + +## Validation + +- **Unauthenticated request is rejected**: `curl -i http://localhost:3001/api/channels/main` β†’ expect `401` (previously `200` with full state). +- **Authenticated request still works**: `curl -i -b cookies.txt http://localhost:3001/api/channels/main` β†’ expect `200` with state JSON. +- **Guest access**: with `allowGuests: true`, an unauthenticated curl should now receive a `Set-Cookie` guest session *and* still be able to read state on the next request (guests can listen). Confirm the second request returns 200. +- **Client still works**: load the app, switch channels, confirm no regression (the client always sends the session cookie). + +## Risk / Rollback + +- The client already authenticates every request via cookie, so legitimate UI is unaffected. +- If any external/SSR/integration consumer relied on the open endpoint, they will now get 401 β€” re-grant via a real session. This was never intended public surface. +- Rollback = revert the handler signature and the router call. diff --git a/todo/client-cache-correctness/overview.md b/todo/client-cache-correctness/overview.md new file mode 100644 index 0000000..39f858a --- /dev/null +++ b/todo/client-cache-correctness/overview.md @@ -0,0 +1,155 @@ +# Client Cache Correctness: Revoke Blob URLs, LRU Pruning, Real `streamOnly` + +Priority: **Medium** Β· Effort: **Medium** Β· Risk: **Low** + +## Problem + +Three independent correctness/efficiency bugs in the client caching layer: + +1. **Blob URL leak.** `URL.createObjectURL(blob)` results stored in `M.trackBlobs` (`public/audioCache.js:80,127`) are never revoked. `M.trackBlobs.clear()` (`queue.js:188`) drops the references but the URLs stay live in the document until unload β€” a slow memory leak over long sessions with many cached tracks. + +2. **`pruneCache` is expensive and non-LRU.** `public/audioCache.js:27-54` calls `TrackStorage.get(key)` for every cached track (pulling each blob out of IndexedDB) just to read `.size`, and evicts oldest-first by Map iteration order β€” *not* by the `cachedAt` timestamp that is actually written at `public/trackStorage.js:86`. So eviction is arbitrary, not least-recently-used. + +3. **`streamOnly` doesn't do what it says.** The flag is stored (`audioCache.js:8-15`) and checked by exactly one context-menu item (`trackContainer.js:752`), but the background prefetch loop (`audioCache.js:226`) never consults it β€” so "stream-only mode" still fetches every segment in the background and still triggers bulk caching. + +## Affected Locations + +- `public/audioCache.js:27-54` β€” `pruneCache` +- `public/audioCache.js:80,127` β€” blob URL creation +- `public/audioCache.js:224-262` β€” prefetch loop (no `streamOnly` check) +- `public/trackStorage.js:139-155` β€” `getStats` (loads all blobs) +- `public/trackStorage.js:86` β€” `cachedAt` is written but unused +- `public/queue.js:188` β€” `M.trackBlobs.clear()` without revoke + +## Implementation Plan + +### Part A β€” Track and revoke blob URLs + +**Step A1: Track blob URLs in a single Map with a revocation helper.** + +`M.trackBlobs` is already `Map`. Add: + +```js +// public/audioCache.js +M.revokeTrackBlob = function (trackId) { + const url = M.trackBlobs.get(trackId); + if (url) { + URL.revokeObjectURL(url); + M.trackBlobs.delete(trackId); + } +}; +``` + +**Step A2: Revoke on eviction and on explicit clear.** + +- In `pruneCache` (after Part B), when a track is evicted from IndexedDB, also call `M.revokeTrackBlob(trackId)` so the in-memory URL is released. +- In `queue.js:188` (`clearAllCaches`), revoke every entry before clearing: + +```js +for (const url of M.trackBlobs.values()) URL.revokeObjectURL(url); +M.trackBlobs.clear(); +``` + +- When a track is removed from the library (`track_removed` handler in `channelSync.js`), revoke its blob if present (it can't be played anymore). + +**Step A3: Don't double-revoke.** `URL.revokeObjectURL` is safe to call on an already-revoked URL, but guard with the Map check anyway to keep state clean. + +### Part B β€” Make pruning LRU and cheap + +**Step B1: Store size at write time, avoid loading blobs at prune time.** + +Extend the IndexedDB record to carry `size` (the blob already has `.size` β€” store it alongside). In `trackStorage.js`: + +```js +// in set(): store { filename (keyPath), blob, size, cachedAt } +const record = { filename: trackId, blob, size: blob.size, cachedAt: Date.now() }; +``` + +Add a lightweight metadata accessor that uses a cursor and reads only `size`/`cachedAt`, not the blob: + +```js +// trackStorage.js +getMetaList: function () { + return new Promise((resolve) => { + const result = []; + const tx = db.transaction(STORE, "readonly"); + const store = tx.objectStore(STORE); + const req = store.openCursor(); + req.onsuccess = (e) => { + const cursor = e.target.result; + if (cursor) { + const v = cursor.value; + result.push({ id: v.filename, size: v.size || 0, cachedAt: v.cachedAt || 0 }); + cursor.continue(); + } else { + resolve(result); + } + }; + req.onerror = () => resolve([]); + }); +} +``` + +**Step B2: Rewrite `pruneCache` to be LRU and blob-free.** + +```js +// audioCache.js +async function pruneCache() { + const limit = M.cacheLimitBytes ?? 500 * 1024 * 1024; // tune as needed + const meta = await TrackStorage.getMetaList(); + let total = meta.reduce((s, m) => s + m.size, 0); + if (total <= limit) return; + + // Evict least-recently-used first. + meta.sort((a, b) => a.cachedAt - b.cachedAt); + for (const m of meta) { + if (total <= limit) break; + await TrackStorage.delete(m.id); + M.revokeTrackBlob(m.id); + M.cachedTracks.delete(m.id); + total -= m.size; + } +} +``` + +**Step B3: Update `cachedAt` on access** (optional, makes LRU reflect actual use). When a cached track is played (`loadTrackBlob` returns a URL), bump its `cachedAt`. If skipped, the policy becomes "first-in-first-out", which is still better than today's arbitrary order. + +**Step B4: Rewrite `getStats`** to use `getMetaList` so the settings/stats panel doesn't load every blob either. + +### Part C β€” Make `streamOnly` actually disable background fetching + +**Step C1: Consult the flag in the prefetch loop.** + +In `audioCache.js:226` (top of the prefetch loop): + +```js +M.prefetchSegments = async function () { + if (prefetching) return; + if (M.streamOnly) return; // <-- new + prefetching = true; + try { + // ... existing logic ... + } finally { + prefetching = false; + } +}; +``` + +**Step C2: Also short-circuit the buffered-range-scan-triggered bulk download when in stream-only mode**, since streaming clients don't want IndexedDB writes. Easiest: have `maybeStartBulkDownload` (see `todo/duplicate-download-race/overview.md`) check `M.streamOnly` and return `false`. + +**Step C3: Decide intent for the already-playing track.** Even in stream-only mode the *current* track is being buffered by the browser's own media element β€” that's fine and desirable. The flag is about suppressing *additional* segment prefetching ahead of the playhead and *bulk* caching. Confirm the UX label reflects this ("Don't pre-cache ahead" rather than "stream nothing"). + +## Validation + +- **Blob revoke**: open DevTools β†’ Memory, take a snapshot; play and fully cache ~10 tracks; confirm `trackBlobs` grows by 10 and the document's blob URL count grows accordingly. Trigger a prune (or set a tiny `cacheLimitBytes`) and confirm the count drops as tracks are evicted. Run `clearAllCaches()` and confirm all blob URLs are revoked. +- **LRU**: with a small cache limit, cache tracks A, B, C; play A again (bump cachedAt if Part B3 is implemented); cache D. Confirm B (not A) is evicted. +- **Prune performance**: with a large cache (e.g. 200 tracks), instrument `pruneCache` and confirm it no longer reads blob payloads β€” wall time should drop from "loads every blob" to a cheap cursor scan. +- **streamOnly**: toggle the flag on, play a track, seek around, and confirm via DevTools Network that only the media element's own range requests fire (no `/api/tracks/:id` segment prefetches), and IndexedDB write count stays flat. Toggle off and confirm prefetching resumes. +- **Regression**: with streamOnly off, confirm normal caching behavior (green indicators, blob swap, IndexedDB entries) is unchanged. + +## Risk / Rollback + +- **IndexedDB schema change** (adding `size`/`cachedAt` fields to the record): old records written before this change will lack those fields. `getMetaList` must default missing fields to `0` (shown above), and `pruneCache` must handle `size === 0` gracefully (skip eviction of unknown-size entries rather than evicting everything). Consider a one-time migration that deletes records lacking `size`, or simply let them age out. +- Revoking a blob URL that is *currently* the `src` of the audio element will break playback. Only revoke blobs for tracks that are not the currently-playing track, or that have just been removed from the library. Add a guard: `if (trackId === M.currentTrackId) return;` in `revokeTrackBlob` callers that run during playback. +- streamOnly is additive; default stays off. +- Rollback per part is independent β€” A, B, C can land separately. diff --git a/todo/client-store-and-windowing/overview.md b/todo/client-store-and-windowing/overview.md new file mode 100644 index 0000000..6598899 --- /dev/null +++ b/todo/client-store-and-windowing/overview.md @@ -0,0 +1,154 @@ +# Client Store & List Windowing (Strategic Refactor) + +Priority: **Medium (strategic)** Β· Effort: **High** Β· Risk: **Medium** + +This is the largest item of the eight. Unlike the others, it is a refactor rather than a bug fix. It should be sequenced *after* the XSS, cache, and WS work so the new structure inherits correct behavior. It can ship incrementally. + +## Problem + +The client is a pre-module vanilla-JS app that has outgrown its structure: + +1. **One mutable global** (`window.MusicRoom` / `M`, `public/core.js:4-73`) holds *all* state β€” audio element, WS, current track, queue, library, cache maps, permission flags, and UI bookkeeping (`lastProgressPct`, etc.). Every IIFE mutates `M` directly and calls `M.render*()`. There is no contract between modules; forward references are sometimes guarded with `M.foo && M.foo()` (`channelSync.js:502`), often not. + +2. **Full re-render of every row on every change.** `trackContainer.js:101-165` clears `innerHTML` and rebuilds every track row β€” triggered by every WS state update, every 5s cache poll (`ui.js:175-182`), and every prefetch completion. No windowing/virtualization: a 10k-track library renders 10k `.track` divs with fresh closures each time. Search (`queue.js:196-204`) filters the full array synchronously per keystroke. + +3. **God module.** `public/trackContainer.js` (972 lines) fuses rendering, drag state, selection, drop-zones, and context-menu DOM. + +4. **Duplicated logic**: local-playback setup (4Γ—), cookie get/set/clear (3Γ—), `escapeHtml` (2Γ—), segment-availability scan (3Γ—). + +## Affected Locations + +- `public/core.js:4-73` β€” global state +- `public/trackContainer.js` (972 lines) β€” render + drag + selection + context menu +- `public/trackContainer.js:101-165` β€” `render()` full clear+rebuild +- `public/ui.js:175-182` β€” 5s cache-poll re-render trigger +- `public/queue.js:196-204` β€” synchronous per-keystroke filter +- All IIFEs in `public/*.js` β€” module pattern + +## Goals (in priority order) + +1. **Virtualize the library list** so 10k+ tracks render smoothly. +2. **Introduce a tiny reactive store** so state changes are explicit and modules subscribe rather than mutate globals. +3. **Decompose `trackContainer.js`** into focused modules. +4. **De-duplicate** playback setup, cookie helpers, escaping. + +Non-goals: adopting a framework (React/Vue/etc.). Bun serves static files with no build step; introducing one is out of scope unless explicitly desired. This plan stays vanilla. + +## Implementation Plan + +The plan is staged so each phase ships value independently. + +### Phase 1 β€” Virtualize the list (highest standalone value) + +Even without a store refactor, windowing fixes the worst performance problem. + +**1.1 Add a windowed renderer** for the library (and queue when long). Only render rows visible in the viewport plus a small overscan buffer (e.g. 10 rows above/below). Use a sentinel/spacer div with the full scroll height so the scrollbar stays accurate. + +Sketch (`trackContainer.js`): + +```js +const ROW_HEIGHT = 44; // measure actual rendered row height +const OVERSCAN = 10; + +function renderWindow(container, items, scrollTop, viewportHeight) { + const totalHeight = items.length * ROW_HEIGHT; + const firstVisible = Math.max(0, Math.floor(scrollTop / ROW_HEIGHT) - OVERSCAN); + const visibleCount = Math.ceil(viewportHeight / ROW_HEIGHT) + OVERSCAN * 2; + const slice = items.slice(firstVisible, firstVisible + visibleCount); + + container.innerHTML = ""; // or keep a stable spacer + content wrapper + const spacerTop = document.createElement("div"); + spacerTop.style.height = `${firstVisible * ROW_HEIGHT}px`; + const spacerBottom = document.createElement("div"); + spacerBottom.style.height = `${(items.length - firstVisible - slice.length) * ROW_HEIGHT}px`; + container.appendChild(spacerTop); + for (let i = 0; i < slice.length; i++) { + container.appendChild(renderRow(slice[i], firstVisible + i)); + } + container.appendChild(spacerBottom); +} +``` + +**1.2 Drive renders from `scroll` events** (passive listener) and `requestAnimationFrame`-throttled. Debounce on resize. Do *not* re-render on every WS update β€” instead mark rows dirty and only update the visible slice. + +**1.3 Index rows by track id** so targeted updates (e.g. a single track's cache status changes) patch a specific DOM node instead of rebuilding the list. + +**1.4 Move search filtering to the model layer**: maintain `M.libraryFiltered` as a computed array; the filter runs once per query (debounced ~150ms), and the windowed renderer reads from it. Never re-filter on every keystroke synchronously. + +Deliverable: a library that scrolls smoothly at 10k tracks. + +### Phase 2 β€” Introduce a minimal reactive store + +Replace ad-hoc `M.x = …; M.render()` with a store that notifies subscribers. + +**2.1 Store shape** (new file `public/store.js`): + +```js +(function () { + const M = window.MusicRoom; + const state = {}; // the actual values + const subs = new Map(); // key -> Set + + M.store = { + get(key) { return state[key]; }, + set(key, value) { + if (Object.is(state[key], value)) return; // skip no-op + state[key] = value; + subs.get(key)?.forEach((cb) => cb(value)); + }, + update(key, fn) { M.store.set(key, fn(state[key])); }, + on(key, cb) { + if (!subs.has(key)) subs.set(key, new Set()); + subs.get(key).add(cb); + return () => subs.get(key)?.delete(cb); // unsubscribe + }, + }; +})(); +``` + +**2.2 Migrate state field by field.** Don't do a big-bang rewrite. Start with the fields that change most and drive renders: `M.library`, `M.queue`, `M.cachedTracks`, `M.currentTrackId`. Each becomes `M.store.set("library", …)` and the renderer subscribes via `M.store.on("library", renderLibrary)`. + +Keep the `M.library` getter as a compatibility shim (`Object.defineProperty(M, "library", { get: () => M.store.get("library") })`) so unmigrated callers keep working during the transition. + +**2.3 Decouple renders from setters.** Today `audioCache.js` calls `M.renderQueue()` directly. After migration it just `M.store.set("cachedTracks", …)` and the queue renderer β€” which subscribed once β€” decides whether and what to re-render. + +Deliverable: a single source of truth per field, explicit update points, and the ability to add per-field logging/invariants cheaply. + +### Phase 3 β€” Decompose `trackContainer.js` + +Split the 972-line file along its existing seams: + +- `trackList.js` β€” the windowed renderer (from Phase 1) + list-level keyboard nav. +- `trackSelection.js` β€” the `selection`/`lastSelected` state (`trackContainer.js:11-22`) and click/shift-click range logic. +- `trackDrag.js` β€” drag state (`trackContainer.js:8,24-31`), drop-zone rendering, queue reorder calls. +- `trackContextMenu.js` β€” menu construction + DOM (`showContextMenuUI` at `:880-949`), built on top of the row events. + +Each becomes a small module that subscribes to the store (Phase 2) and exposes a narrow API on `M.tracks.*`. + +Deliverable: no file over ~400 lines in the client. + +### Phase 4 β€” De-duplicate + +- **Playback setup**: extract a single `M.loadAndPlay(track, { seek: 0 })` used by `init.js`, `controls.js`, and both call sites in `trackContainer.js`. Today it's copy-pasted 4Γ— (see assessment). +- **Cookie helpers**: one `M.prefs.get/set/clear` (with `Secure` added β€” see XSS plan) replacing the three copies in `core.js`, `themes.js`, `visualizer.js`. +- **`escapeHtml`**: single `M.escapeHtml` (already part of the XSS plan). +- **Segment scan**: one `M.getBufferedSegments(audio, trackId)` used by `audioCache.js`, `ui.js`, `controls.js`. + +Deliverable: fewer copies, clearer ownership. + +## Validation + +- **List performance**: load a fixture library of 10k tracks (synthesize or copy metadata rows). Measure initial render time, scroll FPS, and keystroke-to-render latency in DevTools Performance. Target: 60fps scroll, <100ms keystroke response. +- **Correctness parity**: after each phase, verify against a manual checklist β€” play, pause, seek, jump, add-to-queue, remove-from-queue, drag-reorder, multi-select, context-menu actions, cache indicator updates, search filter, channel switch. Each phase must preserve all of these. +- **Store**: add a temporary `M.store.on("library", (v) => console.count("library"))` and confirm the count matches expected update frequency (not 5Γ— per WS message). +- **No regressions in cache behavior**: the windowed render must still update a row's cache indicator when its blob completes β€” verify by playing a track and watching its row in a scrolled-out list update without a full re-render. +- **Decomposition**: confirm no module exceeds ~400 lines and that each can be reasoned about in isolation. + +## Risk / Rollback + +- This is the highest-risk item because it touches the most code. Mitigate by: + - Shipping phases in order; each is independently mergeable and each is a checkpoint. + - Keeping `M.library`-style getters as shims during Phase 2 so partial migrations don't break. + - Behind a fallback: if windowing introduces scroll glitches, the non-windowed path can be kept as `M.renderLegacyList` and re-enabled until fixed. +- No server or DB changes; rollback is purely client-side file reversion. +- Do **not** attempt this before the XSS, cache-race, and WS plans land β€” those fix correctness bugs that a refactor would otherwise carry forward or obscure. diff --git a/todo/duplicate-download-race/overview.md b/todo/duplicate-download-race/overview.md new file mode 100644 index 0000000..af1aeb5 --- /dev/null +++ b/todo/duplicate-download-race/overview.md @@ -0,0 +1,111 @@ +# Fix Duplicate Full-Track Download Race in Client Cache + +Priority: **Medium** Β· Effort: **Low** Β· Risk: **Low** + +## Problem + +`M.bulkDownloadStarted` is a per-track flag meant to ensure `downloadAndCacheTrack` runs at most once per track. But the flag is **set inside `downloadAndCacheTrack` after an `await`**, while multiple callers check it **before** that await resolves. When the last two segments complete near-simultaneously β€” one from the browser's buffered-range scan in `ui.js`, one from the explicit `fetchSegment` in `audioCache.js` β€” both callers observe `trackCache.size >= SEGMENTS` and both pass the `bulkDownloadStarted` guard before the first caller sets the flag. Result: the same track is fetched in full **twice**, wasting bandwidth and racing on the IndexedDB write. + +## Affected Locations + +The check+call pattern appears in three places, all racy: + +- `public/audioCache.js:99-107` β€” `checkAndCacheComplete` +- `public/ui.js:148-150` β€” buffer-segment scan +- `public/audioCache.js:183-187` β€” inside `fetchSegment` + +And the flag is set here: + +- `public/audioCache.js:110-112` β€” inside `downloadAndCacheTrack`, **after** `await M.loadTrackBlob(trackId)`. + +## Root Cause + +Guard-then-do where the guard write is not atomic with respect to the awaits surrounding it. JavaScript's single-threaded execution means a synchronous "check-and-set" is race-free, but here the set happens after the first `await` inside the function being guarded, so a second caller can enter the function and pass the check before the first caller reaches the set. + +## Implementation Plan + +### Step 1 β€” Move the flag set to be synchronous with the check + +Refactor so the check and the set happen in the same synchronous tick, before any `await`. Create a single entry point: + +```js +// public/audioCache.js +M.maybeStartBulkDownload = function (trackId) { + if (M.bulkDownloadStarted.get(trackId)) return false; // already started + if (M.cachedTracks.has(trackId)) return false; // already cached + const trackCache = M.trackCaches.get(trackId); + if (!trackCache || trackCache.size < SEGMENTS) return false; // not fully buffered + // All conditions met β€” claim the slot synchronously: + M.bulkDownloadStarted.set(trackId, true); + // Fire the async work without awaiting here: + M.downloadAndCacheTrack(trackId).catch((err) => { + console.warn("[audioCache] bulk download failed for", trackId, err); + M.bulkDownloadStarted.delete(trackId); // allow a later retry + }); + return true; +}; +``` + +Then make `downloadAndCacheTrack` **assume the flag is already set** (remove the guard inside it): + +```js +M.downloadAndCacheTrack = async function (trackId) { + // Precondition: M.bulkDownloadStarted.get(trackId) === true (set by maybeStartBulkDownload) + const cachedUrl = await M.loadTrackBlob(trackId); + if (cachedUrl) { // already cached under us + M.cachedTracks.add(trackId); + M.bulkDownloadStarted.delete(trackId); + return cachedUrl; + } + // ... existing download + TrackStorage.set + URL.createObjectURL logic ... + M.cachedTracks.add(trackId); + M.bulkDownloadStarted.delete(trackId); // clear after success + M.renderQueue && M.renderQueue(); + M.renderLibrary && M.renderLibrary(); + return blobUrl; +}; +``` + +### Step 2 β€” Replace all three call sites + +Each caller becomes a single synchronous call: + +**`audioCache.js` `checkAndCacheComplete`:** +```js +M.maybeStartBulkDownload(trackId); +``` +(remove the old `if (...size >= SEGMENTS)` + `downloadAndCacheTrack` block.) + +**`ui.js:148-150`** (buffered-range scan, after a segment is marked present): +```js +M.maybeStartBulkDownload(M.currentTrackId); +``` + +**`audioCache.js:183-187`** (inside `fetchSegment`, after marking the segment): +```js +M.maybeStartBulkDownload(trackId); +``` + +Because the check-and-claim is now synchronous, even if all three callers fire in the same tick, only the first will start the download. + +### Step 3 β€” Guard the blob-URL swap during playback + +`downloadAndCacheTrack` swaps `M.audio.src` to the blob URL mid-playback (`audioCache.js:213-220`). After the refactor, confirm this swap still checks `M.currentTrackId === trackId` and that `wasPlaying` is captured at the moment of the swap (not earlier). Keep the existing `play().catch(()=>{})`. + +### Step 4 β€” (Related, recommended in same pass) Revoke blob URLs + +While in this file, address the related memory leak: every `URL.createObjectURL` stored in `M.trackBlobs` is never revoked. See `todo/client-cache-correctness/overview.md` β€” that plan covers it. If doing both together, the `downloadAndCacheTrack` success path is the natural place to track the URL for later revocation. + +## Validation + +- **Race reproduction**: this is hard to trigger deterministically. Add a temporary `console.log("[bulk]", trackId)` at the top of the download body and another in `maybeStartBulkDownload` when it returns `true`. Play a track, let it fully buffer, and confirm exactly **one** "started" log and one completion per track β€” even when forcing the buffered-range scan and `fetchSegment` to both fire (e.g. seek near the end then back). +- **Error path**: temporarily make the download throw (e.g. point the track URL at a 404) and confirm `bulkDownloadStarted` is cleared so a later attempt can retry, rather than permanently preventing caching. +- **Already-cached path**: if a track is already in IndexedDB (`loadTrackBlob` returns a URL on the first line), confirm no full download is triggered and `cachedTracks` is updated. +- **UI**: confirm the buffer bar fills, the cache indicator turns green, and queue/library re-render exactly once per completed cache (not twice). + +## Risk / Rollback + +- The refactor centralizes three near-duplicate code paths into one β€” net simplification. +- Risk: if `downloadAndCacheTrack` is called from anywhere else that relied on the internal guard, that caller must be migrated to `maybeStartBulkDownload`. Grep for `downloadAndCacheTrack(` before merging. +- If a download fails after the flag is set and the catch handler fails to clear it, the track becomes uncachable until reload. The `.catch` in `maybeStartBulkDownload` is the safety net β€” confirm it clears the flag on all rejection paths. +- Rollback = revert `audioCache.js` and the two call-site edits in `ui.js`. diff --git a/todo/fetch-response-validation/overview.md b/todo/fetch-response-validation/overview.md new file mode 100644 index 0000000..6f110ca --- /dev/null +++ b/todo/fetch-response-validation/overview.md @@ -0,0 +1,87 @@ +# Validate `fetch()` Responses Before Parsing JSON + +Priority: **Medium** Β· Effort: **Low** Β· Risk: **Low** + +## Problem + +Several client `fetch` call sites parse the response body as JSON **without checking `res.ok`** (or `res.status`). When the server returns an error envelope (e.g. `{ error: "Authentication required" }` with 401) or a non-JSON body (proxy 502, HTML error page), the parsed object is then **assigned directly to application state** and immediately rendered β€” silently corrupting the UI. + +## Affected Locations + +| File:Line | Call | Current behavior on error | +|-----------|------|---------------------------| +| `public/queue.js:382` | `M.library = await res.json();` then `M.renderLibrary()` | The entire library silently becomes the error object; render runs on garbage. | +| `public/auth.js:11` | guest session bootstrap | Error body used as the user object. | +| `public/auth.js:101` | `/api/auth/me` | Error body used as the current user. | +| `public/channelSync.js:11` | `/api/channels` list | Error body assigned to `M.channels`; `channels.length === 0` check then behaves confusingly. | + +(Other call sites in `playlists.js`, `upload.js`, etc. already do `if (!res.ok)` correctly β€” follow those as the model.) + +## Implementation Plan + +### Step 1 β€” Add a shared helper in `public/utils.js` + +Centralize the check so it can't be forgotten again: + +```js +M.apiJson = async function (res) { + if (!res.ok) { + let detail = ""; + try { detail = (await res.clone().json()).error ?? ""; } catch { /* non-JSON body */ } + const err = new Error(`Request failed (${res.status})${detail ? ": " + detail : ""}`); + err.status = res.status; + err.detail = detail; + throw err; + } + return res.json(); +}; +``` + +(`res.clone()` so the original body remains readable if the caller wants it.) + +### Step 2 β€” Update each affected call site + +**`queue.js:~380`** (library load): + +```js +const res = await fetch("/api/library", { credentials: "include" }); +if (res.status === 401) { M.handleAuthRequired?.(); return; } // or surface login UI +M.library = await M.apiJson(res); +M.renderLibrary(); +``` + +Wrap the surrounding logic in `try/catch` and show a toast on failure (e.g. `M.showToast("Couldn't load library", "error")`) rather than leaving the UI empty/silent. Keep `M.library` as its previous value on failure rather than overwriting with garbage. + +**`auth.js:11`** (guest bootstrap) and **`auth.js:101`** (`/api/auth/me`): + +```js +const res = await fetch("/api/auth/me", { credentials: "include" }); +if (res.status === 401) { /* not logged in / guest expired β€” drive login UI */ return; } +const user = await M.apiJson(res); +``` + +**`channelSync.js:11`** (channel list): + +```js +const res = await fetch("/api/channels", { credentials: "include" }); +if (!res.ok) { M.showToast("Couldn't load channels", "error"); return; } +const channels = await M.apiJson(res); +M.channels = channels; +``` + +### Step 3 β€” Audit remaining call sites + +Grep for `await res.json()` and `await response.json()` across `public/`. Convert any that lack a preceding `res.ok` / status check to use `M.apiJson`. The ones already guarded can be left or migrated for consistency. + +## Validation + +- **Simulate 401**: clear the session cookie in DevTools, reload. Confirm the library view does **not** render an error object as tracks, and a login prompt / toast appears instead. +- **Simulate 500/502**: stop the server mid-session and trigger a library reload (e.g. via the refresh path). Confirm a toast shows and the previous library remains visible. +- **Happy path**: confirm normal load of library, channels, and `/api/auth/me` still works after the change. +- **Guest expiry**: let a guest session lapse (or delete it from the DB) and reload; confirm graceful handling rather than a broken render. + +## Risk / Rollback + +- Behavior change only on the error path; success path is identical. +- One subtlety: previously-silent failures will now surface as toasts. That is the desired behavior, but verify the messages are user-friendly and not noisy in normal flaky-network conditions. +- Rollback = revert the helper and the four call sites; no data or schema impact. diff --git a/todo/websocket-robustness/overview.md b/todo/websocket-robustness/overview.md new file mode 100644 index 0000000..c07b380 --- /dev/null +++ b/todo/websocket-robustness/overview.md @@ -0,0 +1,121 @@ +# WebSocket Client Robustness: Parse Guard, Reconnect Backoff, Dead-Channel Fallback + +Priority: **High** Β· Effort: **Low–Medium** Β· Risk: **Low** + +## Problem + +The WebSocket client in `public/channelSync.js` is brittle in three ways that can leave the UI silently desynced or hammering a dead server/channel: + +1. **`JSON.parse(e.data)` has no try/catch** (`channelSync.js:262`). A single malformed or partial frame throws, the `onmessage` handler aborts, and the client stops processing all further state updates while the socket appears "open." +2. **Reconnect has no backoff, no jitter, no max-retries** (`channelSync.js:353-369`). A down server produces a fixed 2–3s reconnect storm with no cap. +3. **No fallback to the default channel**. If the current channel is deleted server-side while the client is disconnected, every reconnect 404s and the client loops forever against a dead channel ID. + +Separately, the message dispatcher falls through to `M.handleUpdate(data)` for any unknown `data.type` (`channelSync.js:346`), so a new server message type silently corrupts state. Worth hardening in the same pass. + +## Affected Locations + +- `public/channelSync.js:262` β€” unguarded `JSON.parse` +- `public/channelSync.js:346` β€” `handleUpdate` fallback for unknown types +- `public/channelSync.js:353-369` β€” reconnect logic +- `public/channelSync.js:245-261` β€” socket setup (reference for structure) + +## Implementation Plan + +### Step 1 β€” Wrap `JSON.parse` in try/catch + +```js +M.ws.onmessage = (e) => { + let data; + try { + data = JSON.parse(e.data); + } catch (err) { + console.warn("[channelSync] Dropping malformed WS frame:", err); + return; + } + // ... existing dispatch ... +}; +``` + +Keep behavior: a bad frame is dropped, the socket stays open, subsequent good frames still process. + +### Step 2 β€” Strict dispatch with explicit unknown-type handling + +Replace the implicit fall-through to `M.handleUpdate(data)`. Make the dispatch explicit: + +```js +switch (data.type) { + case "channel_list": /* ... */ break; + case "switched": /* ... */ break; + case "track": /* fallthrough */ + case "state": M.handleUpdate(data); break; // explicit state-shape types only + // ... all other known types ... + default: + console.warn("[channelSync] Unknown WS message type:", data.type, data); +} +``` + +(If the server currently sends `type` values other than the ones handled, gather the full set first via `grep` for `"type":` / `type: "` in the server broadcast paths and list them in this switch.) + +### Step 3 β€” Reconnect with exponential backoff + jitter + max attempts + +Track reconnect state on `M` (e.g. `M.wsReconnect = { attempts: 0, timer: null }`). On close: + +```js +M.ws.onclose = () => { + M.ws = null; + // clear any user-facing "connected" state + if (!M.wantSync) return; + + const recon = M.wsReconnect; + recon.attempts++; + const base = 1000; // 1s + const cap = 30000; // 30s max + const exp = Math.min(cap, base * 2 ** recon.attempts); + const jitter = Math.random() * 500; // 0–500ms + const delay = exp + jitter; + + recon.timer = setTimeout(() => { + connectChannel(M.currentChannelId); + }, delay); +}; +``` + +Reset `recon.attempts = 0` on a successful `open`. Cancel `recon.timer` on any explicit/manual `connectChannel` call to avoid double-connects. + +**On max attempts**: rather than giving up entirely (a music app should keep trying), cap the delay at 30s but keep retrying. Optionally surface a "reconnecting…" indicator after the first few failures so the user knows the stream is stale. + +### Step 4 β€” Dead-channel fallback + +In `connectChannel`, if the upgrade/open fails or the server responds with a channel-not-found error message (the server already sends `{ type: "error", message: "Channel not found" }` per `websocket.ts:50`), fall back to the default channel: + +```js +// inside onmessage error handler: +if (data.type === "error" && /not found/i.test(data.message)) { + console.warn(`[channelSync] Channel ${M.currentChannelId} not found, falling back to default`); + // fetch default channel id from /api/channels (isDefault === true) + const def = await fetchDefaultChannelId(); + if (def && def !== M.currentChannelId) { + M.currentChannelId = def; + M.saveChannelId(def); + connectChannel(def); + M.showToast("This channel no longer exists β€” switched to the default channel."); + return; + } +} +``` + +Add a small helper `fetchDefaultChannelId()` that GETs `/api/channels` and returns the `id` where `isDefault === true`. Cache it on `M.defaultChannelId` after first successful list load so we don't re-fetch on every failure. + +## Validation + +- **Malformed frame**: from server console or a test, `ws.send("not json")` to a client β€” confirm the client logs a warning and continues processing subsequent valid frames (state still updates). +- **Server kill / restart**: stop the server, confirm reconnect attempts grow with backoff (log each attempt + delay) rather than firing every 2s. Restart, confirm the client reconnects and `attempts` resets. +- **Channel deletion while disconnected**: note a non-default channel ID, stop server, delete the channel row from `blastoise.db`, restart server, ensure the client reconnects β†’ gets "not found" β†’ falls back to the default channel and shows the toast. +- **Dispatch**: inject an unknown `{ type: "future_thing" }` message server-side (or via a debug WS send) and confirm it logs a warning and does **not** call `handleUpdate`. + +## Risk / Rollback + +- All changes are additive hardening; normal-path behavior is preserved. +- Backoff introduces up to a 30s worst-case reconnect delay β€” acceptable and better than a tight loop. If a deployment wants faster recovery, tune `base`/`cap`. +- Default-channel fallback adds one HTTP fetch on the error path only; cache the result to avoid repeats. +- Rollback = revert the file; no schema or wire-protocol change. diff --git a/todo/ws-guest-control-permission/overview.md b/todo/ws-guest-control-permission/overview.md new file mode 100644 index 0000000..48739ae --- /dev/null +++ b/todo/ws-guest-control-permission/overview.md @@ -0,0 +1,74 @@ +# Fix WS Control-Permission Bypass for Guests + +Priority: **High** Β· Effort: **Trivial (one location)** Β· Risk: **Low** + +## Problem + +Guests are *intended* to be unable to control playback (`routes/helpers.ts:30`: + +```ts +if (user.is_guest && permission === "control") return false; +``` + +) but the **WebSocket message handler** does not route through `userHasPermission`. It re-implements the check inline and **omits the guest guard**: + +```ts +// websocket.ts:87-93 +const canControl = user.is_admin + || config.defaultPermissions?.includes("control") + || hasPermission(userId, "channel", ws.data.channelId, "control"); +if (!canControl) { ... return; } +``` + +Because the default config ships `defaultPermissions: ["listen", "control"]` (`config.json:5-8`), **any guest can `pause`, `unpause`, `seek`, and `jump` on any channel**, affecting all listeners. + +The HTTP control endpoints (`routes/channels.ts:176, 196, 216, 261`) correctly use `userHasPermission`, so this is an inconsistency between the two code paths that reach the same `Channel` mutators. + +## Affected Location + +- `websocket.ts:86-93` β€” inline permission check, guest-unaware. + +## Root Cause + +Duplicated permission logic in two places. The WS handler was written before / diverged from `helpers.userHasPermission`. + +## Implementation Plan + +### Step 1 β€” Import `userHasPermission` + +At the top of `websocket.ts`: + +```ts +import { userHasPermission } from "./routes/helpers"; +``` + +### Step 2 β€” Replace the inline check + +Replace `websocket.ts:86-93` with: + +```ts +if (!userHasPermission(user, "channel", ws.data.channelId, "control")) { + console.log("[WS] User lacks control permission:", user.username, "(guest=" + user.is_guest + ")"); + return; +} +``` + +`userHasPermission` already encapsulates: admin bypass, the guest `control` denial (`helpers.ts:30`), the `defaultPermissions` config check, and the DB permission lookup. Routing through it makes WS and HTTP behavior identical. + +### Step 3 β€” Verify no other call sites rely on the old behavior + +`grep` the repo for the inline pattern (`config.defaultPermissions?.includes("control")`) β€” it should now appear only in `helpers.ts:33` (the canonical check). Any other duplicate should be replaced the same way. + +## Validation + +- **Guest cannot control**: sign in as a guest (or hit the server unauthenticated with `allowGuests: true`), open a channel WS, send `{"action":"pause"}`. Confirm the channel does **not** pause and the server logs the "lacks control permission" line. +- **Non-guest user still controls**: sign in as `test`/`testuser`, send the same action, confirm the channel pauses. +- **Admin controls**: sign in as admin, confirm control works on channels the admin does not own. +- **Guest still receives state**: confirm the guest client still gets `track`/state broadcasts (i.e. the *listen* path is unaffected β€” only `control` is denied). +- **Config matrix**: temporarily set `defaultPermissions` to `["listen"]` only and confirm a *non-guest* non-admin user is now also denied control via WS (matches the HTTP path). + +## Risk / Rollback + +- The only behavioral change is denying control to guests (and, if `defaultPermissions` lacks `"control"`, to non-admin users) β€” which is the documented intent. If any deployment currently relies on guests controlling playback, that was an unintentional privilege and should be re-granted explicitly via the permissions table, not by reverting. +- No schema or DB change. +- Rollback = revert the single edit. diff --git a/todo/xss-fixes/overview.md b/todo/xss-fixes/overview.md new file mode 100644 index 0000000..cfe9a26 --- /dev/null +++ b/todo/xss-fixes/overview.md @@ -0,0 +1,98 @@ +# XSS Sweep: Escape Server-Controlled Data at `innerHTML` Boundaries + +Priority: **Critical** Β· Effort: **Low** Β· Risk: **Low** + +## Problem + +The client injects server-controlled strings into the DOM via `innerHTML` **without escaping** in several high-traffic render paths. Because the server broadcasts channel names, listener usernames, toast messages, and track titles over WebSocket to *every* connected client, a single malicious payload is a **stored/reflected XSS propagated peer-to-peer**. + +There is also **no Content-Security-Policy** header set on the document or by the server, so once injected, script runs with full page privilege (same-origin as the session cookie). + +## Affected Locations + +| File:Line | Sink | Source | +|-----------|------|--------| +| `public/channelSync.js:174-183` | channel list `${ch.name}` and `` | channel name (server) | +| `public/channelSync.js:161-163` | `listenersHtml` (listener usernames) | `ch.listeners` (server) | +| `public/utils.js:113` | toast history `... ${item.message}` | WS `toast` message (server) | +| `public/utils.js:149,159` | track title marquee | `track.title` (file metadata / yt-dlp) | +| `public/queue.js:342` | now-playing bar `${title}` | `track.title` (server) | +| `public/upload.js:307` | slow-queue list `${group.name}` | playlist name (server) | +| `public/upload.js:330` | slow-queue list `${item.title}` | `/api/fetch` response (server) | + +Note: `trackComponent.js:59` and `playlists.js:44,57,58` **do** escape correctly today. The codebase is internally inconsistent β€” those are the model to follow. + +## Root Cause + +- No single shared escaping utility. `escapeHtml` is defined **twice** (`public/playlists.js:481-486` and `public/trackComponent.js:74-79`) as local copies. +- No lint rule or review guard preventing raw `${serverData}` inside template literals feeding `innerHTML`. +- No CSP as defense-in-depth. + +## Implementation Plan + +### Step 1 β€” Create one shared `escapeHtml` in `public/utils.js` + +Move/deduplicate the existing helper into `utils.js` and expose it on `M`: + +```js +M.escapeHtml = function (str) { + if (str == null) return ""; + return String(str) + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +}; +``` + +Delete the two local copies in `playlists.js:481` and `trackComponent.js:74`; replace callers with `M.escapeHtml(...)`. + +### Step 2 β€” Escape every server-sourced value at each sink above + +For each location, wrap the interpolated value in `M.escapeHtml(...)`. For attribute contexts (e.g. `value="${...}"`), escaping with the helper above is sufficient since it includes `"`. + +Worked example for `channelSync.js:174-183`: + +```js +div.innerHTML = ` +
+ ${M.escapeHtml(ch.name)} + + ... +
${listenersHtml}
+
+`; +``` + +And `listenersHtml` itself (built at `channelSync.js:161-163`) must escape each username before joining. + +### Step 3 β€” Add a defense-in-depth CSP header + +In the static file route handler (`routes/static.ts`) β€” or centrally where index.html is served β€” add: + +``` +Content-Security-Policy: default-src 'self'; script-src 'self'; connect-src 'self' ws: wss:; media-src 'self' blob:; img-src 'self' data: blob:; style-src 'self' 'unsafe-inline' +``` + +(`'unsafe-inline'` for styles only β€” needed until stylesheets are consolidated. No `'unsafe-inline'` for scripts.) + +Confirm this does not break the blob-URL audio playback (`media-src 'self' blob:`) or WebSocket (`connect-src ... ws: wss:`). + +### Step 4 β€” (Optional, recommended) Centralize user-visible rendering + +Longer-term, every track row / title render should go through `trackComponent.js`'s pure renderer which already escapes. Route now-playing-bar and marquee titles through the same path so escaping can't be forgotten again. + +## Validation + +- **Manual payload test**: create a channel named ``, connect a second client, confirm no alert fires and the name renders literally. +- **Username payload**: set a username (or guest) containing `