Repository navigation
fix(ui): escape perf dashboard values (#392) - #394
Conversation
Follow-up to #333, carved out of PR #373. public/perf.js interpolated /api/perf values into innerHTML without escaping. The request-controlled sinks are the server-endpoints table key (unmatched /api/ routes collapse to the raw path) and the slow-query path (raw r.URL.Path at cmd/server/routes.go), so a slow 404 under /api/ could carry markup. Frontend (public/perf.js): - Add an IIFE-local escapeHtml that prefers app.js's canonical global and falls back to an identical 5-char OWASP escaper, so perf.js is safe both in the browser and when evaluated standalone (perf-render unit sandboxes). - Escape every interpolated string value: endpoint key, client path and slow-query path (the three row tables), write-source name + sampleAt, server uptime string, and the error fallback. Numeric metrics typed `number` in api-spec.md are left as-is (they coerce, cannot carry markup). Server (cmd/server/routes.go, read-only, no new map[string]interface{}): - Normalise SlowQuery.Path via a pure normalizeSlowPath helper: pair the :id-folded route key with a control-char-stripped, rune-safe, length-capped (<=128 runes) raw path, matching the endpoint key's treatment and bounding the in-memory ring. It does NOT HTML-escape — the dashboard does that at render time. Documented in docs/api-spec.md. Tests (red before, green after; one mutant each): - test-xss-escape-sinks.js: 6 hostile-string unit tests driving the real extracted perf.js row templates via new Function (vm-style), asserting the <img> payload is escaped; dropping escapeHtml flips them red. - test-xss-392-perf-e2e.js: full-page #/perf E2E against a LOCAL Go server with /api/perf mocked in-browser (page.route) to a hostile endpoint key + slow-query path; asserts no injected <img>, no onerror, no dialog, and the payloads render as inert text. Wired into deploy.yml's Playwright step. - cmd/server/perf_slowpath_392_test.go: table tests for normalizeSlowPath (template+raw, unmatched dedupe, rune-safe cap, control-char strip, no-escape); removing the cap flips the long-path case red. check-xss-sinks.sh cannot flag perf.js's pre-fix pattern: perf builds HTML via an `html += ` accumulator and does one `el.innerHTML = html`, and the per-line regex only treats `.innerHTML/.outerHTML/.srcdoc` assignment lines as sinks, so the node-controlled ${...} interpolations on `html +=` lines are never audited. The unit + E2E tests (with mutants) are the regression guard instead. Relates to #392 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rapport — CS-MacBook PR#394 #392 — head 37ebc67Status: Done — draft PR open, CI fully green (incl. the new #392 E2E in CI); no outstanding failures. Requirements → test + mutant
All six unit tests drive the real extracted Mandatory checks
|
| Job | Result |
|---|---|
Go Build & Test (incl. go server build+test, test-all.sh 235/235, Preflight XSS --diff, eslint no-undef) |
✅ success [T][K] |
Playwright E2E Tests (incl. test-xss-392-perf-e2e.js → 1 passed, 0 failed) |
✅ success [T][A] |
| Build & Publish Docker Image | ✅ success |
| Release Artifacts / Deploy Staging / Publish Badges | ⏭️ skipped (fork-guarded) |
No flaky #271 occurrence; no re-run needed.
Local verification
node test-xss-escape-sinks.js→ 62/0 [T]node test-frontend-helpers.js→ 712/0 [T]sh test-all.sh→ 235/235 [T]test-xss-392-perf-e2e.jsvs local Go server (port 13900, mocked/api/perf) → 1 passed; mutant → red [T][A]go test .incmd/server→ ok;go test -raceon perf/slow-path → ok [T]
Remainders
- None blocking. The server-side normalisation is additive defense-in-depth (length cap + consistency with the endpoint key); the frontend escaping alone already neutralises the XSS.
Review — CS-Macmini PR#394 — head 37ebc67Dom: APPROVE med nits Independent, read-only review. Verified against Findings
F1 detail — per-field browser isolation on head, hostile value injected one field at a time into a mocked
All twelve are The review pointsCoverage — are all interpolated values escaped by context? Grepped all 70 Server-side Behaviour change beyond the purpose. Mandatory checks. Acceptance criteria → test. Each of the three sinks named in the issue has a dedicated unit test plus the E2E, red before and green after (mutants below). The criterion "make the gate cover these sinks" is the one not met — F5. Tests run (merged tree =
|
| Run | Result |
|---|---|
go test ./... in cmd/server |
ok, 39.9s [T] |
go test ./... in cmd/ingestor |
ok, 105.7s [T] |
sh test-all.sh |
235 passed, 0 failed (235 files) [T] |
node test-frontend-helpers.js |
712 passed, 0 failed [T] |
node test-xss-escape-sinks.js |
62 passed, 0 failed [T] |
node test-xss-392-perf-e2e.js (local Go server, port 13800, fixture prepared as in deploy.yml: freshen → Kpa-clawbot#1486 seed → corescope-migrate → seeds 2073/199/245) |
1 passed, 0 failed [T][A] |
node test-xss-333-pages-e2e.js (same server) |
11 passed, 0 failed [T][A] |
node test-xss-333-e2e.js (same server) |
8 passed, 0 failed [T][A] |
scripts/check-xss-sinks.sh --diff origin/master |
clean, exit 0 [T] |
CI run 38331317579 on this exact head, checked per job: Go Build & Test ✅, Playwright E2E ✅ (log confirms #392 perf E2E: 1 passed, 0 failed), Docker ✅, three release/deploy jobs skipped by the fork guard [T]. No #256 or #267 occurrence; no re-run needed.
Mutants
Author's six frontend mutants reproduced, each killing exactly one test and nothing else [T]:
| Mutant | Result |
|---|---|
drop escapeHtml(path) (endpoints key) |
61/1 — #392 perf endpoints table red |
drop escapeHtml(s.path) (client path) |
61/1 — #392 perf client-calls table red |
drop escapeHtml(q.path) (slow-query path) |
61/1 — #392 perf slow-queries table red |
drop escapeHtml(k) (write-source name) |
61/1 — #392 perf write-sources table red |
drop escapeHtml(writeSources.sampleAt) |
61/1 — #392 perf write-sources footer red |
drop escapeHtml(health ? health.uptimeHuman …) |
61/1 — #392 perf overview red |
E2E mutant (drop both sink escapes, server restarted on the mutated file): #392 perf E2E: 0 passed, 1 failed — perf dashboard: 2 injected <img>, 2 handler call(s), 0 dialog(s) — exactly the author's claim [T][A].
Own mutants beyond the author's:
| # | Mutant | Expected | Actual |
|---|---|---|---|
| X1 | weaken the perf.js-local fallback escaper (drop the ' → ' rule) |
red | survives — 62/0 [T] → F2 |
| X2 | replace the perf.js-local fallback escaper with the identity function | red | survives — 62/0 and test-all.sh 235/235 [T] → F2 |
| X3 | widen slowPathRawCap 128 → 4096 |
red | killed — long raw is capped red [T] |
| X4 | drop the key == raw dedupe branch |
red | killed — two subtests red [T] |
| X5 | remove the length cap (author's stated mutant, reproduced) | red | killed [T] |
| X6 | remove the control-char strip | red | killed [T] |
Two direct probes (no mutation, head code as-is) back F3 and F4 [T]:
normalizeSlowPath("/api/:id/"+5000×"z", "/api/deadbeef12/"+5000×"z") → 5139 runes; normalizeSlowPath on a CR/LF path → "/api/\r\nX-Injected: 1/:id /api/X-Injected: 1/deadbeef12".
Browser check
Own Playwright script against the local Go server, #/perf, three scenarios [A]:
- hostile strings in every string-typed field (endpoint key, slow-query path,
uptimeHuman,version,commit,sampleAt, write-source name) → 0 injected<img>, 0onerrorcalls, 0 dialogs; payloads present as inert text; 4 tables and 52 cards render. - hostile strings in the number-typed fields → 22 injected
<img>and 22 handler calls → F1. - real data, no mocking → 51 cards, 2 tables, 0 page errors, overview intact. The dashboard behaves normally.
Not verified
- Staging or production — not touched; everything ran against a local server on port 13800 with the committed fixture, and no API key was used.
- The remaining ~40 Playwright E2Es in
deploy.yml(ran only the security(ui): escape perf dashboard values (raw slow-query path) in perf.js #392 and the two security(ui): escape untrusted node, observer and route values in HTML rendering #333 suites); CI covers the rest and is green on this head. - Whether the canonical skill-side mirror of
check-xss-sinks.shcould be extended to seehtml +=accumulators — the PR's argument for not touching the gate is about that mirror, which is outside this repo. /api/perf/ioand/api/perf/sqlitewere left unmocked in scenario 1/3 and only partially covered in the isolation sweep; their values all flow throughfmtRate/Math.round/toFixed, which coerce.- Long-run or concurrency behaviour of the slow-query ring under the new path shape beyond the existing
-racesuite.
…est local escaper (#392) Round-2 review follow-ups on PR #394: - F1: escape every API-sourced interpolation in perf.js (all numeric cards across overview, Go runtime, system health, cache, packet store, SQLite, write sources), not only the three table paths. Defence in depth: these are number-typed server-side today, so not request-controlled, but the issue criterion is to escape EVERY interpolated perf value. Values passed through Math.round / unary-+ / toFixed / fmtRate, local computed vars, and the JS array .length are left as-is (a hostile string there coerces to NaN or is engine-computed, never markup). - F2: the IIFE-local escapeHtml fallback in perf.js is now driven directly from source by a unit test; an identity mutant on it flips red. - F3: normalizeSlowPath sanitises AND caps BOTH halves (route key + raw path), so the whole stored value is bounded regardless of a long template key, not just the raw half. - F4: control characters (incl. CR/LF/TAB) are stripped from both halves. - F5: documented concretely in check-xss-sinks.sh why the name-based, same-line gate cannot see perf.js's html-accumulator sinks (and why the generic locals path/q.path/k cannot be allowlisted) — the DOM-grep unit tests + browser E2E are the regression guard instead. - F6: documented the shared SlowQuery shape under GET /api/health perf.recentSlow[] and corrected the /api/perf length-cap wording. Tests: perf_slowpath_392_test.go gains two subtests (long template key is bounded; control chars stripped from the key half). test-xss-escape-sinks.js gains the F2 fallback test and five F1 numeric-card tests. All red before the fix, green after; one mutant per finding. Relates to #392
Rapport — CS-MacBook PR#394 runde-2 — head fa441cbReview feedback addressed (commit
Out of scope (per the task, F1–F6 only): F7 ( Tests, mutants (one per finding)
Local verification
CI per job (run 38338509061, head fa441cb)
Note on the first run: |
Review — CS-Macmini PR#394 — head fa441cbDom: APPROVE med nits Independent, read-only round-3 re-review of round-2 head All six round-1 findings (F1–F6) are closed or honestly documented. The security fix itself is real and I proved it independently: the same hostile-data browser sweep gives 38 injected Findings
The review pointsF5 — does the gate now catch F3 — is the whole stored value bounded? Yes. F4 — are CR and LF gone from both halves? Yes. My round-1 probe F2 — is an identity mutant of the fallback escaper red? Yes. Two of my own mutants kill the new test: replacing the fallback body with F1 — are the numeric values escaped? Yes, all of them, and the exclusions are justified. I classified every interpolation site in the 405-line file at this head [K]: the only non-escaped, non-coerced interpolation left is F6 — is Browser check of
Totals: 38 injections pre-fix → 0 at this head, 0 dialogs, 0 page errors, payloads present as inert text, 20–57 cards and 2–4 tables rendering in each scenario. Both servers ran the same binary; only Behaviour change beyond the purpose. None found. Mandatory checks. Acceptance criteria → test. Criteria 1–3 (escape every interpolated value, normalise Tests run (merged tree =
|
| Run | Result |
|---|---|
go test ./... in cmd/server |
ok, 40.1s [T] |
go test ./... in cmd/ingestor |
ok, 106.4s [T] |
go test -race -count=2 -run 'TestNormalizeSlowPath|TestPerf' |
ok [T] |
sh test-all.sh |
235 passed, 1 failed (236 files) — the one failure is test-backup-sqlite-safety.js, 9 subtests "missing tool setsid"; it fails identically on plain origin/master here (37/9) and setsid does not exist on macOS, so it is environment-local, not this branch [T] |
node test-frontend-helpers.js |
712 passed, 0 failed [T] |
node test-xss-escape-sinks.js |
68 passed, 0 failed [T] |
same suite against master's perf.js (red-before) |
56 passed, 12 failed — all 12 new cases red [T] |
node test-xss-392-perf-e2e.js (local Go server :13800, fixture prepared as in deploy.yml: freshen → Kpa-clawbot#1486 seed → corescope-migrate → seeds 2073/199/245) |
1 passed, 0 failed [T][A] |
node test-xss-333-pages-e2e.js (same server) |
11 passed, 0 failed [T][A] |
node test-xss-333-e2e.js (same server) |
8 passed, 0 failed [T][A] |
scripts/check-xss-sinks.sh --diff origin/master (real clone, sanity-checked live) |
clean, exit 0 [T] |
check-xss-sinks.sh --file × 80 public/** files, master |
2 files with findings, perf.js not among them [T] |
| own browser sweep, 6 scenarios × (pre-fix vs head) | 38 injections → 0 [A] |
CI run 38338509061 on this exact head, checked per job: Go Build & Test ✅ (15m11s), Playwright E2E ✅ (24m20s, log line #392 perf E2E: 1 passed, 0 failed), Docker ✅, Release/Deploy/Badges skipped by the fork guard [T]. run_attempt = 2; attempt 1's single failure is N9. No #256 or #267 occurrence.
Mutants
Author's round-2 mutants reproduced, plus my own. Nine run in total; six killed, three survive as the test gaps above.
| # | Mutant | Expected | Actual |
|---|---|---|---|
| M1 | sanitizeSlowPathComponent: drop || r == 0x7f |
red | survives — whole cmd/server suite green [T] → N4 |
| M2 | cap off-by-one: n >= slowPathRawCap → n > |
red | killed — 2 subtests red (130 runes, 261 runes) [T] |
| M3 | normalizeSlowPath: emit key unsanitised (pre-fix) |
red | killed — 5139 runes (max 259) + control chars survived in the key half [T] |
| M4 | perf.js fallback body → return s; |
red | killed — #394 F2: fallback did not escape <, 67/1 [T] |
| M5 | drop escapeHtml(m.rss) (health Node branch) |
red | survives — 68/0 and 712/0 green [T] → N5 |
| M6 | drop escapeHtml(err.message) |
red | survives — 68/0 green [T] → N6 |
| M7 | fallback selector condition → (false) |
red | killed — selector did not prefer globalThis.escapeHtml, 67/1 [T] |
| M8 | drop escapeHtml(health ? health.uptimeHuman …) |
red | killed — #392 perf overview red, 67/1 [T] |
| M9 | drop escapeHtml(k) (write-source name) |
red | killed — #392 perf write-sources table red, 67/1 [T] |
Direct probes with no mutation, at this head [T]: total stored value 259 runes for a 5000-char key and raw (round 1: 5139); CR/LF/NUL/ESC/DEL stripped from both halves; HTML payload passes through un-escaped as intended ("/api/:id /api/<img src=x onerror=alert(1)>").
Not verified
- Staging and production — not touched. Everything ran against local servers on ports 13800/13801 with the committed fixture; no API key was used, no hostile data was persisted (the E2E and my probes mock in-browser only).
- The remaining ~40 Playwright E2Es in
deploy.yml— I ran the#392suite and the two#333suites; CI covers the rest and is green on this head. test-backup-sqlite-safety.js's 9setsidsubtests — cannot run on macOS (fails identically on plain master).- Whether the canonical skill-side mirror of
check-xss-sinks.shand its shared allowlist could absorbpath/q.path/kwithout harm in other repos — my N3 false-positive count is for this repo'spublic/tree only. That mirror is outside this repo. - A live two-half
SlowQuery.Path: I induced 20 real slow queries but never pushed a parameterised route past 100 ms locally, so every live row showed the deduped single-half form. The two-half shape is pinned by unit tests and follows from the single call site. - Whether a hostile endpoint key or slow-query path is reachable end-to-end on a real deployment (round 1 left this unsettled; unchanged — the frontend escaping makes it moot).
- Long-run or concurrency behaviour of the slow-query ring under the new path shape beyond the
-raceruns above.
Relates to #392
Follow-up to #333, carved out of PR #373.
public/perf.jsinterpolated/api/perfvalues intoinnerHTMLwithout escaping. The request-controlled sinks are the server-endpoints table key (unmatched/api/routes collapse to the raw path, so a slow 404 under/api/can carry markup) and the slow-query path (rawr.URL.Path,cmd/server/routes.go).What changed
Frontend —
public/perf.jsescapeHtmlthat prefersapp.js's canonical global and falls back to an identical 5-char OWASP escaper, soperf.jsis safe both in the browser (uses the global) and when evaluated standalone (the perf-render unit-test sandboxes). The RHS readsglobalThis.escapeHtml, never the bare name, to avoid a TDZ self-reference.sampleAt, the server uptime string, and the error fallback. Numeric metrics typednumberin the API spec are left as-is — they coerce and cannot carry markup.Server —
cmd/server/routes.go(read-only, no newmap[string]interface{})SlowQuery.Pathvia a purenormalizeSlowPathhelper: pair the:id-folded route key with a control-char-stripped, rune-safe, length-capped (≤128 runes) copy of the raw path, matching the endpoint key's treatment and bounding the in-memory ring. It does not HTML-escape — the dashboard does that at render time. Documented indocs/api-spec.md.Requirements → tests
test-xss-escape-sinks.js#392 perf endpoints table[T]escapeHtml(path)→ raw<img>survivestest-xss-escape-sinks.js#392 perf client-calls table[T]escapeHtml(s.path)test-xss-escape-sinks.js#392 perf slow-queries table[T]escapeHtml(q.path)sampleAt, uptime)test-xss-escape-sinks.js3 further#392cases [T]escapeHtml(...)#/perf, hostile key + slow-query path, mocked/api/perfon a LOCAL servertest-xss-392-perf-e2e.js[T]<img>, 2onerrorcallsSlowQuery.Pathnormalisationcmd/server/perf_slowpath_392_test.go[T]All six unit tests drive the real extracted
perf.jsrow templates vianew Function(vm-style); the captured template matches pre- and post-fix, so the assertion — not the regex — flips. The E2E mocks/api/perfentirely in-browser withpage.route(fixture DB untouched, nothing persisted, nothing leaves the machine) and judges "did any handler fire / any injected<img>/ any dialog", not a string match. Wired intodeploy.yml's Playwright step.check-xss-sinks.sh--diff origin/masteris clean. The gate cannot flagperf.js's pre-fix pattern (verified: exits 0 on both the pre-fix and post-fix file):perf.jsbuilds HTML via anhtml += \…`accumulator and does a singleel.innerHTML = html, while the per-line regex only treats.innerHTML/.outerHTML/.srcdocassignment/append lines as sinks — the node-controlled${…}interpolations live onhtml +=lines it never audits (andpath/q.path` are not in the node-controlled allowlist anyway). Modifying the gate's detection would diverge from its canonical skill-side mirror, so the unit + E2E tests (with mutants) are the regression guard instead.Verification [T]=test [A]=API/served [K]=code/arch
node test-xss-escape-sinks.js→ 62 passed, 0 failed [T]node test-frontend-helpers.js→ 712 passed, 0 failed [T]sh test-all.sh→ 235/235 files passed [T]test-xss-392-perf-e2e.jsagainst a local Go server (port 13900, mocked/api/perf) → 1 passed; mutant (drop escaping) → 1 failed as expected [T][A]go test .incmd/server→ ok (43s);go test -raceon perf/slow-path tests → ok [T]scripts/check-xss-sinks.sh --diff origin/master→ clean [K]deploy.yml, 1 inrelease-fast-path.yml[K]