mirror of https://github.com/Nezreka/SoulSync.git
A mirrored playlist named with an apostrophe (e.g. "Road trip-The
Rolfe's") rendered dead action buttons. _escAttr HTML-escapes ' to ',
but it was used to inject the name into a single-quoted JS string inside an
inline onclick. The HTML parser decodes ' back to a bare ' BEFORE the JS
parser runs, producing an unterminated string literal -> SyntaxError -> the
whole handler fails to compile.
Two symptoms (both reproduced with the real name + the literal line-524
onclick template): clicking the X delete never ran event.stopPropagation(),
so the click bubbled to the card and opened the track preview instead; and
the preview's "Delete Mirror" silently did nothing (no DELETE request, no
log). Plain names ("Classic Rock") were unaffected, which is why it looked
intermittent.
Add a dedicated _escJs() that backslash-escapes the JS metacharacters (\, ')
first, then HTML-escapes the attribute-breaking chars - correct for a
single-quoted JS string inside a double-quoted HTML attribute. Convert all 16
inline-onclick string-argument sites to it: mirrored card (clear/Auto-Sync/
link/delete) and preview modal, plus the same latent bug in pool Fix Match /
Rematch, group bulk-toggle/rename/delete, and automation history/group/delete.
Genuine HTML-attribute usages (class/value/data-*/title/option) stay on
_escAttr where it is correct.
Tests: tests/static/test_stats_automations_esc.mjs extracts the real _escJs/
_escAttr from source and asserts apostrophe + quote/backslash/&/<> names
round-trip through HTML+JS decoding, documents that _escAttr throws a
SyntaxError for the apostrophe case while _escJs compiles clean, and pins
wolf39's exact name. pytest shim tests/test_stats_automations_esc_js.py runs
it under node --test (skips if node<22 / absent).
pull/761/head
BoulderBadgeDad
2 weeks ago
parent
22f30d3f40
commit
ffb9249ded
@ -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('<value>')" : 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',
|
||||
'<script>alert(1)</script>',
|
||||
"O'Brien \\ \"x\" & <y>",
|
||||
"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));
|
||||
});
|
||||
});
|
||||
@ -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,
|
||||
)
|
||||
Loading…
Reference in new issue