fix(preview): route the preview through the shared sanitize contract - #384
Merged
Merged
Conversation
#376 introduced `src/lib/utils/sanitize.ts` as the single markdown sanitize policy, and the comment on its `FORBID_TAGS: ['style']` entry argues the preview case specifically: the sanitized HTML is injected into the app's own document, where a document `<style>` can hide the title bar or turn any selector into an outbound beacon. Only the export path ever imported it. The preview kept a local copy of the URI regexp and passed `ALLOWED_URI_REGEXP` alone, so `<style>` - which DOMPurify allows by default, CSS unfiltered - reached the live document. Measured in a browser against the pinned DOMPurify build: the title bar's computed `display` became `none` and the outbound request for the beacon appears in `performance.getEntriesByType('resource')`. The app CSP permits both halves. The deleted regexp was byte-identical to the shared one, so no URI behaviour changes; the only delta is that author `<style>` is dropped. The order difference between the two sinks is left alone and now documented: export sanitizes first because its bytes leave the app and filtering Markpad's own generated markup could silently delete part of the export; the preview sanitizes last because it has no second line of defence, so the string reaching `{@html}` must BE the sanitizer's output with no round trip after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 5, 2026
…cannot delete them (#455) Every diagram whose labels Mermaid put in a `<foreignObject>` rendered as empty shapes — in the live preview and in the exported HTML alike, because both call `renderRichContent` → `sanitizeDiagramSvg`. ## Mechanism `sanitizeDiagramSvg` allowed the `foreignObject` *element*: DOMPurify.sanitize(svg, { ADD_TAGS: ['foreignObject'], ADD_ATTR: [...] }) The label is not the element. It is the HTML inside it — `<div class="labelBkg">…<span class="nodeLabel">Alpha</span></div>` — and DOMPurify deletes HTML children of an SVG element unless the parent is an HTML integration point: // dompurify 3.4.12, dist/purify.es.mjs const HTML_INTEGRATION_POINTS = freeze(['annotation-xml']); _checkHtmlNamespace = function (tagName, parent, parentTagName) { if (parent.namespaceURI === SVG_NAMESPACE && !HTML_INTEGRATION_POINTS[parentTagName]) return false; `foreignobject` is not on that list, so no `ADD_TAGS` entry could save its children. `DOMPurify.removed` grew one entry per label and what survived was literally `<foreignObject width="37.98" height="24"></foreignObject>`. ## Scope, measured Real mermaid 11.16.0 driven by real dompurify 3.4.12 in a browser, over every diagram type 11.16.0 ships a renderer for — not only the five sampled in the report: labels entirely deleted (10) flowchart, flowchart-v2, classDiagram, stateDiagram, stateDiagram-v2, erDiagram, requirementDiagram, mindmap, block, kanban labels hidden (1) journey — see the `<switch>` note below unaffected sequence, gantt, pie, quadrantChart, gitGraph, C4Context, timeline, sankey, xychart, architecture, info, ishikawa, wardley, treemap, packet, radar, treeView — all SVG `<text>` ## The fix `mermaid.initialize` now passes `htmlLabels: false`. Mermaid then emits SVG `<text>`, which no sanitizer objects to; every label above survives. One root-level key is enough — the per-diagram `flowchart.htmlLabels` / `class.htmlLabels` / … settings are deprecated in 11.x and the root one takes precedence over them — so the diagram-specific keys are deliberately not set. Because nothing then depends on HTML inside the SVG, `foreignObject` also leaves `sanitizeDiagramSvg`'s `ADD_TAGS`. Nothing needs it: - Mermaid still emits `foreignObject` unconditionally in three places — venn `text` nodes, eventmodeling boxes, architecture `iconText` — but their HTML children are deleted by the rule above whether or not the tag is allowed, so the allowance could only ever produce an empty box. Measured: venn's "Bravo" label is absent with the tag allowed and absent without it. - The `<switch>`-based renderers (journey by default, and anything configured `textPlacement: 'fo'`) pair the `foreignObject` with an SVG `<text>` fallback. A browser renders the first child it supports, so an emptied-but-present `foreignObject` *suppressed* a label that had come through the filter intact. Measured: the journey's task labels are 0×0 with the tag on the allowlist and 37×20 with it removed. Dropping it fixes a diagram `htmlLabels: false` alone cannot. So the filter gets smaller, not larger. ## The rejected alternative DOMPurify 3.4.12 accepts `HTML_INTEGRATION_POINTS` as a config option, so allowing `foreignObject` to be an integration point is a candidate fix that would have preserved rendering fidelity exactly. Two findings: - It only works as an object map. `HTML_INTEGRATION_POINTS: ['annotation-xml', 'foreignobject']` is a silent no-op — the value is `clone()`d, and cloning an array yields index keys, which also drops `annotation-xml`. Passing `{ 'annotation-xml': true, foreignobject: true }` does restore every label. - It is the wrong trade. It re-permits HTML inside SVG, which DOMPurify keeps out on purpose because serialise-then-reparse is the mutation-XSS primitive — and `container.innerHTML = sanitizeDiagramSvg(svg)` is exactly that reparse. Mermaid source is document content, so the SVG is attacker-influenced; #384 exists in this same cycle because a document's `<style>` reached the app's own DOM. Measured difference: with the override, `<svg><foreignObject><img src=x></foreignObject></svg>` survives sanitisation and materialises as a live `<img>` on the reparse, where today (and with this fix) it does not. ## Trade-offs of `htmlLabels: false` - Markup inside a node label renders literally: `A["<b>bold</b> text"]` comes out as the six characters `<b>` followed by `bold`. No sample, test or doc in this repo puts HTML in a Mermaid label (checked: `samples/` has one diagram, `graph TD` with plain labels). - KaTeX inside a label goes the same way — Mermaid's math path runs only under `useHtmlLabels` — so `A["$$x^2$$"]` renders as its source. It rendered as nothing at all before this change, so this is not a loss. - Wrapping is measured by the SVG text engine instead of the browser's layout, so long labels break at slightly different points. ## Not a v2.7.0 regression The allowance predates this cycle: it was introduced in eb9a1c7 ("Fix Mermaid diagram rendering with SVG foreignObject support", 2026-02-04) and #411 only moved it into `richContent.ts`. v2.6.13 ships the byte-identical config, and the dompurify it shipped with (3.3.1) has the same `addToSet({}, ['annotation-xml'])` and the same SVG-namespace check. So the release does not have to hold for this. ## Tests `scripts/mermaidDiagramLabels.test.ts` runs the real `renderRichContent` over a real code block with Mermaid answered out of `scripts/mermaidDiagramCorpus.json` — bytes real mermaid 11.16.0 emitted, captured once with the old config and once with the new one, and keyed by the config the pipeline actually sends, so a config nobody has measured is an error rather than a pass. It parses what the pipeline produced and asserts there is no HTML in the SVG for the namespace rule to reach and that every label is in an SVG `<text>`. What it cannot execute is DOMPurify: without a DOM the library returns a bare factory with no `sanitize` at all, so it is stood in for by the identity function as `exportRichContent.test.ts` already does, and a third test asserts that state so the middle one is not misread as "the filter kept the labels". Making that half real needs a DOM faithful enough to reproduce HTML5 foreign-content parsing, which is the rule under test — a shim written here would be marking its own homework, and no test-only DOM dependency was added. Falsified: with only `htmlLabels: false` reverted, the corpus stand-in returns the old bytes and the middle test fails with `flowchart: the rendered diagram still carries its labels in foreignObject, whose HTML children DOMPurify removes regardless of ADD_TAGS`, 3 !== 0. `previewSanitize.test.ts` asserted the source text `ADD_TAGS: ['foreignObject']`. That assertion confirmed a config string existed while every label was being stripped, so it is replaced rather than re-anchored: the split between the two sanitizer configs is now pinned on the reason that survives — the diagram filter must permit the `<style>` the document policy forbids. The rationale comment in `scripts/sourceTree.ts` still says the diagram config "needs `foreignObject`"; it is left for #454's rewrite of that tree rather than conflicting with it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
#376 introduced
src/lib/utils/sanitize.tsas the single markdown sanitize policy. The comment on itsFORBID_TAGS: ['style']entry argues the preview case specifically:Only
export.tsever imported it. The preview kept its own copy of the URI regexp and passedALLOWED_URI_REGEXPalone — so the one tag that comment was written about was the one the preview did not forbid. Providing the correct path is not the same as closing the wrong one.Measured, not argued
Real DOMPurify (the pinned 3.4.12 build), the real
processMarkdownHtmlfrom this branch, injected into a live document containing a.titlebar, in the preview's real order:<style>.titlebarcomputeddisplaybodycomputedbackground-image{ALLOWED_URI_REGEXP}— beforenoneurl("https://attacker.example/beacon")MARKDOWN_SANITIZE_CONFIG— afterflexnoneThe beacon is not hypothetical: after injection,
performance.getEntriesByType('resource')lists the outbound request. Both halves are permitted by the shipped CSP (style-src 'self' 'unsafe-inline' …,img-src 'self' asset: https: …).The fix
Import
sanitizeMarkdownHtml; delete the localmarkdownLinkExtensions/markdownLinkExtensionPattern/allowedMarkdownUriPattern.sanitize.tsis unchanged — the shared config was already right.The deleted regexp was verified byte-identical to the shared one (
String(local) === String(shared)→true), so no URI behaviour changes. The only behavioural delta is that an author's<style>is dropped.The order difference is deliberate now, not accidental
git log -S "import DOMPurify"shows the preview's sanitize call arrived ineb9a1c7(MermaidforeignObjectsupport), bolted onto the render sink where the processed HTML was already cached. The ordering was never a decision.But the resulting order is the right one for this sink, and it is kept:
exportSanitize.test.tspins that order.{@html}must be the sanitizer's output, with no parse/serialize round trip after the filter ran.Both rationales are now in a comment at the call site, and a test fails if the preview call is moved ahead of processing, so the asymmetry cannot be "tidied up" later.
The render pipeline was verified, not assumed
Measured in a browser, on a fixture with headings/folds, task items, a folded callout,
<img style="width:50%">, video replacement, a YouTube thumbnail, a table, code blocks and inline + display math:processMarkdownHtmloutput contains no<style>; the fixture sanitizes byte-identically under the old and new configs.<style>(KaTeX's CSS is a real imported stylesheet).styleattributes still survive;onerroris still dropped.<style>block —#id-scoped fonts, stroke widths, dash patterns. It is unaffected: mermaid runs on the live DOM after{@html sanitizedHtml}, through the separatesanitizeDiagramSvg, which needsforeignObject(the document policy forbids it) and needs that inline<style>(the document policy forbids it too — applying the shared config to a diagram flattens its fonts and edge styling, measured). The two policies must stay separate, and a test now asserts the split so a future "let's unify these" has to read why.Alignment with other editors
<style>and<meta>won't be applied either."<style>.default-src 'none', so document CSS physically cannot reach the editor chrome. Markpad injects into its own document, so stripping is the equivalent available here.Tests
scripts/previewSanitize.test.ts(new). DOMPurify cannot run undernode --test(no DOM, and the repo deliberately has no test-only DOM dependency — the shim from #378 is scoped toprocessMarkdownHtmland should not grow into a general parser), so the test pins the wiring chain the wayexportSanitize.test.tsdoes and records the browser measurements verbatim in its header.It also adds a call-site allowlist for
DOMPurify.sanitize(acrosssrc/— the durable form of this bug, since no compiler can see a new private policy appearing.npm test364 / 364,npm run check0 errors 0 warningsNot covered
processMarkdownHtml+ real live-DOM computed styles + a real outbound request, plus the CSP read fromtauri.conf.json; I did not launchtauri dev.<style>is emitted from user-controlled diagram source, so a CSS breakout viaclassDef/styledirectives is conceivable. One attempt was rejected by mermaid's own lexer, but one probe is not an audit. Worth a separate look.npm ls dompurifyreports the installed tree asinvalid(3.3.1 present despite the 3.4.12 pin/override). All numbers above were re-measured against a freshly fetched 3.4.12. Worth checking separately; this PR does not touch it.🤖 Generated with Claude Code