fix(analytics): escape-safe ?tab= lookup and withQuery contract (#193)

public/analytics.js

@@ -255,11 +255,15 @@
     const _ap = new URLSearchParams(hashParams);
     // Every mount starts from the URL, falling back to Overview: the tab
     // selected before leaving the page must not survive into this mount (#183).
+    // The button is found by comparing data-tab, never by building a selector
+    // from the URL: a quote in ?tab= threw here, and a crafted value matched a
+    // real button while _currentTab took the arbitrary string (#193).
     const urlTab = _ap.get('tab');
-    const urlTabBtn = urlTab && analyticsTabs.querySelector(`[data-tab="${urlTab}"]`);
+    const tabBtns = Array.from(analyticsTabs.querySelectorAll('.tab-btn'));
+    const urlTabBtn = urlTab ? tabBtns.find(b => b.dataset.tab === urlTab) : null;
     _currentTab = urlTabBtn ? urlTab : 'overview';
-    const activeBtn = urlTabBtn || analyticsTabs.querySelector('[data-tab="overview"]');
-    analyticsTabs.querySelectorAll('.tab-btn').forEach(b => b.classList.remove('active'));
+    const activeBtn = urlTabBtn || tabBtns.find(b => b.dataset.tab === 'overview');
+    tabBtns.forEach(b => b.classList.remove('active'));
     if (activeBtn) activeBtn.classList.add('active');
     // #749 — restore time window from URL.
     const urlWindow = _ap.get('window');
@@ -327,10 +331,13 @@
   var _themeRefreshHandler = null;
   let _currentTab = 'overview';
 
-  // Filter fragments ("&region=…", "&area=…", "&window=…") all start with
-  // '&'; turn their concatenation into the path's query string (#179).
+  // Append a query fragment to a path (#179, #193). The filter fragments
+  // ("&region=…", "&area=…", "&window=…") start with '&'; '?…' and a bare
+  // 'a=1' are taken too, and an empty fragment (or a lone '&' / '?') leaves
+  // the path as it is. The separator is '&' when the path already has a '?'.
   function withQuery(path, frag) {
-    return frag ? path + '?' + frag.slice(1) : path;
+    const q = frag ? String(frag).replace(/^[?&]/, '') : '';
+    return q ? path + (path.indexOf('?') < 0 ? '?' : '&') + q : path;
   }
 
   async function loadAnalytics(startedAt) {
@@ -3151,6 +3158,7 @@ function destroy() { _stopRolesRefresh(); _stopScopesRefresh(); _stopForeignTraf
   // Expose for testing
   if (typeof window !== 'undefined') {
     window._analyticsAssignTableIds = assignAnalyticsTableIds;
+    window._analyticsWithQuery = withQuery;
     window._analyticsDecorateChannels = decorateAnalyticsChannels;
     window._analyticsSortChannels = sortChannels;
     window._analyticsLoadChannelSort = loadChannelSort;

test-analytics-tab-state-and-query.js

@@ -4,6 +4,10 @@
  *       whenever a region or area filter was active, and got the SPA HTML.
  * #183: leaving #/analytics and coming back without ?tab= marked Overview
  *       active but rendered the tab selected before leaving.
+ * #193: a ?tab= value must never reach a selector (a quote threw in init();
+ *       a crafted value matched a real button and left the page on
+ *       "Loading analytics…"), the remaining #191 test gaps, and the
+ *       withQuery(path, frag) contract.
  *
  * Runs the REAL app.js api() and the REAL analytics page in one vm with a
  * fake fetch and a small fake DOM whose tab buttons come from the markup
@@ -50,6 +54,28 @@ function fakeEl(id) {
   };
 }
 
+// The one selector shape init() could build from ?tab=: a list of
+// [data-tab="…"] attribute selectors. Parsed like a browser would: a stray
+// quote is a SyntaxError, a comma starts another selector of the list, and
+// querySelector returns the first match in document order.
+function parseTabSelector(sel) {
+  const open = '[data-tab="';
+  const values = [];
+  let i = 0;
+  for (;;) {
+    if (sel.slice(i, i + open.length) !== open) throw new SyntaxError("'" + sel + "' is not a valid selector.");
+    i += open.length;
+    let v = '';
+    while (i < sel.length && sel[i] !== '"') { if (sel[i] === '\\') i++; v += sel[i++]; }
+    if (sel[i] !== '"' || sel[i + 1] !== ']') throw new SyntaxError("'" + sel + "' is not a valid selector.");
+    i += 2;
+    values.push(v);
+    if (i === sel.length) return values;
+    if (sel[i] !== ',') throw new SyntaxError("'" + sel + "' is not a valid selector.");
+    i++;
+  }
+}
+
 function pageEnv() {
   const fetchLog = [];
   let region = '', area = '';
@@ -62,6 +88,9 @@ function pageEnv() {
     const m = /\/api\/analytics\/(rf|topology|channels|hash-sizes|hash-collisions)(\?|$)/.exec(url);
     const key = m && { rf: 'rfData', topology: 'topoData', channels: 'chanData', 'hash-sizes': 'hashData', 'hash-collisions': 'collisionData' }[m[1]];
     if (key) return respond(REAL[key]);
+    // The neighbor-graph tab only needs its request observed; failing it keeps
+    // the canvas renderer, which the fake DOM cannot host, out of the run.
+    if (/\/api\/analytics\/neighbor-graph/.test(url)) return { ok: false, status: 404, headers: { get: () => null }, json: async () => ({ error: 'not found' }) };
     if (/\/api\/nodes\?/.test(url)) return respond({ nodes: [{ public_key: 'aabbccddeeff', name: 'R1', role: 'repeater' }] });
     return respond({});
   };
@@ -94,15 +123,18 @@ function pageEnv() {
       if (id === 'analyticsTabs') {
         els[id].querySelectorAll = (sel) => (sel === '.tab-btn' ? tabButtons() : []);
         els[id].querySelector = (sel) => {
-          const m = /^\[data-tab="([^"]+)"\]$/.exec(sel);
-          return m ? tabButtons().find((b) => b.dataset.tab === m[1]) || null : null;
+          const wanted = parseTabSelector(sel);
+          return tabButtons().find((b) => wanted.includes(b.dataset.tab)) || null;
         };
       }
+      // The markup ships the area filter hidden; the page shows it per tab.
+      if (id === 'analyticsAreaFilter') els[id].style.display = 'none';
     }
     return els[id];
   };
 
   const pages = {};
+  let initError = null;
   const ctx = {
     window: { addEventListener() {}, removeEventListener() {}, dispatchEvent() {} },
     document: {
@@ -137,7 +169,16 @@ function pageEnv() {
   return {
     fetchLog,
     setFilters: (r, a) => { region = r ? '&region=' + r : ''; area = a ? '&area=' + a : ''; },
-    async mount(hash) { ctx.location.hash = hash; page.init(app); await flush(); },
+    async mount(hash) {
+      ctx.location.hash = hash;
+      initError = null;
+      page.init(app).catch((e) => { initError = e; });
+      await flush();
+    },
+    initError: () => initError,
+    invalidateApiCache: (prefix) => ctx.invalidateApiCache(prefix),
+    withQuery: ctx._analyticsWithQuery,
+    areaFilterDisplay: () => el('analyticsAreaFilter').style.display,
     destroy: () => page.destroy(),
     clickTab: async (tab) => {
       const btn = tabButtons().find((b) => b.dataset.tab === tab);
@@ -247,6 +288,143 @@ function pageEnv() {
     assert.ok(MARKERS.overview.test(env.content()), 'content is not Overview: ' + env.content().slice(0, 200));
   });
 
+  console.log('\n=== #193: ?tab= never reaches a selector ===');
+
+  // Each value either throws in a selector (a quote) or builds a selector
+  // list that matches a real button ('overview', 'rf'). All must end on
+  // Overview, with Overview rendered: _currentTab is a real tab again.
+  const hostileTabs = ['x"', 'x"],[data-tab="overview', 'x"],[data-tab="rf', 'overview"]', '\\', '', 'overview', '__proto__'];
+  for (const value of hostileTabs) {
+    await test('?tab=' + JSON.stringify(value) + ' shows Overview with Overview active, no exception', async () => {
+      const env = pageEnv();
+      await env.mount('#/analytics?tab=' + encodeURIComponent(value));
+      assert.strictEqual(env.initError(), null, 'init() threw: ' + (env.initError() && env.initError().message));
+      assert.deepStrictEqual(env.activeTabs(), ['overview'], 'active button');
+      assert.ok(MARKERS.overview.test(env.content()), 'content is not Overview: ' + env.content().slice(0, 200));
+    });
+  }
+
+  await test('a hostile ?tab= after another tab was selected still ends on Overview', async () => {
+    const env = pageEnv();
+    await env.mount('#/analytics');
+    await env.clickTab('topology');
+    env.destroy();
+    await env.mount('#/analytics?tab=' + encodeURIComponent('x"],[data-tab="topology'));
+    assert.strictEqual(env.initError(), null, 'init() threw');
+    assert.deepStrictEqual(env.activeTabs(), ['overview'], 'active button');
+    assert.ok(MARKERS.overview.test(env.content()) && !MARKERS.topology.test(env.content()), 'content is not Overview');
+    await env.clickTab('topology');
+    assert.ok(MARKERS.topology.test(env.content()), 'tab switching broken after the hostile mount');
+  });
+
+  console.log('\n=== #193: remaining #191 test gaps ===');
+
+  await test('the neighbor-graph tab asks for neighbor-graph?region=…&min_count=1&min_score=0', async () => {
+    const env = pageEnv();
+    env.setFilters('SJC');
+    await env.mount('#/analytics?tab=neighbor-graph');
+    assert.ok(env.fetchLog.includes('/api/analytics/neighbor-graph?region=SJC&min_count=1&min_score=0'), env.fetchLog.join(', '));
+    const env2 = pageEnv();
+    await env2.mount('#/analytics?tab=neighbor-graph');
+    assert.ok(env2.fetchLog.includes('/api/analytics/neighbor-graph?min_count=1&min_score=0'), env2.fetchLog.join(', '));
+  });
+
+  // loadAnalytics asks for the same hash-sizes URL on every mount and api()
+  // dedupes by URL, so the Prefix Tool's own request is only visible when it
+  // goes out on its own: mount Overview, drop the cache, open the tab.
+  await test('the Prefix Tool\'s own hash-sizes request carries region and area', async () => {
+    const env = pageEnv();
+    env.setFilters('SJC', 'north');
+    await env.mount('#/analytics');
+    env.invalidateApiCache('/analytics');
+    env.fetchLog.length = 0;
+    await env.clickTab('prefix-tool');
+    const urls = env.fetchLog.filter((u) => /\/api\/analytics\/hash-sizes/.test(u));
+    assert.deepStrictEqual(urls, ['/api/analytics/hash-sizes?region=SJC&area=north']);
+  });
+
+  await test('the Prefix Tool\'s own hash-sizes request is plain without filters', async () => {
+    const env = pageEnv();
+    await env.mount('#/analytics');
+    env.invalidateApiCache('/analytics');
+    env.fetchLog.length = 0;
+    await env.clickTab('prefix-tool');
+    assert.deepStrictEqual(env.fetchLog.filter((u) => /\/api\/analytics\/hash-sizes/.test(u)), ['/api/analytics/hash-sizes']);
+  });
+
+  await test('the area filter is shown on the mounted tab only where it applies', async () => {
+    const env = pageEnv();
+    await env.mount('#/analytics');
+    assert.strictEqual(env.areaFilterDisplay(), '', 'Overview: area filter not shown');
+    await env.mount('#/analytics?tab=prefix-tool');
+    assert.strictEqual(env.areaFilterDisplay(), 'none', 'Prefix Tool: area filter not hidden');
+    await env.clickTab('rf');
+    assert.strictEqual(env.areaFilterDisplay(), '', 'RF: area filter not shown after the switch');
+    await env.clickTab('prefix-tool');
+    assert.strictEqual(env.areaFilterDisplay(), 'none', 'Prefix Tool: area filter not hidden after the switch');
+  });
+
+  await test('returning to a plain #/analytics after a tab without the area filter shows it again', async () => {
+    const env = pageEnv();
+    await env.mount('#/analytics?tab=prefix-tool');
+    env.destroy();
+    await env.mount('#/analytics');
+    assert.strictEqual(env.areaFilterDisplay(), '');
+  });
+
+  console.log('\n=== #193: withQuery(path, frag) contract ===');
+
+  await test('withQuery is exposed for the test', async () => {
+    assert.strictEqual(typeof pageEnv().withQuery, 'function', 'window._analyticsWithQuery missing');
+  });
+
+  const WQ = [
+    // path, fragment, expected
+    ['/a', '', '/a'],
+    ['/a', undefined, '/a'],
+    ['/a', null, '/a'],
+    ['/a', '&region=X', '/a?region=X'],
+    ['/a', '&region=X&area=Y', '/a?region=X&area=Y'],
+    ['/a', '?region=X', '/a?region=X'],
+    ['/a', 'region=X', '/a?region=X'],
+    ['/a', '&', '/a'],
+    ['/a', '?', '/a'],
+    ['/a?x=1', '', '/a?x=1'],
+    ['/a?x=1', '&region=X', '/a?x=1&region=X'],
+    ['/a?x=1', '?region=X', '/a?x=1&region=X'],
+    ['/a?x=1', 'region=X', '/a?x=1&region=X'],
+    ['/a?x=1', '&', '/a?x=1'],
+  ];
+  for (const [path, frag, want] of WQ) {
+    await test('withQuery(' + JSON.stringify(path) + ', ' + JSON.stringify(frag) + ') = ' + JSON.stringify(want), async () => {
+      assert.strictEqual(pageEnv().withQuery(path, frag), want);
+    });
+  }
+
+  // The pre-#179 call sites built their query strings by hand. The helper
+  // must produce exactly those strings for every filter combination.
+  await test('withQuery equals the hand-built query strings it replaced (every filter combination)', async () => {
+    const { withQuery } = pageEnv();
+    const rqsV = ['', '&region=SJC', '&region=' + encodeURIComponent('A,B')];
+    const aqsV = ['', '&area=north', '&area=' + encodeURIComponent('a&b')];
+    const twV = ['', '&window=7d'];
+    const oldSep = (q) => { const t = q.slice(1); return t ? '?' + t : ''; };
+    let compared = 0;
+    for (const r of rqsV) for (const a of aqsV) for (const t of twV) {
+      const cases = [
+        [r + a, oldSep(r + a)],                               // hash-sizes, hash-collisions
+        [r + a + t, oldSep(r + a + t)],                       // rf, topology, relay-airtime-share
+        [r + t, oldSep(r + t)],                               // channels
+        [r, r ? '?' + r.slice(1) : ''],                       // distance
+      ];
+      for (const [frag, want] of cases) { assert.strictEqual(withQuery('/x', frag), '/x' + want); compared++; }
+      const sep = r ? '?' + r.slice(1) : '';                  // neighbor-graph
+      assert.strictEqual(withQuery('/x', r + '&min_count=1&min_score=0'), '/x' + sep + (sep ? '&' : '?') + 'min_count=1&min_score=0');
+      compared++;
+    }
+    assert.strictEqual(compared, 90, 'URL pairs compared');
+  });
+
   console.log('\n' + passed + ' passed, ' + failed + ' failed');
   if (failed > 0) process.exit(1);
 })();

test-issue-1375-scope-stats-fetch.js

@@ -146,18 +146,18 @@ function makeSandbox() {
         id, innerHTML: '', textContent: '', value: '',
         style: {}, dataset: {}, classList: { add() {}, remove() {}, toggle() {}, contains() { return false; } },
         addEventListener() {}, removeEventListener() {},
-        // init()'s deep-link tab selector (analytics.js ~205) does
-        // `analyticsTabs.querySelector('[data-tab="scopes"]')` to find the
-        // tab button matching a `#/analytics?tab=scopes` hash, then reads
-        // it back truthy to set `_currentTab`. A generic `[data-tab="x"]`
-        // match (rather than special-casing the analyticsTabs id) covers
-        // that real production lookup without hand-modeling the tab bar's
-        // actual child markup.
-        querySelector(sel) {
-          const m = /\[data-tab="([^"]+)"\]/.exec(sel || '');
-          return m ? elFor('tab-btn:' + m[1]) : null;
+        // init()'s deep-link lookup (#193) lists `analyticsTabs.querySelectorAll
+        // ('.tab-btn')` and matches `btn.dataset.tab` against ?tab= (it no
+        // longer builds a selector from the URL). A generic `.tab-btn` list of
+        // the two tabs this test drives (rather than special-casing the
+        // analyticsTabs id) covers that real production lookup without
+        // hand-modeling the tab bar's actual child markup. `#/analytics
+        // ?tab=scopes` therefore finds the Scopes button and sets _currentTab.
+        querySelector() { return null; },
+        querySelectorAll(sel) {
+          if (sel !== '.tab-btn') return [];
+          return ['overview', 'scopes'].map((t) => { const b = elFor('tab-btn:' + t); b.dataset.tab = t; return b; });
         },
-        querySelectorAll() { return []; },
         appendChild() {}, closest() { return null; },
         getAttribute() { return null; }, setAttribute() {}, removeAttribute() {},
       });