fix(ui): escape and validate miniMarkdown link destinations (#368)

[dborup]

Relates to #368

Problem

miniMarkdown() in public/roles.js built its anchors from a '$2' replacement template:

.replace(/\[([^\]]+)\]\(([^)]+)\)/g,
         '<a href="$2" target="_blank" rel="noopener" style="color:var(--link-color)">$1</a>')

The surrounding text is & < > escaped on the first pass, but the captured destination lands in a double-quoted href with neither attribute escaping nor any restriction on the URL scheme. Two consequences:

1. The two quote characters are never escaped, so a destination can close the attribute and add its own — including an event handler.
2. Nothing restricts the scheme, so javascript:, data:text/html, vbscript: all become live links.

public/home.js renders Home step descriptions and FAQ answers through miniMarkdown() and assigns the result with innerHTML. That text comes from SITE_CONFIG.home (served by /api/config/theme, importable site customization), so the destination is attacker-reachable. Verified on this branch: with the fix reverted, the browser E2E below reports injected handler fired 1 time(s) on the real Home page.

Source-code finding only. No live-site change is authorized by #368 and none was made.

Plan / what changed

public/roles.js

• Attribute escape. The destination now goes through escapeAttr before it reaches the href. Rather than add a third copy of that helper, it is defined once in roles.js (loaded before home.js by index.html) and exported as window.escapeAttr; home.js's local copy now delegates to it, so the Markdown path and the Home attribute sinks cannot drift apart. The implementation is byte-for-byte the behaviour of the copy it replaces (" → ", ' → '); &/</> are already entities by the time a destination is captured.
• Scheme allowlist. safeLinkHref() lowercases the destination and strips every byte in the range U+0000 to U+0020 plus U+007F (ASCII controls, space and DEL) — the characters a browser ignores while resolving a scheme — then matches the RFC 3986 scheme grammar ^[a-z][a-z0-9+.-]*:. A match outside {http, https} is rejected. A destination with no scheme (relative path, #fragment, ?query) is untouched and still renders.
• Rejected links degrade to text. A rejected destination renders as its already-escaped label, so no markup and no URL leaks into the page.
• Replacement function, not a template. The anchor is now built in a function(_m, label, dest) callback, which also closes a latent second hole: a $& or $' in the destination was being re-expanded by String.replace.

Everything else in miniMarkdown (bold, italic, code, lists, <br>, the <ul> wrap) is unchanged and pinned by tests.

scripts/collect-frontend-coverage.js exercises both arms of the new branch so the instrumented coverage run does not report it as dead code.

Acceptance → tests → mutants

Unit: test-issue-368-minimarkdown-links.js (53 assertions, registered in test-all.sh) drives the real miniMarkdown from public/roles.js in a vm sandbox and parses the rendered anchor's attribute names, so the breakout checks are payload-independent.

Browser: test-xss-368-home-links-e2e.js (9 checks, registered in the Playwright step of deploy.yml) drives the real Home page against a local Go server with e2e-fixture.db, injecting the hostile customization by rewriting /api/config/theme with page.route. The fixture DB is untouched and no payload leaves the machine. Checks are DOM-level: did a handler run, what protocol did the browser resolve for each anchor, which attributes does each anchor actually carry.

#	Acceptance point	Test	Red before	Green after
A1	Hostile destinations cannot create attributes in the Home rendering path	unit A1 (8) + E2E "no checklist anchor grew an attribute…", "…no inline event handler", "quote-breakout destinations stay inert"	yes	yes
A2	Unsafe schemes rejected after case + ASCII control/whitespace normalisation	unit A2 (21) + E2E "no injected handler ran", "no checklist anchor resolves to an unsafe scheme", "every unsafe-scheme destination degraded to plain label text"	yes	yes
A3	Safe links and ordinary checklist text still render and navigate	unit A3 (11) + E2E "every safe destination still renders…", "a safe hash link still navigates"	n/a (guard)	yes
A4	Markdown behaviour outside link destinations preserved	unit A4 (11)	n/a (guard)	yes
A5	One shared escapeAttr on the Home path	unit A5 (2)	yes	yes

Red-before was measured by restoring public/roles.js and public/home.js from origin/master (4a5ec5e2) with the new tests in place: unit 24 passed / 29 failed, E2E 3 passed / 6 failed (including the live injected handler fired 1 time(s)). With the fix: unit 53/0, E2E 9/0.

Mutants — each applied to the fixed public/roles.js, both suites re-run, then reverted from a scratch snapshot:

Mutant	Unit	E2E
M1 return escapeAttr(raw) → return raw	47 / 6 failed	5 / 4 failed
M2 drop the control+whitespace strip (probe = raw.toLowerCase())	41 / 12 failed	7 / 2 failed
M3 accept every scheme (if (false) return '')	34 / 19 failed	7 / 2 failed
M4 restore the '$2' replacement template	26 / 27 failed	3 / 6 failed
baseline restored	53 / 0	9 / 0

Verification run locally

• sh test-all.sh — 239 files, 239 passed, 0 failed (includes the root test-registry gate: both new files are registered in exactly one entry point, and the E2E is correctly in deploy.yml rather than test-all.sh).
• node test-frontend-helpers.js — 713 passed, 0 failed.
• node test-preflight-xss-gate.js — all assertions passed.
• Go: every module green — cmd/server, cmd/ingestor, cmd/migrate, cmd/decrypt and all internal/* (go test ./... per module; this repo is multi-module, there is no root go.mod).
• npx eslint@8 public/*.js — 0 errors (89 pre-existing warnings, none in the changed hunks).
• bash scripts/check-xss-sinks.sh --diff origin/master — clean; also clean per-file on public/roles.js and public/home.js.
• E2E run against a local corescope-server on a private port with a migrated copy of test-fixtures/e2e-fixture.db, prepared as in CI. No staging or production access.

Constraints held: cmd/server untouched, no new map[string]interface{}, no hardcoded colours (the anchor keeps var(--link-color)), fork guards unchanged at 9 in deploy.yml and 1 in release-fast-path.yml.

Left for the owner

• mailto: is not on the allowlist. Nothing in the repo emits a mailto: link today, so adding it would widen behaviour rather than preserve it. If site owners are expected to put contact addresses in Home customization, mailto should be added to SAFE_LINK_SCHEMES — a product call, not a security one. A unit test currently pins the exclusion, so it would need updating with it.
• Protocol-relative destinations (//host/path) are still accepted. They carry no scheme, so they fall in the "relative" bucket and render as an ordinary cross-origin link with target="_blank" rel="noopener". Blocking them would be a policy choice about cross-origin links in customization content, beyond the scope of this issue.
• public/path-inspector.js keeps its own escapeAttr with different semantics (&, ", < — no '). It is not on the Home path and consolidating the two variants is a separate cleanup, not folded in here.
• A rejected destination is dropped silently — the reader sees the label as plain text with no indication the link was removed. If site owners should be warned, that belongs in the customizer's validation UI rather than in the renderer.

[dborup]

Rapport — CS-Minimax PR#420 #368 — head 7002497

Status: Done — every acceptance point implemented, each with a test red before and green after plus mutants; every CI job on this head is green and nothing is left unfinished.

Evidence tags: [T] = I ran it. [A] = from reading the code. [K] = from CI.

Rechecked against current master 4a5ec5e2 (merge of #402), not the 1f423e06 the issue was written on. #373 / #394 / #402 touched other sinks (analytics.js, compare.js, perf.js) and left miniMarkdown() untouched, so the finding still reproduces verbatim; #385 and #358 do not touch this path. cmd/server has no change in this diff. [A]

Requirements

#	Requirement	Change	Test	Mutant
A1	Hostile destination cannot create attributes in the real Home rendering path	destination goes through escapeAttr before the href; the anchor is built in a replace function, so $& / $' in the destination is no longer re-expanded	test-issue-368-minimarkdown-links.js A1 (8 cases, asserts on the parsed attribute-NAME list, so it is payload-independent) + test-xss-368-home-links-e2e.js "no checklist anchor grew an attribute…", "…no inline event handler", "quote-breakout destinations stay inert" [T]	M1 return escapeAttr(raw) → return raw: unit 6 red, E2E 4 red. M4 restore the '$2' template: unit 27 red, E2E 6 red [T]
A2	Reject unsafe schemes after normalising case and stripping ASCII controls/whitespace	safeLinkHref() strips every byte in the range U+0000 to U+0020 plus U+007F (ASCII controls, space and DEL), lowercases, then matches the RFC 3986 scheme grammar ^[a-z][a-z0-9+.-]*: and requires http / https. Schemeless destinations are untouched	A2 (21 cases: mixed/upper case, leading spaces, embedded TAB / LF / CR / NUL / VT / FF / DEL, space before the colon, data:, vbscript:, file:, whitespace-only) + E2E "no injected handler ran", "no checklist anchor resolves to an unsafe scheme" (checks the live HTMLAnchorElement.protocol, not a string match), "every unsafe-scheme destination degraded to plain label text" [T]	M2 drop the control/whitespace strip: unit 12 red, E2E 2 red. M3 accept every scheme (if (false) return ''): unit 19 red, E2E 2 red [T]
A3	Rejected link renders as escaped label text; safe links and ordinary text unchanged	a rejected destination returns the already-escaped label	A3 (11 cases: https, http, mixed-case scheme, /nodes, #/map, relative, dot-relative, query-only, colon after the first path segment, & escaped exactly once, target/rel/style intact) + A2 "rejected destination inside a sentence keeps the rest of the text" and "…label is still HTML-escaped" + E2E "every safe destination still renders as an anchor with the given href" and "a safe hash link still navigates" [T]	M3 also flips the "a rejected destination inside a sentence…" case red; M4 flips "two links in one line are handled independently" red [T]
A4	Preserve existing Markdown behaviour outside link destinations	no change to bold / italic / code / list / <br> / the <ul> wrap	A4 (11 cases, incl. a link inside a list item and two links on one line) [T]	M4: unit 27 red, incl. A4 [T]
A5	Reuse the existing helper instead of adding a parallel one	escapeAttr is now defined once in roles.js (loaded before home.js by index.html) and exported as window.escapeAttr; home.js's local copy delegates to it. Behaviour byte-for-byte identical to the copy it replaces	A5 (2 cases: the export behaves as before, and home.js carries no second implementation) [T]	reverting home.js to its own body: A5 red [T]
A6	Register the tests in the active entry points	unit test in test-all.sh, browser test in the Playwright step of deploy.yml	the repo's own root test-registry gate ("every root test-*.js runs in test-all.sh or deploy.yml", "no file is registered twice", "no registered file needs a browser or a server") passes on this branch [T][K]	n/a (the registry gate is itself the check)

Red-before was measured by restoring public/roles.js and public/home.js from origin/master with the new tests in place: unit 24 passed / 29 failed, browser 3 passed / 6 failed — the browser run reports injected handler fired 1 time(s), i.e. a live handler execution on the real Home page, not just an unescaped string. With the fix: unit 53 / 0, browser 9 / 0. [T]

Mutants were applied to the fixed public/roles.js, both suites re-run, then reverted from a scratch snapshot (never with git checkout --, so the fix under test could not be discarded). Baseline after all four: unit 53/0, browser 9/0. [T]

CI per job (head 7002497b, run 38380769976)

Job	Result
Go Build & Test	pass (24m06s) — test-all.sh 239 passed / 0 failed over 239 files, incl. test-issue-368-minimarkdown-links.js at 53/0; preflight XSS gate step green [K]
Playwright E2E Tests	pass (27m24s) — test-xss-368-home-links-e2e.js ran and reported 9 passed / 0 failed [K]
Build & Publish Docker Image	pass [K]
Release Artifacts	skipping (not a release ref) [K]
Publish Badges & Summary	skipping [K]
Deploy Staging	skipping [K]

No job needed a re-run; the known flake #271 did not appear. [K]

Local verification

• sh test-all.sh — 239 / 239 [T]
• node test-frontend-helpers.js — 713 / 0 [T]
• node test-preflight-xss-gate.js — all assertions passed [T]
• Go: green in every module — cmd/server, cmd/ingestor, cmd/migrate, cmd/decrypt and all internal/*. This repo is multi-module, so go test ./... was run per module rather than once at the root [T]
• npx eslint@8 public/*.js — 0 errors (89 pre-existing warnings, none in the changed hunks) [T]
• bash scripts/check-xss-sinks.sh --diff origin/master — clean; also clean per-file on public/roles.js and public/home.js [T]
• The browser test ran against a locally built corescope-server on a private port, against a migrated copy of test-fixtures/e2e-fixture.db prepared as in CI; the server was stopped afterwards. No staging or production access, no server API key used [T]

Constraints held: cmd/server untouched, no new map[string]interface{}, no hardcoded colours (the anchor keeps var(--link-color)), fork guards unchanged at 9 in deploy.yml and 1 in release-fast-path.yml. [T]

Remainders — left for the owner

1. mailto: is not on the allowlist. Nothing in the repo emits one today, so adding it would widen behaviour rather than preserve it. If site owners are expected to put contact addresses in Home customization, mailto belongs in SAFE_LINK_SCHEMES; a unit test currently pins the exclusion and would need updating with it. Product call, not a security one. [A]
2. Protocol-relative destinations (//host/path) are still accepted. They carry no scheme, so they land in the "relative" bucket and render as an ordinary cross-origin link with target="_blank" rel="noopener". Blocking them is a policy choice about cross-origin links in customization content, beyond this issue. [A]
3. public/path-inspector.js keeps its own escapeAttr with different semantics (&, ", < — no '). It is not on the Home path, so consolidating the two variants is a separate cleanup and was not folded in here. [A]
4. A rejected destination is dropped silently — the reader sees plain label text with no sign a link was removed. Warning the author belongs in the customizer's validation UI, not in the renderer. [A]
5. security(ui): escape untrusted node, observer and route values in HTML rendering #333 scope is untouched. The other HTML sinks fed by node / observer / route data remain that issue's business. [A]

This is a source-code finding, not a claim of an observed production exploit, and no live-site change was made. The PR is left as a draft; no merge, no ready-for-review, no issue closed.

[dborup-agent]

Review — CS-pve-agent1 PR#420 — head 7002497

Dom: APPROVE med nits

Independent, read-only review. Evidence tags: [T] = I ran it, [A] = from reading the code, [K] = from CI.

Trees used: the head (git archive 7002497b) and the merged tree git merge-tree --write-tree origin/master 7002497b = da9ef62f on current origin/master 20513d48 (contains c9a0ad76). The merge is clean. public/roles.js, public/home.js and scripts/collect-frontend-coverage.js are byte-identical between head and merged tree. test-all.sh and deploy.yml differ only by the changes master picked up since the PR's merge-base 4a5ec5e2. git ls-remote showed head 7002497b before and after the review. [T]

There is no earlier review on this PR and no comments on #368, so each point below is checked against the issue text.

Findings

#	Severity	Finding	Evidence
1	nit (docs)	The code comment in miniMarkdown and the PR body say the '$2' template "re-expanded" a $& / $' found in the destination, and call that a "latent second hole". It did not: String.prototype.replace expands $ patterns in the replacement template only, never inside the substituted capture. '[a](/a$&b)'.replace(re, '<a href="$2">$1</a>') gives href="/a$&b". The test a $' in the destination is not re-expanded is red before the fix only because of the ' → &#39; escape. a $& … is not re-expanded is green on master. The function form is still fine. Only the rationale in the comment and the PR body is wrong.	[T] node one-liner on the old template; red-before run
2	nit (docs)	The comment above the new escapeAttr in roles.js says it is the "same … set as the escapeAttr helpers in public/home.js and public/path-inspector.js". path-inspector.js escapes &, ", < and not ', so the sets differ. The PR body itself says so under "Left for the owner".	[A] public/path-inspector.js:184
3	nit	The claim that the shared escapeAttr is "byte-for-byte identical" to home.js's old copy is not quite right for null/undefined. The old copy returns "null"/"undefined" and the new one returns "". For example, a footerLinks entry with no url used to get href="undefined" and now gets href="". This is harmless and arguably better, but it is a change.	[T] old vs new helper on null/undefined
4	follow-up, out of scope, not blocking	The same Home customization source has another href sink with no scheme check: footerLinks[].url at public/home.js:152 (href="${escapeAttr(l.url)}"). A footer link with javascript: still renders as a live javascript: anchor on master and on this branch. It is not in #368, which is about miniMarkdown, and #333 covers only node, observer and route data. safeLinkHref could be reused there. For the owner to decide. I have not opened an issue.	[T] browser probe on the local server; a.protocol === "javascript:" for the footer anchor on both trees
5	info	The normalisation is stricter than a browser's. It strips every C0 control, space and DEL anywhere in the destination, while a browser strips only TAB/LF/CR inside and C0/space at the ends. That errs on the safe side. Two cases show it: java\0script: and javascript : are rejected even though a browser would resolve them as relative URLs. The comment "mirrors what a browser does" is therefore slightly imprecise. My mutant R4 (browser-exact stripping) shows the tests pin the stricter behaviour (5 red).	[A] + [T] mutant R4
6	info	The "Left for the owner" reasoning on mailto: is backwards. On master, a mailto: Markdown link rendered as a link, so leaving it off the allowlist narrows behaviour; it does not "widen" it. The decision itself is defensible and is disclosed in the PR body, a code comment and a pinning test. Nothing in the repo emits mailto:/tel:, and the shipped default Home links are all https:// and keep working.	[A] grep of defaults in cmd/server/routes.go and public/customize*.js

The issue, point by point

Issue item	Status	Test (red before → green after)
Escape the destination for attribute context	Done: escapeAttr(raw) inside the href	Unit A1, 4 breakout cases plus "/' markers. Red on master's roles.js/home.js, green on the fix [T]
Reject unsafe schemes after normalising case and ASCII controls/whitespace; keep HTTPS, relative and fragment links	Done: strict http/https allowlist on an RFC 3986 scheme match; destinations without a scheme pass	Unit A2, 19 rejected destinations, all red before [T]. Unit A3 (9 accepted plus & and hardening) is a guard, green before and after, which fits a preservation criterion [T]
A rejected link renders as escaped label text; other Markdown unchanged	Done	"rejected destination inside a sentence" red → green [T]. "label is still HTML-escaped" and A4 (11) are guards, green on both [T]
Focused tests, registered in the active entry points	Done: unit test in test-all.sh, E2E in the Playwright step of deploy.yml. Both are still present in the merged tree, and the registry gate passes	test-all.sh 241/241 on the merged tree [T]
Acceptance: hostile destinations cannot create attributes or executable links in the real Home path	Met	E2E with master's roles.js/home.js: 3 passed / 6 failed, including injected handler fired 1 time(s). On the merged tree: 9/0 [T]
Acceptance: safe links and ordinary text still render and navigate	Met	E2E "every safe destination…" and "a safe hash link still navigates" green [T]. Home tests in test-e2e-playwright.js green [T]
Acceptance: frontend unit, browser regression and XSS preflight pass	Met	see Tests [T][K]

"Left for the owner" (mailto:, protocol-relative //host, the path-inspector.js variant, silent drop, #333) is honest and has reasons. Nothing that needs a product decision or new config was implemented without being disclosed. On scope creep: the new window.escapeAttr global and the coverage-script lines are small and on-topic for the DRY requirement in AGENTS.md. No other file defines a top-level escapeAttr that could shadow the global, and path-inspector.js's copy sits inside an IIFE [A].

Security review (#368)

• Escaping. The destination is HTML-escaped (& < >) in the first pass, then escapeAttr adds "/'. Nothing can leave the double-quoted href. Entities in the source come out literal because & becomes &. In the browser, javascript:, javascript:, javascript: and jav	ascript: all resolve as relative http: URLs [T].

• Allowlist after normalisation. Rejected and rendered as plain label text [T]:

  • mixed and upper case;
  • leading spaces, TAB/LF/CR/NUL/VT/FF/DEL;
  • jav ascript:, JaVa\tScRiPt:, JAVA\0SCRIPT:, " \t\njavascript:";
  • data:, vbscript:.

  Some destinations carry their scheme behind something the scheme regex does not match: NBSP, BOM or U+2028 in front, or java*s*cript:, java\s`cript:andjavascript:, where the earlier Markdown pass puts , orinside the destination. These stay links, but the browser resolves every one of them as a relativehttp:` URL. Not exploitable [T].

• Browser check on Home against a local Go server with the hostile customization injected via page.route. 25 extra destinations, plus FAQ and footer links. Every checklist anchor was hovered and focused. Result on the merged tree: 0 non-http(s) checklist anchors, every anchor carries exactly href,target,rel,style, no handler ran, no dialog. On master the same probe gives 4 non-http(s) anchors, and the FAQ javascript: link is live [T]. The footer sink is the out-of-scope finding 4.

• Safe links. https://, HTTPS://…?b=1&c=2 (with & escaped once), /nodes, #/map and docs/x render with the given href. #/map navigates [T]. Protocol-relative //evil.example and \\evil.example still resolve cross-origin, the same as on master and as disclosed [T].

Tests and mutants

On the merged tree da9ef62f, unless stated otherwise:

Run	Result
cmd/server go test ./...	ok [T]
cmd/ingestor go test ./...	ok [T]
sh test-all.sh	241 passed / 0 failed (241 files) [T]
node test-frontend-helpers.js	713 / 0 [T]
node test-issue-368-minimarkdown-links.js	53 / 0. Red-before with master's roles.js/home.js: 24 / 29 [T]
test-xss-368-home-links-e2e.js	9 / 0. Red-before: 3 / 6 [T]
test-home-coverage-e2e.js	12 / 0 [T]
test-issue-304-my-mesh-e2e.js	pass [T]
test-xss-333-pages-e2e.js	11 / 0 [T]
test-e2e-playwright.js	Home tests green ("Home page loads", "Customizer open does not overwrite server home config", "Home customization persists through page refresh"). The fail-fast run stops at "Version info lives on Perf dashboard" (navStats wait timeout). It stops at the same test against a master-built server on the same prepared DB, so it is local-environment and unrelated. CI ran the full suite green [T][K]
scripts/check-xss-sinks.sh --diff origin/master on head	clean, exit 0 [T]. CI step green [K]

E2E server: corescope-server built from the merged tree. It ran on a private port against a copy of test-fixtures/e2e-fixture.db, prepared as in CI: freshen, the #1486/#1791 seed SQL, corescope-migrate, then seeds 2073, 199 and 245. Servers were stopped by the pid found via their port. [T]

Own mutants on public/roles.js. Each was applied to a scratch copy and the copy was restored and cmp-verified after every run. [T]

Mutant	Unit (53)	E2E (9)
R1 drop the escape: return escapeAttr(raw) → return raw	6 red	4 red, incl. injected handler fired 1 time(s)
R2 allowlist → denylist (m[1] === 'javascript')	4 red (data:, vbscript:, file:, mailto:)	2 red (data:, vbscript: anchors)
R9 remove the scheme check entirely	—	2 red
R3 drop .toLowerCase()	2 red	—
R4 browser-exact stripping (TAB/LF/CR plus leading C0 only)	5 red (pins the stricter normalisation, see finding 5)	—
R5 unanchored scheme regex	1 red (/nodes/a:b)	—
R6 fallback returns the raw Markdown instead of the label	12 red	—
R7 drop the ' escape	3 red	—
R8 escape the normalised probe instead of raw	2 red	—

All of them were killed. One weakness in the E2E: under R9, "no injected handler ran" stays green because the test never activates a javascript: anchor. The live a.protocol assertion catches it. That is adequate.

Constraints

• cmd/server untouched (0 files under cmd/). No added map[string]interface{} [T]

• No hardcoded colours: the anchor keeps var(--link-color) [T]

• Fork guards: grep -c 'Kpa-clawbot/CoreScope' gives 9 in deploy.yml and 1 in release-fast-path.yml, the same as on master [T]

• No closing keywords in the PR body ("Relates to #368") or the commit message [T]

• Commit author and committer: dborup <kontakt@meshview.dk> [T]

• CI on head, run 38380769976:

  • Go Build & Test: pass, including test-issue-368 53/0 and the preflight XSS gate;
  • Playwright E2E: pass, with the #368 E2E at 9/0;
  • Docker: pass;
  • Release, Badges, Deploy: skipped (not applicable).

  No rerun was needed, and neither #256 nor #267 appeared. CI ran on the merge with 4a5ec5e2, not on current master; that gap is covered by my merged-tree runs [K][T]

Not verified

• Browsers other than Chromium. Firefox and Safari URL parsing were not exercised.
• javascript: anchors were not actually activated. Unsafe links are detected through HTMLAnchorElement.protocol.
• The full test-e2e-playwright.js past its local environment stop (CI covers it), and the other E2E files outside the Home path.
• The instrumented coverage run (scripts/collect-frontend-coverage.js).
• The external reference PR named in the issue was not consulted.