diff --git a/cmd/server/perf_slowpath_392_test.go b/cmd/server/perf_slowpath_392_test.go index 7fc1d0087..b06278c9d 100644 --- a/cmd/server/perf_slowpath_392_test.go +++ b/cmd/server/perf_slowpath_392_test.go @@ -105,4 +105,17 @@ func TestNormalizeSlowPath(t *testing.T) { t.Fatalf("control chars survived in the key half: %q", got) } }) + + // #393 N4: the strip covers DEL (0x7f) as well as C0 (< 0x20); only the + // C0 half was pinned. Neighbouring '~' (0x7e) must survive. Mutant: drop + // `|| r == 0x7f` → red. + t.Run("DEL is stripped from both halves, '~' is kept", func(t *testing.T) { + if got := sanitizeSlowPathComponent("a\x7fb~"); got != "ab~" { + t.Fatalf("sanitizeSlowPathComponent: got %q, want %q", got, "ab~") + } + got := normalizeSlowPath("/api/\x7f:id", "/api/x\x7fy") + if got != "/api/:id /api/xy" { + t.Fatalf("normalizeSlowPath: got %q, want %q", got, "/api/:id /api/xy") + } + }) } diff --git a/docs/api-spec.md b/docs/api-spec.md index 53f08f2a5..d8a62ec3e 100644 --- a/docs/api-spec.md +++ b/docs/api-spec.md @@ -315,8 +315,9 @@ Server health and telemetry. Used by monitoring. "recentSlow": [ // last 5 { // Same SlowQuery shape as GET /api/perf → slowQueries[]: normalized - // route key + a sanitised, control-char-stripped, ≤128-rune-per-half - // copy of the raw request path, joined by a space (e.g. "/api/nodes/:id + // route key + a sanitised, control-char-stripped copy of the raw + // request path (each half ≤128 content runes plus an "…" truncation + // marker), joined by a space (e.g. "/api/nodes/:id // /api/nodes/abcd1234"); unmatched routes emit the capped raw path // once. Not HTML-escaped — clients MUST escape before DOM insertion // (#392, #394 F3/F4/F6). @@ -358,8 +359,9 @@ Detailed performance metrics per endpoint. // Normalized route key + a sanitised copy of the raw request path, // joined by a single space, e.g. "/api/nodes/:id /api/nodes/abcd1234". // BOTH halves are independently control-char-stripped (incl. CR/LF/TAB) - // and capped to ≤128 runes each (an "…" marks truncation), so the whole - // value is bounded at ≤257 runes regardless of input (#392, #394 F3/F4). + // and capped to ≤128 content runes each (an "…" marks truncation), so + // the whole value is bounded at ≤259 runes (2×128 content runes + two + // "…" markers + the join space) regardless of input (#392, #394 F3/F4). // Unmatched routes (key == raw) emit the capped raw path once. Not // HTML-escaped — clients MUST escape before inserting into the DOM. "path": string, diff --git a/public/analytics.js b/public/analytics.js index 9abe5751d..9f6cab8b6 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -1102,7 +1102,7 @@ let html = '
'; repeaters.slice(0, 15).forEach(r => { const pct = (r.count / max * 100).toFixed(0); - html += `
+ html += `
${r.name ? '' + esc(r.name) + '' : '' + r.hop + ''}
${r.count.toLocaleString()}
diff --git a/public/compare.js b/public/compare.js index 9753f17ce..4a2fa602f 100644 --- a/public/compare.js +++ b/public/compare.js @@ -119,6 +119,17 @@ if (typeof window !== 'undefined') { location.hash = row.dataset.value; }); + // #393 N9 — delegated click navigation for the same rows. Lets the packet + // rows drop their per-row inline onclick (consistency with #373), which + // also removes the latent `'`-breakout in that double-quoted/JS context. + // Bound to #compareContent, not the shared #app: the next page's + // app.innerHTML discards it, so it cannot fire on other pages' rows. + document.getElementById('compareContent').addEventListener('click', function (e) { + var row = e.target.closest('tr[data-action="navigate"]'); + if (!row) return; + location.hash = row.dataset.value; + }); + loadObservers(); } @@ -526,7 +537,7 @@ if (typeof window !== 'undefined') { } else { obsLabel = nameB; } - return '' + + return '' + '' + escapeHtml(h.substring(0, 12)) + '' + '' + timeAgo(p.timestamp || p.first_seen) + '' + '' + escapeHtml(typeName) + '' + diff --git a/scripts/check-xss-sinks.sh b/scripts/check-xss-sinks.sh index 01a0ae30d..44ece4501 100755 --- a/scripts/check-xss-sinks.sh +++ b/scripts/check-xss-sinks.sh @@ -44,13 +44,16 @@ # per-line audit finds no interpolation there, and the `html += `…`` lines # are not recognised as sinks. The gate therefore reports 0 findings on the # pre-#392 perf.js (verified: `--file` on the pre-fix file exits 0) even -# though `${path}` / `${q.path}` / `${k}` were live XSS. A SECOND reason the -# gate can't flag them even if it saw the line: those values flow through -# generically-named locals (`path`, `q.path`, the write-source key `k`) that -# are NOT in the node-controlled allowlist and cannot be added without a -# false-positive explosion (`path` and especially single-letter `k` appear -# all over public/*.js). Extending detection here would also diverge from -# the canonical skill-side mirror and its shared allowlist. The #392 sinks +# though `${path}` / `${q.path}` / `${k}` were live XSS. Those values flow +# through generically-named locals (`path`, `q.path`, the write-source key +# `k`) that are NOT in the node-controlled allowlist. Adding them to the +# allowlist was measured (#394 F5 re-review): across all public/**/*.{js,html} +# it produces exactly ONE extra finding (route-view.js, a genuine false +# positive where `k` is a .map() index) — not an explosion. So the allowlist +# is not the real obstacle; the accumulator shape above is, and it hides the +# pre-#392 perf.js sinks regardless of the allowlist. Extending detection here +# would also diverge from the canonical skill-side mirror and its shared +# allowlist, so the allowlist is left unchanged. The #392 sinks # are instead pinned by DOM-grep unit tests (test-xss-escape-sinks.js, the # `#392`/`#394` cases) plus a browser E2E (test-xss-392-perf-e2e.js), each # with a mutant, which is the regression guard for this file. diff --git a/test-all.sh b/test-all.sh index 2c76ff975..eb0b5755e 100755 --- a/test-all.sh +++ b/test-all.sh @@ -198,6 +198,7 @@ run test-reach-rank.js run test-issue-1107-live-layout.js run test-issue-1619-feed-detail-card-draggable.js run test-xss-escape-sinks.js +run test-issue-393-slowpath-doc-bound.js run test-preflight-xss-gate.js run test-issue-1648-m4-emoji-scan.js run test-issue-1753-copy-url-slash.js diff --git a/test-issue-1640-compare-discovery-e2e.js b/test-issue-1640-compare-discovery-e2e.js index c67b301a9..bb56ba3d4 100644 --- a/test-issue-1640-compare-discovery-e2e.js +++ b/test-issue-1640-compare-discovery-e2e.js @@ -163,6 +163,56 @@ async function run() { assert(valB === idB, 'compareObsB should be pre-selected to b=, got ' + valB); }); + // ── #393 F1: compare's row-click listener must not outlive the page ── + // compare.js once bound its delegated row click to the shared #app + // container and never removed it, so after one compare visit it fired on + // every later page's tr[data-action="navigate"]. On #/observers at + // <=1023px the observers handler opens the #1056 AC#4 slide-over and calls + // preventDefault(); the leaked compare handler navigated anyway. Drive the + // real SPA flow (hash navigation, no reload) home → compare → observers. + const narrow = await browser.newContext({ viewport: { width: 800, height: 800 } }); + const np = await narrow.newPage(); + np.setDefaultTimeout(15000); + np.on('pageerror', e => console.error(' pageerror:', e.message)); + + await step('#393 F1 compare row click still navigates to the packet', async () => { + await np.goto(BASE + '/#/home', { waitUntil: 'domcontentloaded' }); + await np.evaluate(([a, b]) => { location.hash = '#/compare?a=' + a + '&b=' + b; }, [idA, idB]); + await np.waitForSelector('#compareContent [data-cview]', { timeout: 20000 }); + // Open the first non-empty packet list (fixture overlap varies). + const view = await np.$$eval('#compareContent [data-cview]', btns => { + const b = btns.find(x => x.dataset.cview !== 'summary' && /\(([1-9]\d*)\)/.test(x.textContent)); + return b ? b.dataset.cview : null; + }); + assert(view, 'fixture observers ' + idA + '/' + idB + ' have no packets to list'); + await np.click('#compareContent [data-cview="' + view + '"]'); + const row = await np.waitForSelector('#compareDetail tr[data-action="navigate"]'); + const target = await row.getAttribute('data-value'); + await row.click(); + await np.waitForFunction(t => location.hash === t, target, { timeout: 5000 }); + }); + + await step('#393 F1 after compare, an observers row opens the slide-over and does not navigate', async () => { + await np.evaluate(() => { location.hash = '#/observers'; }); + await np.waitForSelector('#obsTable tbody tr[data-action="navigate"]'); + await np.evaluate(() => { + window.__f1HashChanges = 0; + window.addEventListener('hashchange', () => { window.__f1HashChanges++; }); + }); + await np.click('#obsTable tbody tr[data-action="navigate"] td:not(:empty)'); + await np.waitForFunction(() => { + const p = document.querySelector('.slide-over-panel'); + return p && !p.hidden; + }, null, { timeout: 10000 }); + // Give a leaked handler's navigation time to land before asserting. + await np.waitForTimeout(500); + const st = await np.evaluate(() => ({ hash: location.hash, changes: window.__f1HashChanges })); + assert(st.hash === '#/observers', + 'observers row click navigated after a compare visit (leaked compare listener): ' + st.hash); + assert(st.changes === 0, 'expected no hashchange on a slide-over row click, got ' + st.changes); + }); + + await narrow.close(); await browser.close(); console.log('\n' + passed + ' passed, ' + failed + ' failed'); if (failed) process.exit(1); diff --git a/test-issue-393-slowpath-doc-bound.js b/test-issue-393-slowpath-doc-bound.js new file mode 100644 index 000000000..2d650e4dd --- /dev/null +++ b/test-issue-393-slowpath-doc-bound.js @@ -0,0 +1,51 @@ +/** + * test-issue-393-slowpath-doc-bound.js + * + * #393 N1 — docs/api-spec.md must document the slow-path bound that the Go + * code actually enforces. `sanitizeSlowPathComponent` caps each half at + * `slowPathRawCap` CONTENT runes and then appends one "…" ellipsis when it + * truncates; `normalizeSlowPath` joins two halves with a single space. So: + * + * per half = slowPathRawCap content runes + 1 ellipsis + * total = 2*slowPathRawCap + 2 ellipses + 1 join space = 2*cap + 3 + * + * With cap = 128 that is 259, not the 257 the spec documented (and the + * "≤128-rune-per-half" phrasing ignored the ellipsis entirely). This pins the + * doc against the real constant so the drift cannot silently return. + */ +'use strict'; +const fs = require('fs'); +const assert = require('assert'); + +let passed = 0, failed = 0; +function test(name, fn) { + try { fn(); passed++; console.log(' ✅ ' + name); } + catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } +} + +const routes = fs.readFileSync('cmd/server/routes.go', 'utf8'); +const capM = routes.match(/const slowPathRawCap = (\d+)/); +const cap = capM ? Number(capM[1]) : null; +const total = cap != null ? 2 * cap + 3 : null; // 2*cap + 2 ellipses + 1 space +const spec = fs.readFileSync('docs/api-spec.md', 'utf8'); + +test('#393 N1: slowPathRawCap is readable from cmd/server/routes.go', () => { + assert.ok(cap != null, 'could not read `const slowPathRawCap = N` from routes.go'); +}); + +test('#393 N1: api-spec documents the correct total bound (≤' + total + ' runes)', () => { + assert.ok(spec.includes('≤' + total + ' runes'), + 'api-spec.md must document the total slow-path bound as ≤' + total + ' runes'); + assert.ok(!spec.includes('≤257 runes'), + 'api-spec.md still documents the stale ≤257-rune total (off by two)'); +}); + +test('#393 N1: api-spec per-half wording accounts for the truncation ellipsis', () => { + // The per-half copy is `cap` content runes PLUS the "…" marker, so the old + // "≤128-rune-per-half" phrasing was off by one and must be gone. + assert.ok(!spec.includes('≤128-rune-per-half'), + 'api-spec.md still says "≤128-rune-per-half" (ignores the ellipsis → off by one)'); +}); + +console.log('\n#393 slow-path doc bound: ' + passed + ' passed, ' + failed + ' failed'); +if (failed > 0) process.exit(1); diff --git a/test-xss-escape-sinks.js b/test-xss-escape-sinks.js index c50d8631c..d970cc6cc 100644 --- a/test-xss-escape-sinks.js +++ b/test-xss-escape-sinks.js @@ -67,15 +67,26 @@ const escapeHtml = loadEscapeHtml(); // #333 F5: sinks that call a file-local escaper must be tested with THAT // escaper, not with app.js's escapeHtml, or a weakened local escaper goes // unnoticed. Lift the one-line `function esc(s) { … }` from the file. +// #393 N10: count EVERY declaration named esc, not just `function esc(`. +// #333 N4 only counted the function form, so a differently-declared second +// escaper (`const esc = …`, `let`/`var`) slipped past the guard and could +// silently retarget the contract test. Count both shapes. +function countEscDefs(src) { + const fnDefs = (src.match(/function\s+esc\s*\(/g) || []).length; + const bindDefs = (src.match(/\b(?:const|let|var)\s+esc\s*=/g) || []).length; + return fnDefs + bindDefs; +} + function loadLocalEsc(file, ctxExtra) { const src = fs.readFileSync(file, 'utf8'); - // #333 N4: the regex below lifts the *first* one-line `function esc(s)`. - // If a second file-local esc were added above the real sink's escaper, it - // would silently retarget this contract test. Pin the assumption: there - // must be exactly one esc() definition in the file, else fail loudly. - const defs = src.match(/function esc\s*\(/g) || []; - if (defs.length !== 1) - throw new Error(`expected exactly one function esc() in ${file}, found ${defs.length}; ` + // #333 N4 / #393 N10: the regex below lifts the *first* one-line + // `function esc(s)`. If a second file-local esc were added above the real + // sink's escaper — in ANY declaration form — it would silently retarget + // this contract test. Pin the assumption: there must be exactly one esc + // definition in the file, else fail loudly. + const defs = countEscDefs(src); + if (defs !== 1) + throw new Error(`expected exactly one esc definition in ${file}, found ${defs}; ` + 'loadLocalEsc would silently pick the first — add a line anchor to disambiguate'); const m = src.match(/\n\s*(function esc\(s\) \{[^\n]*\})/); if (!m) throw new Error('could not locate function esc(s) in ' + file); @@ -1220,6 +1231,264 @@ perfCardTest('#394 F1 packet store: ps.indexes.byObserver card is escaped', /(
\$\{[^}]*ps\.indexes\.byObserver[^}]*\}<\/div>
Observers<\/div><\/div>)/, { ps: { indexes: { byObserver: PERF_PAYLOAD } } }, '#394 F1 byObserver'); +// ========================================================================= +// #393 N5 — the #394 F1 cards above only cover the Go-runtime health branch +// (gr.*). The Node-runtime branch (health.memory `m.*` / health.eventLoop +// `el.*`) escapes too but no unit test entered it, so dropping escapeHtml on +// e.g. `m.rss` stayed green. Drive one card per Node-branch response object. +// ========================================================================= +console.log('\n=== #393 N5 perf.js Node-branch health cards ==='); + +perfCardTest('#393 N5 health(node): m.rss RSS card is escaped', + /(
\$\{[^}]*m\.rss[^}]*\}MB<\/div>
RSS<\/div><\/div>)/, + { m: { rss: PERF_PAYLOAD } }, '#393 N5 m.rss'); + +perfCardTest('#393 N5 health(node): el.maxLagMs card is escaped', + /(
\$\{[^}]*el\.maxLagMs[^}]*\}ms<\/div>
EL Max Lag<\/div><\/div>)/, + { el: { maxLagMs: PERF_PAYLOAD } }, '#393 N5 el.maxLagMs'); + +perfCardTest('#393 N5 health(node): el.currentLagMs card is escaped', + /(
\$\{[^}]*el\.currentLagMs[^}]*\}ms<\/div>
EL Current<\/div><\/div>)/, + { el: { currentLagMs: PERF_PAYLOAD } }, '#393 N5 el.currentLagMs'); + +// ========================================================================= +// #393 F3 (round-2 review) — the explicit card tests above cover 8 of the +// escapeHtml-wrapped perf-num cards. Drive EVERY card instead: list the +// escaped cards per group, so dropping escapeHtml from any of them fails by +// label, and evaluate each card's real perf-num template with a hostile +// value behind every field it reads. +// ========================================================================= +console.log('\n=== #393 F3 perf.js escaped perf-num cards (all) ==='); + +const PERF_ESCAPED_CARDS = [ + // overview + 'Total Requests', 'Avg Response', 'Uptime', + // Go runtime + 'Goroutines', 'GC Collections', 'CPUs', 'WS Clients', + // Node runtime + 'Heap Used / ${escapeHtml(m.heapTotal)}MB', 'RSS', 'Event Loop p95', 'EL Max Lag', + 'EL Current', 'WS Clients', + // SQLite stats + 'Page Count', 'Page Size', + // cache (server + client) + 'Server Entries', 'Server Hits', 'Server Misses', 'Server Hit Rate', + 'Stale Hits (SWR)', 'Recomputes', 'Client Hits', 'Client Misses', 'Client Hit Rate', + // packet store + 'Packets in RAM', 'Tracked Memory', 'Memory Limit', 'Heap (debug)', 'Queries Served', + 'Live Inserts', 'Evicted', 'Unique Hashes', 'Observers', 'Indexed Nodes', + // SQLite + 'DB Size', 'WAL Size', 'Freelist', 'Transmissions', 'Observations', 'Nodes', + 'Observers', 'WAL Busy Pages', +]; + +function perfCards() { + const src = fs.readFileSync('public/perf.js', 'utf8'); + const re = /
(.*?)<\/div>
(.*?)<\/div><\/div>/g; + return [...src.matchAll(re)].map(m => ({ num: m[1], label: m[2] })); +} + +// Stands in for every API object: any property is another hostile value, and +// it converts to PERF_PAYLOAD as a string, number or via toLocaleString(). +function hostileValue() { + return new Proxy(function () {}, { + get(_, k) { + if (k === Symbol.toPrimitive) return () => PERF_PAYLOAD; + if (k === 'toString' || k === 'valueOf' || k === 'toLocaleString') return () => PERF_PAYLOAD; + return hostileValue(); + }, + }); +} + +test('#393 F3 perf.js: every listed card wraps its perf-num value in escapeHtml', () => { + const escaped = perfCards().filter(c => /\$\{escapeHtml\(/.test(c.num)).map(c => c.label); + assert.deepStrictEqual(escaped.slice().sort(), PERF_ESCAPED_CARDS.slice().sort(), + '#393 F3: the set of escapeHtml-wrapped perf-num cards changed'); +}); + +for (const card of perfCards().filter(c => /\$\{escapeHtml\(/.test(c.num))) { + test(`#393 F3 perf card "${card.label}": hostile value is escaped`, () => { + const roots = [...new Set([...card.num.matchAll(/\$\{([\s\S]*?)\}/g)] + .flatMap(x => x[1].match(/(? id !== 'escapeHtml' && id !== 'Math'); + const fn = new Function(...roots, 'escapeHtml', 'return `' + card.num + '`;'); + assertNoXss(fn(...roots.map(hostileValue), escapeHtml), '#393 F3 ' + card.label); + }); +} + +// ========================================================================= +// #393 N6 (from the #394 re-review) — the error path at perf.js ~374 puts +// escapeHtml(err.message) into innerHTML. err.message is attacker-influenced: +// a non-JSON /api/perf body embeds in the JSON.parse message. It was the one +// newly escaped sink with no test. Capture the template regardless of the +// wrapper so dropping escapeHtml flips the assertion, not the match. +// ========================================================================= +console.log('\n=== #393 N6 perf.js load-error sink ==='); + +test('#393 N6 perf.js error paragraph: err.message is escaped', () => { + const html = evalTemplate( + 'public/perf.js', + /el\.innerHTML = `(

Error: \$\{[^}]*err\.message[^}]*\}<\/p>)`;/, + { err: { message: PERF_PAYLOAD } } + ); + assertNoXss(html, '#393 N6 perf.js err.message'); +}); + +// ========================================================================= +// #393 follow-ups to #373 — analytics a11y (N6), delegated-nav pin (N7), +// compare.js onclick removal (N9), loadLocalEsc guard robustness (N10). +// ========================================================================= +console.log('\n=== #393 follow-ups to #373 ==='); + +// Minimal element whose closest(sel) honours an optional tag-name prefix, so +// a DIV row matches `[data-action="navigate"]` but NOT `tr[...]` and a TR row +// matches both — exactly the distinction the delegated selectors turn on. +function fakeNavRow(tag, value) { + const el = { tagName: tag.toUpperCase(), dataset: { action: 'navigate', value } }; + el.closest = (sel) => { + const mm = sel.match(/^([a-z]*)\[data-action="navigate"\]$/i); + if (!mm) return null; + if (mm[1] && mm[1].toUpperCase() !== el.tagName) return null; + return el.dataset.action === 'navigate' ? el : null; + }; + return el; +} + +// N6: the analytics repeater row is a

whose parent .repeater-list is a +// plain
, so role="row" has no required rowgroup/table/grid ancestor +// (11 critical axe violations). #393 F4 (round-2 review): rejecting only the +// literal role="row" let role="cell" (equally invalid there) pass. Allow-list +// the focusable row's role instead, and reject every WAI-ARIA 1.2 role with a +// required context role anywhere in the row — none has its parent here. +const ARIA_ROLES_NEEDING_CONTEXT = ['cell', 'columnheader', 'gridcell', 'listitem', + 'menuitem', 'menuitemcheckbox', 'menuitemradio', 'option', 'row', 'rowgroup', + 'rowheader', 'tab', 'treeitem']; +test('#393 N6 analytics repeater row: a standalone interactive role, no role needing a context', () => { + const src = fs.readFileSync('public/analytics.js', 'utf8'); + const m = src.match(/html \+= `
String(s), 50, encodeURIComponent); + const rowTag = html.match(/^
needs role="link" or "button", got ' + rowRole + ': ' + rowTag[0]); + const bad = [...html.matchAll(/\srole="([^"]*)"/g)].map(x => x[1]) + .filter(r => ARIA_ROLES_NEEDING_CONTEXT.includes(r)); + assert.deepStrictEqual(bad, [], + '#393 N6: repeater row carries a role with no required ancestor here: ' + html); + assert.ok(/\btabindex="0"/.test(rowTag[0]), + '#393 N6: repeater row lost tabindex="0" keyboard access: ' + html); +}); + +// N7: the delegated handler selector was broadened to [data-action="navigate"] +// so the DIV repeater row is clickable; nothing pinned it. Drive the real +// handler with a DIV row — narrowing the selector back to tr[...] goes red. +test('#393 N7 analytics delegated handler: a DIV row navigates on click', () => { + const src = fs.readFileSync('public/analytics.js', 'utf8'); + const m = src.match(/const handler = \(e\) => \{[\s\S]*?location\.hash = row\.dataset\.value;\s*\};/); + assert.ok(m, '#393 N7 analytics delegated nav handler not found'); + const makeHandler = new Function('location', m[0] + '\n return handler;'); + const loc = { hash: '' }; + const handler = makeHandler(loc); + handler({ type: 'click', target: fakeNavRow('div', '#/nodes/deadbeef12') }); + assert.strictEqual(loc.hash, '#/nodes/deadbeef12', + '#393 N7: delegated [data-action="navigate"] did not navigate a DIV row on click'); +}); + +// N9a: the compare packet row dropped its inline onclick (the latent +// '-breakout in that double-quoted/JS context). Pin that the row carries no +// inline on* handler — re-adding onclick goes red. #393 F5 (round-2 review): +// scan the whole row expression, through '', so an on* attribute +// moved onto a child / goes red too. +test('#393 N9 compare.js packet row: no inline on* handler on the row or its children', () => { + const src = fs.readFileSync('public/compare.js', 'utf8'); + const m = src.match(/return '';/); + assert.ok(m, '#393 N9 compare packet-row expression not found'); + assert.ok(/ children: ' + m[0]); + const on = m[0].match(/\son[a-z]+\s*=/i); + assert.ok(!on, '#393 N9: compare packet row still has an inline handler (' + (on && on[0]) + '): ' + m[0]); +}); + +// Run the REAL compare.js init() in a vm with a fake #app whose innerHTML +// assignment recreates its children, as the browser does. Each call returns +// the shared app element and that visit's #compareContent so a test can see +// which element a listener landed on. +function loadComparePage() { + const src = fs.readFileSync('public/compare.js', 'utf8'); + function fakeEl() { + const el = { listeners: {} }; + el.addEventListener = (type, fn) => { (el.listeners[type] = el.listeners[type] || []).push(fn); }; + return el; + } + let children = {}; + const app = fakeEl(); + Object.defineProperty(app, 'innerHTML', { + set() { children = { compareControls: fakeEl(), compareContent: fakeEl() }; }, + }); + const loc = { hash: '#/compare' }; + const ctx = { + window: {}, location: loc, CLIENT_TTL: {}, escapeHtml, URLSearchParams, + document: { getElementById: (id) => children[id] || null }, + api: () => new Promise(() => {}), // observers never load: init's wiring only + registerPage: (name, page) => { ctx.page = page; }, + }; + vm.runInNewContext(src, ctx); + return { + app, loc, + visit() { ctx.page.init(app, ''); return children.compareContent; }, + leave() { ctx.page.destroy(); }, + }; +} + +// N9b: and the replacement delegated click handler actually navigates. Drive +// the click listener the real init() installs on a nav row — removing +// or narrowing it goes red. +test('#393 N9 compare.js delegated click handler navigates a nav row', () => { + const cmp = loadComparePage(); + const content = cmp.visit(); + const clicks = content.listeners.click || []; + assert.strictEqual(clicks.length, 1, '#393 N9: expected one delegated click handler on #compareContent'); + clicks[0]({ type: 'click', target: fakeNavRow('tr', '#/packets/deadbeef12') }); + assert.strictEqual(cmp.loc.hash, '#/packets/deadbeef12', + '#393 N9: compare delegated click did not navigate the nav row'); +}); + +// #393 F1 (round-2 review): the N9 click handler was bound to the shared #app +// element, which every page renders into and nothing ever unbinds. After one +// compare visit it fired on every later page's tr[data-action="navigate"], +// overriding the observers slide-over's preventDefault() (#1056 AC#4), and +// each visit stacked one more. It must live on compare's own container, +// which the next page's app.innerHTML discards. +test('#393 F1 compare.js init: no click listener on the shared #app, none stacked per visit', () => { + const cmp = loadComparePage(); + const first = cmp.visit(); + cmp.leave(); + const second = cmp.visit(); + cmp.leave(); + assert.strictEqual((cmp.app.listeners.click || []).length, 0, + '#393 F1: compare.js bound a click listener to the shared #app; it outlives the page'); + assert.notStrictEqual(first, second, 'harness: each visit must get a fresh #compareContent'); + assert.strictEqual((second.listeners.click || []).length, 1, + '#393 F1: the current visit must carry exactly one row click handler'); +}); + +// N10: loadLocalEsc's guard must also catch a differently-declared second +// esc (const/let/var), not only a second `function esc(`. Exercise the lifted +// counter directly — weakening it back to function-only makes case 2 go red. +test('#393 N10 countEscDefs catches a second, differently-declared esc', () => { + assert.strictEqual(countEscDefs('function esc(s){return s;}'), 1, + '#393 N10: a single function esc must count as 1'); + assert.strictEqual(countEscDefs('function esc(s){return s;}\nconst esc = (s) => s;'), 2, + '#393 N10: a second `const esc =` must be counted (the N4 blind spot)'); + assert.strictEqual(countEscDefs('let esc = (s) => s;\nfunction esc(s){}'), 2, + '#393 N10: let-declared esc must also be counted'); + assert.strictEqual(countEscDefs('function other(s){}'), 0, + '#393 N10: no esc definition must count as 0'); +}); + // ========================================================================= // SUMMARY // =========================================================================