diff --git a/tests/static/test_stats_automations_esc.mjs b/tests/static/test_stats_automations_esc.mjs new file mode 100644 index 00000000..323a0ac4 --- /dev/null +++ b/tests/static/test_stats_automations_esc.mjs @@ -0,0 +1,124 @@ +// Tests for the inline-onclick string escaping in `webui/static/stats-automations.js`. +// +// Run via: +// node --test tests/static/test_stats_automations_esc.mjs +// +// The pytest wrapper at `tests/test_stats_automations_esc_js.py` shells out to +// `node --test` so this fails the suite if the escaping regresses. +// +// Regression context — the "Road trip-The Rolfe's" delete bug: +// A mirrored playlist whose name contains an apostrophe rendered +// onclick="...deleteMirroredPlaylist(2, 'Road trip-The Rolfe's')" +// The browser HTML-decodes ' back to ' BEFORE the JS parser runs, producing +// deleteMirroredPlaylist(2, 'Road trip-The Rolfe's') // unterminated string +// so the whole handler threw a SyntaxError and never executed. Two visible +// symptoms: (1) event.stopPropagation() never ran, so clicking the ✕ bubbled to +// the card and opened the track preview instead; (2) the preview's "Delete +// Mirror" button silently did nothing (no DELETE request). `_escJs` fixes it by +// backslash-escaping the JS metacharacters first; `_escAttr` (HTML-only) cannot. + +import { test, describe } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const SRC = readFileSync( + resolve(__dirname, '..', '..', 'webui', 'static', 'stats-automations.js'), + 'utf8', +); + +// Pull a top-level `function NAME(...) { ... }` out of the real source by +// brace-matching and return it as a live function, so the tests exercise the +// shipped implementation rather than a copy. +function extractFn(name) { + const at = SRC.indexOf(`function ${name}(`); + assert.notEqual(at, -1, `function ${name} must exist in stats-automations.js`); + const open = SRC.indexOf('{', at); + let depth = 0; + for (let i = open; i < SRC.length; i++) { + if (SRC[i] === '{') depth++; + else if (SRC[i] === '}' && --depth === 0) { + // eslint-disable-next-line no-eval + return eval(`(${SRC.slice(at, i + 1)})`); // named function expression -> returns the fn + } + } + throw new Error(`could not brace-match function ${name}`); +} + +const _escJs = extractFn('_escJs'); +const _escAttr = extractFn('_escAttr'); + +// Reproduce what a browser does to onclick="fn('')" : the HTML parser +// resolves entities, THEN the JS engine parses the resulting source. & is +// resolved last so already-decoded entities aren't double-processed. +function htmlAttrDecode(s) { + return s + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/&/g, '&'); +} + +// Build the exact onclick shape the cards build, decode it like the browser +// would, compile + run it, and capture the argument the handler received. +function invokeOnclick(escFn, name) { + const onclick = `cb(2, '${escFn(name)}')`; + const decoded = htmlAttrDecode(onclick); + let received; + const cb = (_id, n) => { received = n; }; + // eslint-disable-next-line no-new-func + new Function('cb', decoded)(cb); // compile + execute the handler like the DOM does + return received; +} + +describe('_escJs — onclick string args survive HTML+JS decoding', () => { + test("apostrophe name (the Rolfe's delete bug) round-trips intact", () => { + const name = "Road trip-The Rolfe's"; + assert.equal(invokeOnclick(_escJs, name), name); + }); + + test('quotes, backslashes, ampersands, angle brackets all round-trip', () => { + for (const name of [ + "Guns N' Roses", + 'He said "hi"', + 'back\\slash', + 'Tom & Jerry', + '', + "O'Brien \\ \"x\" & ", + "it's a \"test\" & more", + ]) { + assert.equal( + invokeOnclick(_escJs, name), name, + `round-trip failed for ${JSON.stringify(name)}`, + ); + } + }); + + test('plain names are not over-escaped (identical to input)', () => { + assert.equal(_escJs('Classic Rock'), 'Classic Rock'); + assert.equal(_escJs('Discover Weekly'), 'Discover Weekly'); + }); + + test('empty / falsy input yields empty string', () => { + assert.equal(_escJs(''), ''); + assert.equal(_escJs(null), ''); + assert.equal(_escJs(undefined), ''); + }); +}); + +describe('regression: _escAttr is unsafe for the onclick JS-string context', () => { + test('apostrophe name compiles to a SyntaxError under _escAttr (the original bug)', () => { + const decoded = htmlAttrDecode(`cb(2, '${_escAttr("Road trip-The Rolfe's")}')`); + // eslint-disable-next-line no-new-func + assert.throws(() => new Function('cb', decoded), SyntaxError); + }); + + test('_escJs compiles cleanly for that same name', () => { + const decoded = htmlAttrDecode(`cb(2, '${_escJs("Road trip-The Rolfe's")}')`); + // eslint-disable-next-line no-new-func + assert.doesNotThrow(() => new Function('cb', decoded)); + }); +}); diff --git a/tests/test_stats_automations_esc_js.py b/tests/test_stats_automations_esc_js.py new file mode 100644 index 00000000..6a8ae71b --- /dev/null +++ b/tests/test_stats_automations_esc_js.py @@ -0,0 +1,72 @@ +"""Run the JS escaping tests for `webui/static/stats-automations.js` under the +regular pytest sweep. + +The actual contract tests live in `tests/static/test_stats_automations_esc.mjs` +and run via Node.js's stable built-in test runner (`node --test`). This shim +shells out to that runner and asserts a clean exit so the JS tests fail the +suite if the inline-onclick escaping (`_escJs`) regresses — e.g. a playlist or +automation named with an apostrophe silently breaking its action buttons. + +Skipped when: + - `node` isn't on PATH (e.g. Python-only dev container). + - Node version < 22 (the assert-flavor used by the test is 22+). + +Run directly: + node --test tests/static/test_stats_automations_esc.mjs +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_TEST_FILE = _REPO_ROOT / "tests" / "static" / "test_stats_automations_esc.mjs" + + +def _node_available() -> bool: + if not shutil.which("node"): + return False + try: + result = subprocess.run( + ["node", "--version"], + capture_output=True, text=True, timeout=10, + ) + except (subprocess.SubprocessError, FileNotFoundError): + return False + if result.returncode != 0: + return False + raw = (result.stdout or "").strip().lstrip("v") + try: + major = int(raw.split(".")[0]) + except (ValueError, IndexError): + return False + return major >= 22 + + +def test_stats_automations_esc_js(): + """Pin the inline-onclick escaping contract via `node --test`.""" + if not _node_available(): + pytest.skip("Node.js >= 22 required to run the JS escaping tests") + + if not _TEST_FILE.exists(): + pytest.skip(f"JS test file missing: {_TEST_FILE}") + + result = subprocess.run( + ["node", "--test", str(_TEST_FILE)], + capture_output=True, text=True, + cwd=str(_REPO_ROOT), + timeout=60, + ) + + if result.returncode != 0: + pytest.fail( + "JS stats-automations escaping tests failed:\n\n" + f"--- stdout ---\n{result.stdout}\n" + f"--- stderr ---\n{result.stderr}", + pytrace=False, + ) diff --git a/webui/static/stats-automations.js b/webui/static/stats-automations.js index b46cbd1c..e16397b7 100644 --- a/webui/static/stats-automations.js +++ b/webui/static/stats-automations.js @@ -518,10 +518,10 @@ function renderMirroredCard(p, container) { ${phaseHtml} - ${disc > 0 ? `` : ''} - - - + ${disc > 0 ? `` : ''} + + + `; card.addEventListener('click', () => { const st = youtubePlaylistStates[hash]; @@ -868,11 +868,11 @@ async function openMirroredPlaylistModal(playlistId) {
- +
- - + +
@@ -1176,7 +1176,7 @@ function renderPoolList() { ${_esc(t.playlist_name)}
- + `).join(''); } else { @@ -1218,7 +1218,7 @@ function renderPoolList() { ${conf}% ${e.use_count}× - + `; @@ -2141,13 +2141,13 @@ function _buildAutomationSection(id, label, automations, useGrid, options = {}) const allEnabled = enabledCount === automations.length; actionsHtml = `
- - -
@@ -3108,7 +3108,7 @@ function _showGroupDropdown(event, autoId, currentGroup) { } allGroups.forEach(g => { const isActive = g === currentGroup; - html += `
${_esc(g)}
`; + html += `
${_esc(g)}
`; }); if (allGroups.size) html += '
'; html += ``; @@ -3204,15 +3204,15 @@ function renderAutomationCard(a) { const _timerTriggers = ['schedule', 'daily_time', 'weekly_time']; if (a.next_run && a.enabled && _timerTriggers.includes(a.trigger_type)) metaParts.push('Next: ' + _autoTimeUntil(a.next_run) + ''); if (!_timerTriggers.includes(a.trigger_type) && a.enabled) metaParts.push('Listening'); - if (a.run_count) metaParts.push('Runs: ' + a.run_count + ''); + if (a.run_count) metaParts.push('Runs: ' + a.run_count + ''); if (a.last_error) metaParts.push('Error: ' + _esc(a.last_error)); const dupeBtn = a.is_system ? '' : ``; const groupBtn = a.is_system ? '' : - ``; + ``; const deleteBtn = a.is_system ? '' : - ``; + ``; card.innerHTML = `
@@ -4660,6 +4660,29 @@ function _escAttr(str) { return String(str).replace(/&/g, '&').replace(/"/g, '"').replace(/'/g, ''').replace(//g, '>'); } +// Escape a string for use INSIDE a single-quoted JS string literal that itself +// sits inside a double-quoted HTML attribute, e.g. onclick="fn('')". +// Such a value is decoded twice: first the HTML attribute parser resolves +// entities, then the JS parser reads the string. _escAttr only HTML-escapes, so +// an apostrophe becomes ' which the attribute parser turns back into a bare +// ' — terminating the JS string and throwing a SyntaxError that silently kills +// the whole handler (this is the "Road trip-The Rolfe's" delete-button bug). +// Fix: backslash-escape the JS metacharacters (\ and ') FIRST so they survive +// HTML decoding intact, then HTML-escape the characters that would otherwise +// break the surrounding attribute. Order matters — & is escaped before the +// later replacements insert their own entities. +function _escJs(str) { + if (!str) return ''; + return String(str) + .replace(/\\/g, '\\\\') + .replace(/'/g, "\\'") + .replace(/\r?\n/g, ' ') + .replace(/&/g, '&') + .replace(/"/g, '"') + .replace(//g, '>'); +} + // ===== ENHANCE QUALITY MODAL ===== let _enhanceQualityData = null;