fix: Follow-ups to #373: analytics row a11y, delegated navigate test, compare.js onclick, loadL (#393)

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")
+		}
+	})
 }

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,

public/analytics.js

@@ -1102,7 +1102,7 @@
     let html = '<div class="repeater-list">';
     repeaters.slice(0, 15).forEach(r => {
       const pct = (r.count / max * 100).toFixed(0);
-      html += `<div class="repeater-row ${r.pubkey ? 'clickable-row' : ''}" ${r.pubkey ? `data-action="navigate" data-value="#/nodes/${encodeURIComponent(r.pubkey)}" tabindex="0" role="row"` : ''}>
+      html += `<div class="repeater-row ${r.pubkey ? 'clickable-row' : ''}" ${r.pubkey ? `data-action="navigate" data-value="#/nodes/${encodeURIComponent(r.pubkey)}" tabindex="0" role="link"` : ''}>
         <div class="repeater-name">${r.name ? '<strong>' + esc(r.name) + '</strong>' : '<span class="mono">' + r.hop + '</span>'}</div>
         <div class="repeater-bar"><div class="hash-bar-track"><div class="hash-bar-fill" style="width:${pct}%;background:var(--accent)"></div></div></div>
         <div class="repeater-count">${r.count.toLocaleString()}</div>

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 '<tr style="cursor:pointer" tabindex="0" role="row" data-action="navigate" data-value="#/packets/' + escapeHtml(h) + '" onclick="location.hash=\'#/packets/' + escapeHtml(h) + '\'">' +
+          return '<tr style="cursor:pointer" tabindex="0" role="row" data-action="navigate" data-value="#/packets/' + escapeHtml(h) + '">' +
             '<td class="mono" style="font-size:0.85em">' + escapeHtml(h.substring(0, 12)) + '</td>' +
             '<td>' + timeAgo(p.timestamp || p.first_seen) + '</td>' +
             '<td><span class="payload-badge badge-' + payloadTypeColor(p.payload_type) + '">' + escapeHtml(typeName) + '</span></td>' +

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.

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

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);

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);