DOMPurify 3.1.7 breaks Mermaid diagrams using `foreignObject`

[slorber]

Background & Context

I'm the maintainer of [Docusaurus], which uses Mermaid diagrams, which uses DOMPurify.

Example: https://docusaurus.io/docs/markdown-features/diagrams

Bug

DOMPurify 3.1.7 breaks retrocompatibility due to a recent foreignObject-related change from @masatokinugawa

This breaks Mermaid diagrams on all our newly initialized sites and is likely to affect other Memaid users very soon (800k dl/week).

Excuse me for not respecting your issue template because the only thing I know is that DOMPurify is causing the problem. I'm not sure how Mermaid uses the lib exactly, and not even sure if they are aware (yet) of the problem.

What I can say is that Mermaid uses DOMPurify, v3.1.6 worked and v3.1.7 doesn't, with foreignObject tags in SVGs being empty:

A temporary fix I'll advise to our users is to force a downgrade with package manager features, for example Yarn resolution

  "resolutions": {
    "dompurify": "3.1.6"
  },

I'm also unsure what's the versioning policy of the lib, but I assume it implicitly respects semver and not release breaking changes on patch releases right? If you plan to release breaking changes on patch releases, it's better to say so explicitly so that users can pin the dependency to a given version.

In the meantime I'd suggest to revert the change to restore retro-compatibility, and postpone this change to v4 to avoid disrupting the Mermaid and Docusaurus ecosystems.

Thanks

---

Related issues:

• Mermaid: https://github.com/mermaid-js/mermaid/issues/5904
• Docusaurus: https://github.com/facebook/docusaurus/issues/10526

[cure53]

Heya, thanks for filing the issue :slightly_smiling_face:

The good news is, you will be fine using 3.1.6 and the resolution you mention works. For 3.1.7 we needed a fix - but as the issue only becomes impactful when a specific set of config options are being used, users relying on 3.1.6 are likely gonna be fine as well.

Please keep in mind that our priority is to prevent XSS and that is what we do. If something breaks for someone along the lines, we need to accept that as what it is and can always discuss that matter here.

I'm also unsure what's the versioning policy of the lib, but I assume it implicitly respects semver and not release breaking changes on patch releases right?

I see your point here but frankly, as long as we don't break our own API or alike, we will keep doing releases as we do because nothing else makes sense. We're offering a sanitizer and with several billion sanitizations per day, there will always be one that is so specific that something will break.

As long as our tests don't break, we need to assume low collateral and that is what we do. If nevertheless something breaks somewhere on the planet, well - so be it. We can neither predict nor prevent this. And weaving our version strategy around that would not make any sense.

So, summed up, with 3.1.6, you are perfectly fine and all is good. Maybe we will make HTML entry points configurable in later versions, let's see how things go. The issue we did the change for is not yet fully researched so things might get adjusted in future releases. We'll see :smile:

Until then, unless there is an actual security release, rest assured that you can stay on the version you are not having issues with. And once there is an actual need to upgrade because of a bypass, then there will likely be some way to work around any breakage if any.

[slorber]

Thanks for the explanation, I understand your point of view, that "freezing behavior" would somehow tie your hands and prevent library evolution, and that this is not considered as a public API surface.

We'll stick to 3.1.6, but it's appreciable to have the option to restore former behavior on v3.1.7+ because as you said, in case of a security issue in older versions it could be problematic to not have the ability to upgrade and benefit from a security patch.

[cure53]

Cool, thanks - then let's conclude that for now, 3.1.6 is best to use in your case and very likely, for 3.1.8, we will add a config option enabling you to define the HTML entry-points in SVG - addressing this and potentially future problems as well :slightly_smiling_face:

[slorber]

Thanks 🙏

[souldzin]

@cure53 in hindsight, should 3.1.7 have been a major version instead of patch version bump? I'm fine with DOMPurify breaking things in the name of security, but as a consumer it'd be nice to have an indicator that a version bump has a risk of breaking normal functionality.

[cure53]

@souldzin Nope, we didn't break anything knowingly. Our tests were green, our API unchanged - so no breakage we can observe.

If anything with some markup somewhere in some setup changes after all, we cannot foresee this and judging by that, technically any release might be "breaking" :slightly_smiling_face:

[cure53]

@slorber Heya :slightly_smiling_face: We have a commit on main that should fix your problem.

You can now do this:

DOMPurify.sanitize(
  '<svg><foreignObject><h1>HELLO</h1></foreignObject></svg>', 
  {
    ADD_TAGS: ['foreignObject'], 
    HTML_INTEGRATION_POINTS: {'foreignobject': true}
  }
); 

Does this address the issue and allows you to get the sanitization output you need for Mermaid?

[slorber]

Thanks

I'm not a Mermaid dev but I guess @aloisklink @sidharthv96 will be able to upgrade and unpin the lib dependency

[aloisklink]

Hi @cure53, sorry for the late reply!

e4caa679715187b17e8af5cdb14ad02406621ac8 seems to work fine if we add HTML_INTEGRATION_POINTS: {'foreignobject': true}!

I did have some issues with eb7b40de8cc77b0531959ae2b741a2273dcd7ecc, but it's due to TypeScript issues, the JavaScript code seems to be fine! I've made https://github.com/cure53/DOMPurify/pull/1008 to fix it.

I also got some type errors like:

packages/mermaid/src/diagrams/common/common.ts:35:13 - error TS2769: No overload matches this call.
  Overload 1 of 3, '(entryPoint: "uponSanitizeElement", hookFunction: UponSanitizeElementHook): void', gave the following error.
    Argument of type '"beforeSanitizeAttributes"' is not assignable to parameter of type '"uponSanitizeElement"'.
  Overload 2 of 3, '(entryPoint: "uponSanitizeAttribute", hookFunction: UponSanitizeAttributeHook): void', gave the following error.
    Argument of type '"beforeSanitizeAttributes"' is not assignable to parameter of type '"uponSanitizeAttribute"'.
  Overload 3 of 3, '(entryPoint: "beforeSanitizeAttributes" | "beforeSanitizeElements" | "afterSanitizeElements" | "afterSanitizeAttributes" | "beforeSanitizeShadowDOM" | "uponSanitizeShadowNode" | "afterSanitizeShadowDOM", hookFunction: Hook): void', gave the following error.
    Argument of type '(node: Element) => void' is not assignable to parameter of type 'Hook'.
      Types of parameters 'node' and 'currentNode' are incompatible.
        Type 'Node' is missing the following properties from type 'Element': attributes, classList, className, clientHeight, and 115 more.

35   DOMPurify.addHook('beforeSanitizeAttributes', (node: Element) => {

However, according to #1006, this is expected behaviour:

• The callbacks for hooks now define the first parameter as a Node, not an Element - this matches what DOMPurify can actually pass to a hook.