Summary
These are follow-ups from the review of #191 (merged as 8e4b13d0). All are low priority. None blocked the merge.
1. ?tab= is interpolated into a querySelector selector without escaping (pre-existing)
init() in public/analytics.js builds an attribute selector from the ?tab= value.
Suggested fix: look the button up by comparing btn.dataset.tab === tab over the tab buttons, not with a selector. Fall back to Overview for unknown values. Add a test with " and with the crafted value.
2. Test gaps in test-analytics-tab-state-and-query.js
Three of the reviewer's mutants survive:
- The neighbor-graph URL is not pinned by the test, although the PR text says it is.
- The Prefix Tool's own request can't be told apart from the shared load with the same URL. A mutant that drops the area filter from the Prefix Tool call passes.
setAreaFilterVisibility in init() is not covered.
Add assertions that kill these mutants.
3. withQuery(path, frag) contract
withQuery expects an &… fragment and a path without a query:
withQuery('/a', 'region=X') returns /a?egion=X;
- a path that already contains
? gets a second ?.
No current caller hits these cases. Either make it tolerant (accept '', &… and ?…, and append with & when the path already has ?) or document and assert the contract. Add unit tests for the edge cases.
Acceptance criteria
Summary
These are follow-ups from the review of #191 (merged as
8e4b13d0). All are low priority. None blocked the merge.1.
?tab=is interpolated into aquerySelectorselector without escaping (pre-existing)init()inpublic/analytics.jsbuilds an attribute selector from the?tab=value.A value containing
"throws ininit(), and the page stays on "Loading analytics…".A crafted value such as
x"],[data-tab="overviewmatches a real button:_currentTabbecomes the arbitrary string;This is the same class of symptom 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 can trigger it.
It is not XSS. The value is only compared and used in a
switch, never written into HTML.Master behaves the same (it is not a fix(analytics): Prefix Tool hash-sizes query (#179) and stale tab on return (#183) #191 regression).
Suggested fix: look the button up by comparing
btn.dataset.tab === tabover the tab buttons, not with a selector. Fall back to Overview for unknown values. Add a test with"and with the crafted value.2. Test gaps in
test-analytics-tab-state-and-query.jsThree of the reviewer's mutants survive:
setAreaFilterVisibilityininit()is not covered.Add assertions that kill these mutants.
3.
withQuery(path, frag)contractwithQueryexpects an&…fragment and a path without a query:withQuery('/a', 'region=X')returns/a?egion=X;?gets a second?.No current caller hits these cases. Either make it tolerant (accept
'',&…and?…, and append with&when the path already has?) or document and assert the contract. Add unit tests for the edge cases.Acceptance criteria
?tab=value with", or the crafted value above, shows Overview with the Overview button active, and there is no exception (tests).withQueryedge cases are tested.