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

[dborup]

Relates to #193

Follow-up to the review of #191: an escape-safe ?tab= lookup, the three test gaps that survived as mutants, and a stated contract for withQuery. Frontend only: public/analytics.js and two test files.

Changes

1. ?tab= never reaches a selector

init() looked the tab button up with an attribute selector built from the URL.

• A value containing " threw in init(), and the page stayed on "Loading analytics…".
• A crafted value such as x"],[data-tab="overview matched a real button. That button was marked active, _currentTab became the arbitrary string, and the page hung on "Loading…". This is the same symptom class as fix(analytics): after leaving and returning to #/analytics, Overview is marked active but the old tab's content is shown #183, and a shared link could trigger it. It is not XSS: the value was only compared and used in a switch.

The button is now found by comparing btn.dataset.tab === tab over .tab-btn. Any value that is not a tab (a quote, a crafted list, empty, unknown) gives Overview, with the Overview button active and _currentTab = 'overview'.

2. The three surviving #191 mutants are now killed

• The neighbor-graph URL is pinned, with and without a region.
• The Prefix Tool's own hash-sizes request is now observable. loadAnalytics asks for the same URL on every mount and api() dedupes by URL, so the test mounts Overview, drops the api cache, then opens the Prefix Tool tab and asserts that its request alone is hash-sizes?region=SJC&area=north (and plain without filters).
• setAreaFilterVisibility in init() is covered. The fake analyticsAreaFilter starts hidden, as the markup ships it, and the tests assert its display state for a plain mount, a ?tab=prefix-tool mount, tab switches, and a plain return after the Prefix Tool.

3. withQuery(path, frag) is tolerant

Chosen over a documented-and-asserted contract because the failure mode of the strict version is silent: a bad fragment produces a wrong URL (/a?egion=X, or a second ?), not an error. The tolerant version is one line longer.

• '', undefined, null, a lone & or ? leave the path unchanged.
• &a=1, ?a=1 and a bare a=1 are all the query a=1.
• The separator is & when the path already has a ?.

The function is exposed as window._analyticsWithQuery, next to the existing _analytics* test exports.

Red before, green after

Commit 1 holds the tests and the one-line export. On origin/master code it gives 26 passed, 13 failed:

• the five hostile ?tab= values and the hostile-after-another-tab case (a throw, or a wrong tab);
• the withQuery edge cases (bare fragment, lone separator, path with a query).

The neighbor-graph, Prefix Tool and area-filter tests pass on master by design: they pin existing behaviour, and their proof is the mutants below. After the fix commit, the file gives 39 passed, 0 failed. The fake tab bar now parses [data-tab="…"] selectors like a browser does (a stray quote is a SyntaxError, a list returns the first match), which is what makes the hostile cases fail honestly on the old code.

test-issue-1375-scope-stats-fetch.js had a fake tab bar that only answered querySelector. It now lists its buttons for querySelectorAll('.tab-btn'); its assertions are unchanged (third commit).

Mutants

Each runs the new test file against a copy of analytics.js with one change:

Mutant	Result
neighbor-graph drops min_count/min_score (was surviving)	fails: neighbor-graph test
Prefix Tool drops the area filter (was surviving)	fails: Prefix Tool own-request test
setAreaFilterVisibility removed from init() (was surviving)	fails: both area-filter tests
selector lookup restored	fails: 6 hostile-?tab= tests
unknown ?tab= kept as _currentTab	fails: 8 tests
withQuery without the & join	fails: 3 withQuery tests
withQuery strips only a leading &	fails: 3 withQuery tests
withQuery back to slice(1)	fails: 2 withQuery tests

URLs stay byte-identical

The real withQuery from origin/master and the new one were compared over every call-site fragment (hash-sizes and hash-collisions, rf, topology and relay-airtime-share, channels, distance, neighbor-graph) × region ∈ {none, SJC, A,B encoded} × area ∈ {none, north, a&b encoded} × window ∈ {none, 7d}: 90 fragments, 0 differences. The test file also asserts, against the hand-built formulas the helper replaced, that the 90 resulting URL pairs are unchanged.

Verification

• sh test-all.sh: 201 passed, 0 failed (201 files).
• node test-frontend-helpers.js: 707 passed, 0 failed.
• scripts/check-xss-sinks.sh --diff origin/master: no findings.
• deploy.yml is untouched and still has 9 × github.repository == 'Kpa-clawbot/CoreScope'.
• No hardcoded colours, no per-item API calls, ES5/6 only.

Browser

A local Go server built from this branch, with e2e-fixture.db freshened and migrated as in CI, driven by Chromium through Playwright. I confirmed the server's working directory was this tree and that it served the new analytics.js.

• #/analytics?tab=x", ?tab=x"],[data-tab="overview and ?tab=x"],[data-tab="rf: Overview button active, Overview content rendered, no page error and no console error. On master the first throws and the other two hang on "Loading analytics…".
• Prefix Tool with region SJC: only hash-sizes?region=SJC, 200 application/json.
• fix(analytics): after leaving and returning to #/analytics, Overview is marked active but the old tab's content is shown #183 scenario (Topology, #/nodes, #/analytics): Overview in button and content.
• Twelve other deep links activate the matching button; back/forward and plain re-entry behave as before; with 503 forced on /analytics/rf, the warm-up retry chain stops on leaving the page and does not double on re-mount.

Not changed

• A rejected ?tab= value stays in the URL hash until the next tab click rewrites it. The page itself is correct.
• destroy() still does not reset _currentTab: init() assigns it on every mount.

🤖 Generated with Claude Code

[dborup]

Rapport — CS-MacBook PR#194 #193 analytics — head 8d5e30f

Status: All three parts are implemented and verified locally; the draft PR is open and CI had not finished when this was posted.

Draft PR for #193, three commits on codex/issue-193-analytics-tab-escape (from master 8e4b13d0): tests first, then the fix, then a repair of one existing test's fake DOM. Commit identity is dborup <kontakt@meshview.dk>; nothing was amended, rebased or force-pushed.

Evidence tags: [T] test, probe, browser run or CI result I ran or saw myself, [A] analysis, [K] known, not run again.

Changes

Part 1: escape-safe ?tab= lookup. init() no longer builds a selector from the URL. It lists .tab-btn and compares btn.dataset.tab === tab. An unknown value (a quote, a crafted selector list, empty, unknown) gives Overview, with the Overview button active and _currentTab = 'overview'. [T] Six hostile or odd values plus a hostile-after-another-tab case are in the test file; Overview content rendering proves _currentTab is valid again.

Part 2: the three surviving #191 mutants. [T]

• The neighbor-graph URL is pinned, with and without a region.
• The Prefix Tool's own request is now distinguishable: the test mounts Overview, drops the api cache, opens the Prefix Tool tab, and asserts the one hash-sizes?region=SJC&area=north request it sends (and the plain URL without filters).
• setAreaFilterVisibility in init() is covered through a fake area filter that starts hidden, as the markup ships it.

Part 3: withQuery contract. I made it tolerant instead of documenting a strict contract. A strict contract fails silently (a wrong URL such as /a?egion=X, not an error), and the tolerant version is a single extra line. '', undefined, null, a lone & or ? leave the path alone; &a=1, ?a=1 and a bare a=1 are the same query; the separator is & when the path already has a ?. It is exposed as window._analyticsWithQuery, next to the existing test exports. [T]

Tests

• New file content, before the fix (commit 1 on master code): 26 passed, 13 failed. The failures are the hostile ?tab= cases and the withQuery edge cases. The neighbor-graph, Prefix Tool and area-filter tests pass on master by design, and the mutants below are their proof. [T]
• After the fix: 39 passed, 0 failed. [T]
• sh test-all.sh: 201 passed, 0 failed (201 files). [T] My first run gave 200 of 201: test-issue-1375-scope-stats-fetch.js has a fake tab bar that answered only querySelector, so it no longer found its Scopes button. I fixed the fake (third commit, assertions unchanged) and the file passes again.
• node test-frontend-helpers.js: 707 passed, 0 failed. [T]
• scripts/check-xss-sinks.sh --diff origin/master: no findings. deploy.yml untouched, 9 × github.repository == 'Kpa-clawbot/CoreScope'. No hardcoded colours, no per-item API calls. [T]
• URLs byte-identical: the real withQuery from origin/master and the new one agree on all 90 call-site fragments (hash-sizes and hash-collisions, rf, topology and relay-airtime-share, channels, distance, neighbor-graph × region × area × window), 0 differences. The test file also asserts the 90 resulting URL pairs against the hand-built formulas the helper replaced. [T]

Mutants (new test file against a copy of analytics.js with one change) [T]

Mutant	Result
neighbor-graph drops min_count/min_score (was surviving)	caught
Prefix Tool drops the area filter (was surviving)	caught
setAreaFilterVisibility removed from init() (was surviving)	caught
selector lookup restored	caught (6 tests)
unknown ?tab= kept as _currentTab	caught (8 tests)
withQuery without the & join	caught (3 tests)
withQuery strips only a leading &	caught (3 tests)
withQuery back to slice(1)	caught (2 tests)

Browser [T]

Local Go server built from this branch's tree, e2e-fixture.db freshened and migrated as in CI; I confirmed by pid and working directory that the server on the port was serving my tree, and stopped it by pid afterwards. Chromium through Playwright:

• ?tab=x", ?tab=x"],[data-tab="overview and ?tab=x"],[data-tab="rf: Overview button active, Overview content, no page error, no console error. Before the fix (master, previous review) the first threw and the other two hung on "Loading analytics…".
• Prefix Tool with region SJC: only hash-sizes?region=SJC, 200 application/json.
• fix(analytics): after leaving and returning to #/analytics, Overview is marked active but the old tab's content is shown #183 scenario (Topology, #/nodes, #/analytics): Overview in button and content.
• Twelve further deep links activate their button; back/forward and plain re-entry unchanged; with 503 forced on /analytics/rf, the warm-up chain stops on leaving and does not double on re-mount. The only console errors were the forced 503 lines.

CI

Pending when this was posted (Go Build & Test had started; Playwright E2E and the Docker build had not). I did not poll it. Per-job results are not in this report; they need a follow-up check on the PR.

Known leftovers

• A rejected ?tab= value stays in the URL hash until the next tab click rewrites it; the page itself is correct. [A]
• destroy() still does not reset _currentTab; init() assigns it on every mount. [A]
• The unit test's fake DOM has no layout, so the area filter's visibility is asserted as the display property, not as pixels. [A]

[dborup-agent]

Review — CS-pve-agent2 PR#194 analytics — head 8d5e30f

Dom: APPROVE med nits

Independent, read-only review of head 8d5e30f2 (3 commits, 3 files: public/analytics.js, test-analytics-tab-state-and-query.js, test-issue-1375-scope-stats-fetch.js). origin/master moved from 8e4b13d0 to 457dbf34 (#196) during the review. I therefore ran everything on the merged tree against 8e4b13d0 (identical to the head tree) and re-ran the test, rule and static checks on git merge-tree --write-tree origin/master <head> against 457dbf34 (clean merge). #196 touches no file under public/; diff -rq of public/ in the two merged trees is empty. The head was 8d5e30f2 on git ls-remote before and after the review.

Evidence tags: [T] test, probe, browser run or CI result I ran or saw myself, [A] analysis from reading code, [K] taken from the author's report and not run again.

Summary

The fix does what it says. No ?tab= value I tried can throw or activate the wrong tab, an unknown value gives Overview with the Overview button active, and all 20 real deep links still work. withQuery is a strict superset of the old behaviour on every input the callers can produce, and the real page requests the same analytics URLs as master in 378 of 378 combinations. The three #191 mutants are now caught. Nothing blocks the merge. The one real finding is a small test gap in the hostile-value list (finding 1).

Findings

#	Priority	Where	Finding
1	Low (test gap)	test-analytics-tab-state-and-query.js, hostileTabs	[T] Three of my mutants of the new lookup survive all 39 tests: matching b.dataset.tab.indexOf(urlTab) >= 0 (substring), indexOf(urlTab) === 0 (prefix) and a case-insensitive compare. The hostile list has no partial or differently-cased real tab name. The shipped code is correct, as the browser run shows ?tab=RF, ?tab=ov, ?tab=Overview and ?tab=overview%20 all give Overview. This is only a gap in what the unit test pins. Suggested values to add: 'RF', 'ov', 'rf-', 'overview '.
2	Nit	init() in public/analytics.js	[T] A rejected ?tab= stays in the hash until the first tab click or time-window change rewrites it, and the page is correct meanwhile (see point 1 below). No action needed.
3	Info	withQuery	[T] For a lone & the old helper returned /a? and the new one returns /a. That is a fix, and no caller can produce it, because regionQueryString() and areaQueryString() return '' or &key=value. [A] A path that already ends in ? gets & appended (/a?&x=1). That is harmless and unreachable.
4	Info (coverage limit)	area filter in the browser	[T] The fixture has no areas (/api/config/areas is []) and AreaFilter.init hides its container (area-filter.js:59), so analyticsAreaFilter is display:none on every tab in a real browser. The setAreaFilterVisibility coverage therefore exists only in the unit test with the fake DOM; I could not observe it in Chromium.
5	Info (unrelated)	#/analytics?tab=rf-health	[T] Two console 500s from /api/observers/metrics/summary?window=24h on the fixture. They are identical with master's public/, so not related to this PR.

Answers to points 1–7

1. Escape-safe ?tab=

• No exception, no wrong tab [T]. In Chromium on the merged tree I loaded #/analytics?tab=<v> for 21 values, URL-encoded: x", x"],[data-tab="overview, x"],[data-tab="rf, overview"], ], \, ü☃日本, empty, unknown, __proto__, <img src=x onerror=alert(1)>, '; alert(1); //, rf-health"], Overview, RF, ov, overview (trailing space), %, 5000 × a, constructor, toString. I also set 6 values raw (unencoded) in location.hash. In all 27 cases: Overview button active (only that one), Overview content rendered ("Total Transmissions"), not stuck on "Loading analytics…", no page error, no console error, no dialog (the <img onerror> value did not execute).
• Master baseline [T]. With master's public/ on the same DB: x" throws '[data-tab="x""]' is not a valid selector and stays on "Loading analytics…". x"],[data-tab="overview hangs on "Loading analytics…". x"],[data-tab="rf has the RF button active and hangs. So the bug reproduced and is fixed.
• Same document, after a real tab [T]. Topology, then #/nodes, then #/analytics?tab=x"],[data-tab="topology gives Overview in button and content, no error. Clicking RF then Overview afterwards works.
• Deep links [T]. All 20 data-tab values (overview … prefix-tool) activate their own button, with no page error.
• Code check [A]. init() only compares btn.dataset.tab === urlTab over .tab-btn inside #analyticsTabs, which is the same scope the old selector had. __proto__ and constructor cannot match a string compare. _currentTab can only ever be a real tab or 'overview'. The same file's other URL values (section, window) were already handled safely. I scanned public/*.js for other selectors built from values: the two other sites I found (channels.js:1829, packets.js:3204) already use CSS.escape, so analytics.js was the only offender.
• Leftover value in the hash: acceptable [T]. After ?tab=bogus, the hash stays #/analytics?tab=bogus. A click on any tab rewrites it (…?tab=rf, or no tab for Overview), and a time-window change rewrites it too (#/analytics?window=7d). Reload with the leftover value gives Overview. The state is correct, so I would not add a replaceState in init().

2. withQuery

• Semantics [T]. The 14-row edge table in the test matches what I probed. Empty, undefined, null, lone & and lone ? leave the path unchanged. &a=1, ?a=1 and bare a=1 are all a=1. The separator is & when the path has a ?.
• Byte-identical to master [T].
  • Fuzz: old formula vs new over 222 (path, fragment) pairs in the old function's domain (path without ?, fragment empty or &…, up to two tokens including %2C, a%26b, x y, = and &): 0 differences.
  • End to end: the real page, with master's analytics.js and with head's, in the vm harness. 3 regions (none, SJC, A,B encoded) × 3 areas (none, north, a&b encoded) × 2 windows (none, 7d) × 20 tabs, plus the Prefix Tool request after a cache drop: 378 combinations, the /api/analytics/* request lists are identical (diff of 0 lines).
• Exposure on window [A]: fine. analytics.js already exports ~29 window._analytics* hooks in the same typeof window block, and the new line follows that naming. Other pages use _xxxForTest suffixes (channels.js), but the local convention in this file is the plain _analytics* prefix.

3. Test gaps and mutants

All of them run against the new test file on a copy of analytics.js with one change [T].

Mutant	Result
Author's 8 (neighbor-graph drops min_count/min_score; Prefix Tool drops area; setAreaFilterVisibility removed from init(); selector lookup restored; unknown tab kept as _currentTab; withQuery without & join, strip only &, back to slice(1))	all caught (1, 1, 2, 6, 8, 3, 3, 7 failing tests)
neighbor-graph drops the region only / drops only min_score	caught (1 each)
Prefix Tool drops the region, keeps the area	caught
rf loses the window / channels also gets the area / distance loses the region	caught (1 each)
setAreaFilterVisibility removed from the tab click handler	caught
AREA_FILTER_TABS gains prefix-tool / loses overview	caught (1 and 2 failing tests)
withQuery strips only a leading ? / keeps the lone separator / separator inverted	caught
Overview button not activated on fallback / active class not cleared / _currentTab = urlTab || 'overview'-style variants	caught
window._analyticsWithQuery export removed	caught (16 failing)
tab lookup by substring, by prefix, case-insensitive	survived (finding 1)
urlTab ? … : null guard removed	survived, equivalent (a null urlTab matches no button)

My first run of the AREA_FILTER_TABS mutant hit LOAD_TABS (an earlier Set with the same prefix) and showed "survived". That was my harness; applied to the right Set it is caught.

Red before, green after [T]. The new file against pure master analytics.js: 17 passed, 22 failed (the hostile-tab cases plus 16 withQuery tests that cannot run without the export). Against master analytics.js plus only the export line: 26 passed, 13 failed, which matches the author's number. On head: 39 passed, 0 failed.

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

• Assertions unchanged [T]. git diff origin/master...head on the file is one hunk: 11 lines out, 11 in, all inside the elFor() mock element in makeSandbox(). It replaces querySelector(sel) (a generic [data-tab="x"] regexp) and the empty querySelectorAll by querySelector() { return null; } and a querySelectorAll('.tab-btn') that lists overview and scopes buttons with dataset.tab set. Every check()/assert line and scenarios A–F are byte-identical.
• Same discriminating power [T]. Five mutants, run for old-test + master-code and new-test + head-code, give identical results:

Mutant	Old test + master	New test + head
Scopes load uses /api/scope-stats (doubled prefix)	3 pass, 6 fail	3 pass, 6 fail
Foreign Traffic uses /api/scope-stats	3 pass, 6 fail	3 pass, 6 fail
Foreign Traffic asks window=7d (no cache sharing)	5 pass, 4 fail	5 pass, 4 fail
Scopes load ttl: 0	7 pass, 2 fail	7 pass, 2 fail
?tab=scopes never reaches the Scopes tab	fails	fails

• Baselines: old test + master code 9 passed, 0 failed. New test + head code 9 passed, 0 failed.
• The new test on master's analytics.js fails (exit 1) because the new fake no longer answers querySelector('[data-tab=…]'). That is expected: the fake now models the new lookup. The test is not weaker; it is coupled to the .tab-btn listing, which is what init() does.

5. Tests

• sh test-all.sh [T]: merged tree against 8e4b13d0: 201 passed, 0 failed (201 files). Merged tree against 457dbf34: 201 passed, 0 failed (201 files). test-test-all.js (registration guard) passes.
• node test-frontend-helpers.js [T]: 707 passed, 0 failed, on both merged trees.
• New test file [T]: red before, green after, as in point 3. 39/0 on both merged trees.
• CI [T] (from gh pr view): Go Build & Test, Playwright E2E and Docker build are SUCCESS on head; Release, Deploy Staging and Badges are SKIPPED, as expected for a draft.

6. Browser [T]

Go server built from the merged tree (8e4b13d0 base), e2e-fixture.db freshened (tools/freshen-fixture.sh), migrated (corescope-migrate) and seeded (seed-2073-route-adverts.sql) as in CI. I confirmed by pid that the process on the port had the scratch tree as its working directory and executable, and that the served /analytics.js is byte-identical to the head file. A second server served master's public/ on another port for the baseline. Both were stopped by pid; I confirmed the ports are free. Chromium through Playwright:

• Hostile ?tab= values: point 1.
• Prefix Tool with region SJC (stored as the region filter): one hash-sizes?region=SJC request, 200, application/json, whether opened by deep link or by clicking from Overview (the click case reuses the cached response from the shared load). No page or console error.
• fix(analytics): after leaving and returning to #/analytics, Overview is marked active but the old tab's content is shown #183 scenario: Topology, then #/nodes, then #/analytics: Overview in button and content, hash #/analytics. Back goes to #/nodes, forward returns to Overview.
• Console: no errors in any of these scenarios. The only console errors in the whole run are the unrelated rf-health 500s in finding 5, identical on master.
• I looked at a screenshot of ?tab=x"],[data-tab="rf: the Overview button is active and the Overview content is rendered.

7. Rules

• scripts/check-xss-sinks.sh --diff origin/master: clean (exit 0), against both 8e4b13d0 and 457dbf34. [T] (run in a scratch git repo with the two commits).
• Hardcoded colours: none in the added lines (the only #-hits are (#193) references in comments). [T]
• deploy.yml is untouched by the PR and has 9 × github.repository == 'Kpa-clawbot/CoreScope' on the merged tree. [T]
• PR body starts with "Relates to analytics: escape-safe ?tab= lookup, pin remaining #191 test gaps, withQuery contract #193" and has no closing keyword; the three commit messages have none either. [T]
• Perf: no new API calls; init() builds one array per mount from the 20 tab buttons, and withQuery is O(length of the fragment). [A]

Not verified

• The area filter's visibility in a real browser (finding 4): the fixture has no areas, so only the unit test with the fake DOM covers it.
• The Go server was built from the tree merged with 8e4b13d0 and not rebuilt on 457dbf34. feat(channels): opt-in auto-approval for new shared channels #196 changes cmd/ingestor and internal/channelregistry, not public/ or the analytics endpoints; the Node tests and the static checks above were re-run on the 457dbf34 merge.
• [K] The author's 503 warm-up retry-chain check (stops on leaving the page, no doubling on re-mount) was not re-run; the PR does not touch that code. Everything else from the author's report that I rely on, I re-ran myself.
• The full Playwright E2E suite from CI was not run locally; I relied on the green CI run on head.
• Mobile layouts and pixel-level rendering of the tab bar.
• The Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 grouped-packet fixture insert was skipped; the analytics page does not use it.