RS-22478: Sanitize as_html while keeping styles tag

[chschan]

Second attempt to sanitise as_html label content with DOMPurify. This time we separate out the <style> tags and sanitize them separately so they are retained.

Why styles are extracted and scrubbed separately

DOMPurify has no CSS-content sanitizer for <style> elements — verified in real Chrome (DOMPurify 3.4.10): with the default config it drops the whole <style> block (this is what broke #51), and the only way to keep it (FORCE_BODY/WHOLE_DOCUMENT) passes the CSS through completely unsanitized (@import, expression(), -moz-binding, behavior, url() all survive). So we pull <style> out first, scrub their CSS ourselves — best-effort, defence-in-depth for the lower-severity CSS surface (@import/external loads/legacy expression()/behavior/-moz-binding, now also robust to CSS backslash escapes such as @\69 mport) — run everything else through DOMPurify (which removes scripts, event-handler attributes, javascript: etc.), then re-insert the scrubbed styles. Residual: plain url() resource-loads are not stripped (low severity, no script execution).

Why <iframe> is allowed

Metro supports embedded video — YouTube (<iframe src=".../embed/...">) and <video controls> (see the VIS-1010/RS-12497 test). DOMPurify still blocks src="javascript:". sandbox is intentionally not applied because YouTube embeds require allow-scripts+allow-same-origin, which together negate most of its protection.

Anchor targets

Anchors with a target get rel="noopener noreferrer" via an afterSanitizeAttributes hook to prevent reverse tabnabbing.

[JustinCCYap]

The hand-rolled CSS scrub is weaker than DOMPurify's own CSS sanitizer and is bypassable via CSS character escapes — e.g. @\69 mport parses as @import in browsers but won't match /@import\b/. The comment correctly notes this is best-effort and that modern browsers no longer execute CSS-borne script, so severity is low. Worth considering the alternative: keep style in DOMPurify's ALLOWED_TAGS and let its built-in CSS parser handle @import/url()/expression() properly instead of pulling styles out. If that path was rejected because DOMPurify dropped the styles in the earlier attempt, a sentence in the PR explaining why would help.

[chschan]

Hardened stripDangerousCss to decode CSS backslash escapes (hex + single-char) before the scrub, so @\69 mport and similar forms canonicalise to @import and get stripped. Added regression tests asserting the external URL is removed for both the plain and the CSS-escaped @import.

On letting DOMPurify's own CSS sanitizer handle this instead of pulling <style> out: this redo extracts styles because the first attempt (#51) dropped the table/widget CSS that flipFormat emits. I haven't been able to confirm DOMPurify's in-browser CSS sanitizer preserves that CSS (under jsdom it drops <style> entirely, though that's not conclusive for a real browser), so I'd rather not flip the approach in this PR. Keeping the extraction here and will file a follow-up to evaluate switching to DOMPurify's CSS sanitizer. Added a sentence to the PR description explaining why styles are separated.

[chschan]

Followed up on the "let DOMPurify handle the CSS" alternative — tested DOMPurify 3.4.10 in real Chrome (puppeteer), not just jsdom, against the actual flipFormat cmd.css. It turns out DOMPurify has no CSS-content sanitizer for <style> elements:

• With the default config (and even ADD_TAGS:["style"]) it drops the whole <style> block — confirmed in real Chrome, not a jsdom quirk. This is why the first attempt (RS-22478: fix stored XSS in metro HTML labels #51) lost the widget styling.
• <style> survives only with FORCE_BODY:true / WHOLE_DOCUMENT:true (an HTML-parser placement thing; style is already in the default allow-list).
• But when it is kept, the CSS passes through completely unsanitized — @import, the escaped @\69 mport, expression(), -moz-binding, behavior, url(), even url("javascript:...") all survive verbatim.

So DOMPurify only ever drops the block or keeps it raw — there is no in-between "sanitize the CSS" path for <style> (it sanitises inline style= attributes only). That makes the extract-scrub-reinsert approach necessary, and the hand-rolled scrub is the only thing sanitising the <style> CSS (now also robust to the escaped forms). Residual is plain url() resource-loads, which neither approach strips — low severity (no script execution). Updated the PR description to capture this.

[JustinCCYap]

Thanks for the thorough updates — all six points from the previous round are addressed (tests, escape-aware scrub, iframe rationale, rel=noopener hook, nullish guard, style-order test). However, the escape-decoding added to close the @import bypass has introduced a higher-severity hole. Two changes needed before merge — see inline.

[JustinCCYap]

🔴 Blocker: this reintroduces <style> breakout XSS. The decoded CSS from stripDangerousCss is re-inserted verbatim inside <style>…</style> here, and this styles string is prepended outside the DOMPurify call (line 61), so it is never sanitized by DOMPurify. <style> is an HTML rawtext element, so a literal </style> in its content terminates it — and now that decodeCssEscapes runs first, a CSS-escaped </style> decodes into a real one.

PoC (verified by running the actual decodeCssEscapes/stripDangerousCss/STYLE_BLOCK logic from this diff):

input:  <style>x\3C/style\3E\3Cimg src=x onerror=alert(1)\3E</style>
output: <style>x</style><img src=x onerror=alert(1)></style>

The escaped \3C/style\3E slips past the non-greedy STYLE_BLOCK regex (not a literal </style> yet), then decoding turns it into a real </style> plus a live <img onerror> that executes. This is the exact XSS class the PR exists to prevent — a regression created by the escape-decoding fix.

Fix: never re-insert decoded text into the rawtext <style> context unescaped. Cleanest is to re-escape </> back to CSS escapes after scrubbing (renders identically in CSS, cannot break out):

function stripDangerousCss (css) {
  return decodeCssEscapes(String(css))
    .replace(/@import\b[^;}]*;?/gi, '')
    .replace(/expression\s*\(/gi, '')
    .replace(/-moz-binding\b[^;}]*;?/gi, '')
    .replace(/\bbehaviou?r\s*:[^;}]*;?/gi, '')
    .replace(/javascript\s*:/gi, '')
    .replace(/</g, '\\3C ')   // neutralise </style> breakout
    .replace(/>/g, '\\3E ')
}

Please add a regression test asserting \3C/style\3E… does not yield a literal </style><…> in the output.

[chschan]

Confirmed and fixed — good catch, this was a real regression from the escape-decoding change. Reproduced your PoC: <style>x\3C/style\3E\3Cimg src=x onerror=alert(1)\3E</style> produced a live <img onerror> outside the <style>.

Fix: after the scrub, re-escape < back to the CSS escape \3C (renders identically) so the re-inserted CSS can't break out of the rawtext <style>. I escape only <, not > — > can't terminate a <style> element, and escaping it would break legitimate > child combinators in as_html CSS.

Verified the fix in real Chrome (puppeteer): the sanitized output is <style>x\3C /style>\3C img ...></style>, and inserting it via .html() yields 0 <img>, 0 <script>, 1 intact <style>, and the onerror never fires. Added a regression test that parses the sanitized output and asserts no <img>/<script> element is produced.

[JustinCCYap]

🟠 String.fromCodePoint throws on out-of-range codepoints, which propagates out of sanitizeHtml and breaks the entire Box/label render (a content-controlled DoS). Verified:

input:  <style>.a{content:"\110000"}</style>
result: RangeError - Invalid code point 1114112

Any escape above U+10FFFF (or in the surrogate range) triggers it. Per the CSS spec, such escapes should map to U+FFFD, not error:

.replace(/\\([0-9a-fA-F]{1,6})[ \t\n\f\r]?/g, (_, hex) => {
  const cp = parseInt(hex, 16)
  return (cp === 0 || cp > 0x10FFFF || (cp >= 0xD800 && cp <= 0xDFFF))
    ? '\uFFFD' : String.fromCodePoint(cp)
})

Minor, related: decoding escapes globally and then regex-stripping can alter legitimate CSS where an escape was semantically meaningful (e.g. inside a content: "…" string, or an escape that prevents token concatenation), so the "renders identically" comment isn't fully accurate. Low impact for this use case — just flagging.

[chschan]

Fixed. decodeCssEscapes now guards the codepoint and maps null / out-of-range (> U+10FFFF) / surrogate escapes to U+FFFD per the CSS spec, instead of letting String.fromCodePoint throw:

.replace(/\\([0-9a-fA-F]{1,6})[ \t\n\f\r]?/g, (_, hex) => {
  const cp = parseInt(hex, 16)
  return (cp === 0 || cp > 0x10FFFF || (cp >= 0xD800 && cp <= 0xDFFF)) ? '�' : String.fromCodePoint(cp)
})

Added regression tests asserting both \110000 and a surrogate \d800 no longer throw.

On the related note: agreed — I've corrected the code comment so it no longer claims decoding "renders identically"; it now notes that global decoding can alter otherwise-meaningful escapes (in content: strings, or token-separating escapes), an accepted trade-off on this best-effort, lower-severity surface to defeat the escape-based bypass.

[JustinCCYap]

LGTM, same comment about testing before merging as the rhtmlCombinedScatter PR.