Repository navigation
fix(ui): escape untrusted node, observer and route values in HTML rendering - #373
Conversation
…dering Relates to #333 Escape mesh/MQTT-supplied strings at the sinks named in the issue so they display as inert text rather than markup: - favorites dropdown node name (app.js) - geo-filter prune preview node name (customize-v2.js) - observer region badges, table + slide-over (observers.js) - packet region badge (packets.js) - RF-health observer_id in data-observer / sparkline id attributes (analytics.js) - unknown-route heading, plus swap its hardcoded colour for var(--text-muted) (app.js) Clock-skew observer/node names were already escaped in our fork; a regression pin with a mutant guards them. Close the #351 N4 gap in scripts/check-xss-sinks.sh: an unescaped interpolation inside a quoted attribute on a sink line is now flagged (the quote-stripper preserves ${...} fragments). Document the residual limitation. Tests: hostile-string unit sinks + browser-level E2E (unknown route, observer region, packet region badge, favorites) + gate fixtures (bad-13/14, good-5). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rapport — CS-MacBook PR#373 #333 — head 97b4a1fStatus: Implemented and self-verified; draft open, all CI green. No merge/ready/approve actions taken. #271 flake not observed (no rerun needed). Scope recheckRechecked current master ( Per-requirement evidence (test red→green + mutant)
Preservation [T]: E2E asserts benign Constraints [K]
Local suites [T]
CI per job [T]
Run: actions/runs/37751681187. No job failed; known flake #271 not triggered, so no rerun. Residual / notes
|
dborup
left a comment
There was a problem hiding this comment.
Review of head 97b4a1f6 against the current PR base: no merge-blocking findings in this diff. The changed HTML sinks now escape node names, region labels, route text, and RF-health attribute values in the right context. In particular, analytics.js's esc() encodes both quote characters, so the new data-observer and id interpolations cannot break out of their quoted attributes. The quoted-attribute XSS-gate fixtures exercise the reported false-negative, and CI is green for Go, Playwright, and the local Docker build.
Non-blocking follow-up outside this PR: public/analytics.js:4561 still puts _rfHealthState.selectedObserver (read from the URL hash at line 4422) directly into querySelector('[data-observer="…"]'). A crafted observer parameter containing a quote can make that selector invalid and send the RF-health grid to its error state. This predates #373 and is a CSS-selector/functional issue, not an HTML-escaping regression. Please track it separately; use CSS.escape or select cells by comparing dataset.observer rather than interpolating the raw ID into selector syntax.
The new browser test renders extracted production sink fragments, not the complete page handlers; it is useful parser-level evidence but does not by itself prove every full navigation path. No PR changes requested on that basis. I have not merged or changed the branch.
Review — CS-pve-agent2 PR#373 — head 97b4a1fDom: REQUEST CHANGES Evidence tags: [T] = I ran it myself. [A] = based on reading the code. [K] = taken from the author's report or CI and not reproduced by me. The seven sinks this PR changes are fixed correctly. Each one uses the right escaper for its context, nothing is double-escaped, and every fix has a unit test that is red before the fix and green after. The issue's first acceptance criterion also covers adjacent rendering sites, and that criterion is not met yet. Two of the three packet region-badge render paths are still unescaped. The shared region filter also renders a hostile observer IATA as live markup on page load, on Observers, Packets, Nodes and Analytics. I reproduced both in Chromium against a local server, and both also inject on the merged tree. Findings
Answers to the review pointsCoverage of the sinks named in the issue
Context and double-escaping. [A]/[T] Every change uses Gate (#351 N4). [T] With the Behaviour preserved. [T] Clicking an observer row navigates to the detail page, the narrow-viewport slide-over opens, favorites navigate, group expand/collapse works, and the observer sort key ( Checklist
Tests and mutantsThese ran on the merged tree,
Mutants [T] (unit =
Browser verification[T] This used Playwright/Chromium against a local Go server only. Hostile IATA values and node names were injected into a copy of the CI-prepared fixture, and the same checks ran against the merged tree and against master.
The flat-view counts differ between the two runs (13 vs 15); I did not look into why. Benign values Suggested fix scope
Not verified
|
…her shape (#333) Review of PR #373, findings F5 and F6: - F5: the geo-filter prune preview, clock-health and RF-health tests injected app.js's escapeHtml as `esc`, so the escaper those files ship was never exercised. They now lift `function esc(s)` from customize-v2.js and analytics.js. analytics.js's esc() gets a 5-char contract test, because the RF-health attribute fix depends on it encoding both quotes. The prune preview also runs in Chromium with the real textContent-based esc(). - F6: the fragment E2E regexes for the observer and packet region cells, and the clock-health unit regex, matched only the escaped form, so a mutant failed with "sink not found". They now match both forms and fail on injected markup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of PR #373 found three more places that put observer-controlled values into HTML unescaped: - F1: packets.js region badge in expanded group child rows and in the flat view (only the group header was fixed). - F2: region-filter.js data-region attributes, pill/dropdown labels and the dropdown trigger label, rendered on every page with the filter. - F3: observers.js row onclick="location.hash='…${encodeURIComponent(id)}'"; encodeURIComponent leaves ' alone, so an id with a quote runs as JS. Unit tests (test-xss-escape-sinks.js §J) drive the real buildFlatRowHtml/buildGroupRowHtml and RegionFilter render with app.js's escapeHtml as the page global. test-xss-333-pages-e2e.js drives the real pages against the CI fixture server, rewriting /api/observers and /api/config/regions in the browser to inject hostile codes, and checks that filtering by a hostile code and row navigation still work. Red on this commit; the fixes follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t view (#333) Review of PR #373, finding F1. The group-header badge was escaped in round 1, but the child rows (childRegion) and the flat view (region) put the same observer IATA into the same <span class="badge-region"> sink raw. Wrap both in escapeHtml, as the header does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of PR #373, finding F2. Region codes include observer IATA values from MQTT and were concatenated raw into data-region attributes, the pill/dropdown label text and the dropdown trigger label, rendering on page load on Observers, Packets, Nodes, Analytics, Channels and Live. Escape each with the global escapeHtml (5-char OWASP set, so it is safe in the double-quoted attributes too). Selection keeps working because toggleRegion() reads btn.dataset.region, which the parser has already decoded back to the exact code. The setSelected sandbox in test-frontend-helpers.js now loads roles.js + app.js like the page does, so the global escaper exists there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of PR #373, finding F3. The row carried onclick="location.hash='#/observers/${encodeURIComponent(o.id)}'". encodeURIComponent does not encode ', so an observer id containing a quote broke out of the JS string and ran (observer ids come from the MQTT topic). The row already has data-action="navigate" with the same target in data-value, handled by the delegated click/keydown listeners, so the inline handler is removed. This also restores the Kpa-clawbot#1056 behaviour on narrow viewports: the inline handler navigated to the detail page even when the delegated handler opened the slide-over instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… misses (#333) Review of PR #373, finding F4. New fixtures for scripts/check-xss-sinks.sh: - bad-15: el.innerHTML = '<span title="' + o.name + '">' passes the gate: the "…" pass takes `"' + o.name + '"` for one string and erases the field. - bad-16: the mirror with "…" pieces around a '…' attribute (already flagged; pinned). - bad-17: '<a href="https://…">' + o.name passes: `//` inside the string is stripped as a comment before strings are parsed. - good-6: the escaped forms must stay clean. - good-7: a plain '…${name}…' literal is flagged by the round-1 N4 workaround although it is not an interpolation. Red on this commit (bad-15, bad-17, good-7); the gate fix follows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mit (#333) Review of PR #373, finding F4. strip() erased "…" runs, then '…' runs, then // to end of line, each across the whole line, so one quote kind could swallow code between strings of the other kind and a URL was cut at `//`. It now walks the line once, recognising '…', "…", `…` and // comments in order. A quote inside a string or template is then just text, so the round-1 workaround that kept ${…} inside "…" runs is gone, along with its false positive on plain '…${x}…' literals. Measured on every public/*.js: the gate reports the same 3 pre-existing flags (bottom-nav.js x2, nav-drawer.js) before and after. The gate still only audits the line that holds the sink. Every #333 sink builds HTML on another line (row builders returning strings), so it cannot catch those mutants; the DOM-level tests do. Flagging every HTML-building line was measured at 224 hits on public/*.js, nearly all pre-escaped locals or static text, so it is not enabled. The script header documents this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent1 PR#373 runde-2 — head a031513Review feedback addressed (commit Evidence tags: [T] = I ran it myself. [A] = based on reading the code. [K] = taken from CI or the review and not reproduced by me.
No other changes. Tests (red first, then green)
New tests:
Mutants [T]U =
Every sink mutant is red in at least one suite. "Not flagged" in the G column is the documented same-line limit (item 5). M12 has no unit test (unchanged from round 1); it is caught behaviourally by the fragment E2E. Local suites [T]
CI per jobRun
The Playwright log shows Not done / notes
|
Review — CS-Macmini PR#373 — head a031513Dom: REQUEST CHANGES Evidence tags: [T] = I ran it myself. [A] = from reading the code. [K] = taken from the author's report or CI and not reproduced by me. Round 2 resolves every finding from round 1. F1, F2 and F3 are fixed with the right escaper for each context, each one has a test that is red on master code and green here, and the gate change (F4) is a real improvement with its remaining gap documented in the script header. F5 and F6 are closed: the tests now run the escapers the production code actually calls, and the sink regexes match the escaped and the unescaped shape, so mutants fail behaviourally. The verdict is still REQUEST CHANGES for the same reason as round 1, and only that reason: the issue's first acceptance criterion covers adjacent rendering sites, and two more are still unescaped. Both render the identical value — observer IATA from Findings
Answers to the review pointsF1 — F2 — F3 — F5 and F6 — tests run the real escapers; E2E regexes are not hardcoded. Both closed. [T] F4 — gate extended or limitation documented. Documented, and the script is genuinely better. [T] More adjacent sinks of the same type. Yes — N1 and N2 above. I scanned Scope creep. None. [T] 21 files: 6 Checklist
Tests and mutantsEverything ran on the merged tree against current
Mutants [T] — U =
Not verified
|
…#333) Round 3 of PR #373, addressing the re-review (N1-N4). N1 nodes.js: the node-detail panel's "Regions:" summary joined observers.map(o => o.iata) raw into panel.innerHTML, one line above the already-escaped per-observer badge. Wrap each code in escapeHtml, matching the full-page view at nodes.js:970. N2 live.js: showNodeDetail's "Heard By - Regions: ..." heading joined the same observer IATAs raw into content.innerHTML; the per-observer rows one line below were already escaped. Same escapeHtml(join) fix. N3 analytics.js: renderRepeaterTable's row carried onclick="location.hash='#/nodes/${encodeURIComponent(r.pubkey)}'". encodeURIComponent leaves ' ( ) alone, so a pubkey quote breaks out of the JS string - the pattern F3 removed from observers.js. Drop the inline handler, navigate via data-action="navigate"/data-value, and broaden the delegated handler's selector from tr[...] to [...] so the div row is covered. tabindex/role match the sibling table rows for keyboard access. N4 test-xss-escape-sinks.js: loadLocalEsc lifts the first one-line function esc(s); assert exactly one esc() definition per file so a second one added above the real escaper fails loudly instead of silently retargeting the contract test. Tests: Section K drives the real extracted production expressions for N1/N2/N3 with a hostile IATA/pubkey (red before, green after); a new N1 step in test-xss-333-pages-e2e.js injects the hostile IATA into /api/nodes/{pk}/health and asserts the detail panel stays inert. Relates to #333 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rapport — CS-MacBook PR#373 runde-3 — head 8474b49Review feedback addressed (commit
Systematic sink sweep —
|
| File:Line | Value (source) | Status |
|---|---|---|
| nodes.js:2005 | iata region join (observer health) |
RAW → FIXED (N1) |
| live.js:2727 | iata region join (observer health) |
RAW → FIXED (N2) |
| analytics.js:1105 | r.pubkey in inline onclick JS-string |
RAW double-context → FIXED (N3) |
| nodes.js:970, 2013; live.js:2729; observers.js:430,544; observer-detail.js:389; compare.js:163; map.js:2340; analytics.js:6812 | iata / region badges |
SAFE — escapeHtml/esc at sink |
packets.js (groupRegion/childRegion/region badges, scope via scopeCellHtml/transportBadge, pkt.scope_name) |
region / scope | SAFE — escaped (F1 + pre-existing) |
| region-filter.js (pills, dropdown items, trigger) | region codes/labels | SAFE — escaped (F2) |
analytics.js:5445/5454 regionsHtml |
esc(r) |
SAFE — pre-escaped local |
| node names across nodes.js/live.js/home.js/analytics.js/compare.js/new-nodes.js/node-changes.js/reach-rank.js | node .name |
SAFE — esc/escapeHtml at sink |
channels.js channel names; home.js labels (escapeAttr/_escHtml) |
channel/name | SAFE — escaped |
| hop-display.js:96 | static onclick + escapeHtml(JSON.stringify(...)) data-attr |
SAFE — static handler, escaped attr |
| compare.js:529 | escapeHtml(h) in inline onclick (packet hash) |
SAFE in practice — hash is hex; inline onclick is load-bearing (compare.js has no delegated click handler for nav rows, only keydown at :114), so removing it would regress mouse navigation. Left unchanged, consistent with the round-2 acceptance; noted that escapeHtml alone does not neutralise ' in this double context — only the hex constraint does. |
| live.js:2729/2737; analytics.js:1152/1203/2266/4017; home.js:201 | encodeURIComponent(observer_id/pubkey/q) in #/… href |
SAFE — URL context, not JS-string |
favStar (app.js:712) + data-fav/data-key/href pubkey sites in home.js, nodes.js, channels.js, map.js, live.js, packets.js |
raw public_key/pubkey in double-quoted attributes |
SAFE in practice — pubkeys are hex, no breakout char; pre-existing; defense-in-depth escaping is a broader change outside this round |
app.js routeTypeName/ROUTE_TYPES; observers.js h.label; static card/label configs |
numeric / fixed in-code enums | SAFE — not untrusted |
| perf.js:335,345 | s.path / q.path (HTTP request-path telemetry on the admin perf dashboard) |
RAW, but out of #333 scope — not observer/region/node/scope/channel/route mesh data; low exploitability (endpoint paths); recommend a separate issue rather than widening this PR. [NEW find from the sweep] |
| area-filter.js:75 | a.key / a.label (/api/config/areas, operator config) |
Out of scope — operator config, not MQTT/advert (as agreed round 2) |
customize.js / customize-v2.js logo-URL <img src> |
branding self-config | Out of scope — operator self-config (self-XSS) |
Tests, mutants
- [T]
node test-xss-escape-sinks.js→ 56 / 0 (was 53; +3 for N1/N2/N3 in new Section K). The N4 assert is live inloadLocalEsc. - [T] Red-before / green-after, each test drives the real extracted production expression with a hostile value:
- N1 nodes summary: red
raw <img> survived→ green. - N2 live heading: red
raw <img> survived→ green. - N3 repeater row: red
node pubkey reaches an inline JS handler→ green.
- N1 nodes summary: red
- [T] Mutants, one per finding (each applied to the fixed tree, suite run, then restored):
- N1 — revert
regions.map(escapeHtml).join→regions.join: unit red; new E2E step red with a live injection (1 injected <img>, 2 handler calls). - N2 — revert
regions.map(escapeHtml).join→regions.join: unit red (raw <img> survived). - N3 — re-add the inline
onclick: unit red (pubkey reaches an inline handler). - N4 — inject a second
function esc(:loadLocalEscthrows (count 2).
- N1 — revert
- [T]
node test-frontend-helpers.js→ 712 / 0.node test-preflight-xss-gate.js→ all pass.sh test-all.sh→ 234 / 0.scripts/check-xss-sinks.sh --diff origin/master→ exit 0. - [T] E2E against a local Go server (port 13900) with the CI-prepared
e2e-fixture.db(freshen, Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL fromdeploy.yml,corescope-migrate, seeds 2073/199/245):test-xss-333-pages-e2e11 / 0 (10 round-2 steps + the new N1 panel step).test-xss-333-e2e8 / 0,test-issue-1639-observers-sort-e2epass,test-slideover-1056-e2e27 / 0,test-issue-199-inactive-observer-e2e3 / 0,test-filter-ux-e2e12 / 0,test-observer-iata-1188-e2epass (the step the round-2 reviewer saw as environmentally flaky passed cleanly here).
- [T] Go:
cmd/servergo test ./...→ok 43.5s. No Go files changed in this round.
N2 is covered by the unit test plus a static read, not by a browser click: reaching it live needs a Leaflet marker click on the Live map, which I did not wire. N1 is the one the fix is most load-bearing on and it has the full E2E.
CI per job
[K] Run 38322081004, head 8474b494, read per job via gh run view:
| Job | Result | Time |
|---|---|---|
| Go Build & Test | ✅ success | 23m |
| Playwright E2E Tests | ✅ success | 28m |
| Build & Publish Docker Image | ✅ success | ~1m |
| Release Artifacts | ⏭️ skipped (fork guard) | — |
| Deploy Staging | ⏭️ skipped (fork guard) | — |
| Publish Badges & Summary | ⏭️ skipped | — |
The Playwright log shows test-xss-333-pages-e2e.js at 11 passed, 0 failed (the new ✓ N1 nodes detail panel: Regions summary renders the hostile IATA as text step included) and test-xss-333-e2e.js at 8 passed, 0 failed. No ✗/❌ line anywhere in either job log; neither known flake (#271, #301) occurred, so no rerun was needed.
Not done / scope
- No merge, ready, issue closing, stg/prod access, upstream writes or API-key use. No payload left the machine — hostile values were injected by rewriting API responses in the browser; the fixture DB was never written with a payload.
perf.js:335/345(above) is a genuine raw sink but outside security(ui): escape untrusted node, observer and route values in HTML rendering #333's data categories; flagged for a separate issue rather than changed here.compare.js:529and the pubkey-in-attribute sites are left unchanged with the reasoning in the table.
Review — CS-Macmini PR#373 — head 8474b49Dom: APPROVE med nits Evidence tags: [T] = I ran it myself. [A] = from reading the code. [K] = taken from the author's report or CI and not reproduced by me. Round 3 closes both round-2 blockers and the three nits. N1 and N2 are escaped with the right escaper, each has a test that is red on the unmutated-escaper code and green here, and I reproduced N2 as a live injection in a real browser on the Live map — the one piece of evidence neither the author nor my round-2 review had. N3 removes the last dynamic inline The nits below are all non-blocking and none of them is an escaping problem. The one I would most like fixed before merge is N6: the Findings
Answers to the review pointsN1 — N2 — N3 — The sink list, verified independently. Verified, and I found nothing missing. Method [T]: 531
N4 — Browser check with hostile region, observer and node names. Done, on a local Go server with the CI-prepared fixture. [T] The PR's own Navigation, favorites and Unicode. All intact. [T] Favorites: starring a node writes the exact pubkey to Checklist
Tests and mutantsEverything ran on the merged tree against current
Mutants [T] — U =
Not verified
|
Relates to #333
What & why
Mesh/MQTT-supplied strings and URL-hash route text were interpolated into HTML
without escaping at several render sinks, so a hostile node/observer/region name
(or a crafted
#/…route) could be interpreted as markup. This escapes thosevalues as text and closes a false-negative in the shared XSS gate.
Rechecked against current master (
1f423e06); the issue was written against6444294d. The clock-skew observer/node-name sinks were already escaped in ourfork, so they get a regression pin rather than a code change.
Approach (plan)
app.js,analytics.js,customize-v2.js,live.js,nodes.js,observers.js,packets.jsandadjacent fork sinks.
escapeHtml(HTML/attr, 5OWASP chars incl. quotes) and the per-file textContent-based
esc(). No newhelpers; no
encodeURIComponent/CSS-selector escaping confusion.browser-level E2E and extend the gate with a fixture + the N4 fix.
Sinks fixed
app.jsescapeHtml(h.node.name …)customize-v2.jsesc(n.name …)observers.jsescapeHtml(o.iata)observers.jsescapeHtml(o.iata)packets.jsescapeHtml(groupRegion)data-observer/ sparklineidanalytics.jsesc(obs.observer_id)<h2>headingapp.jsescapeHtml(route)+var(--text-muted)analytics.jsXSS gate (
scripts/check-xss-sinks.sh) — #351 N4The gate did not flag an unescaped
${…}inside a quoted attribute on a sinkline (
el.innerHTML = \x`): the quote-stripper erased the interpolation before the template-literal handler ran.short_strnow preserves${…}fragments inside quoted runs, so such sinks are flagged while escaped ones (escapeHtml(…)) still pass. The remaining limitation (a literal"${allowlisted}"in a plain, non-template string on a sink line, and the cross-line.join('')` indirection) is documented in the script header.Tests
test-xss-escape-sinks.js, section I): one hostile-string sink testper point — text + attribute contexts (
<,>,&,",', backtick,javascript:style payloads). Each extracts the production template, sodropping the wrapper flips it red.
test-xss-333-e2e.js): real Chromium parses theproduction-rendered HTML and asserts an inert DOM (no
<img>/handler element,no fired side effect) plus verbatim display of benign Unicode (
&, Danishletters, emoji) for unknown route, observer region, packet region badge and
favorites. Registered in
deploy.yml.test-preflight-xss-gate.js+ fixturesbad-13,bad-14,good-5): proves the N4 pattern is now caught and escaped attributes don'tfalse-positive.
Constraints
cmd/serveruntouched; no Go changes; no newmap[string]interface{}.var(--text-muted)).scripts/check-xss-sinks.sh --diff origin/masteris clean.deploy.yml, 1 inrelease-fast-path.yml.