[CT-2147] Replace deprecated method with a sanitize library

[matthieucan]

Describe the feature

Currently, doc blocks are sanitized from custom html elements. This prevents going beyond markdown styling for documentation, and tinkering with features such as mermaid.js.

I understand this is enabled for security reasons, but:

• The flag markedProvider.setOptions({sanitize: true}); has been deprecated because it is not secure (see https://marked.js.org/using_advanced).

Describe alternatives you've considered

I'm not aware of any alternative.

Additional context

N/A

Who will this benefit?

Users who want to

• Apply any custom styling
• Create html elements with class (no pun intended) to be rendered by mermaid.js.
• Extend the catalog with all the possibilities offered by html and javascript.

Are you interested in contributing this feature?

Yes.

[dbeatty10]

Thanks for reaching out @matthieucan !

Even though markedProvider.setOptions({sanitize: true}); has been deprecated, I think we'd still want to replace it with something similar rather than just remove it altogether.

Those docs suggest the following:

Instead use a sanitize library, like DOMPurify (recommended), sanitize-html or insane on the output HTML.

A question I don't know the answer to: would any of these sanitize libraries be compatible with mermaid.js?

I'm not sure if there are other relevant alternatives to DOMPurify, sanitize-html, or insane 🤷.

I'm going to label the effort to replace the sanitization approach as help_wanted, if you or anyone wants to tackle it! I'll also update the issue title accordingly.

[matthieucan]

Thanks for your fast answer @dbeatty10!

I'm not sure sanitizing html elements in markdown would be compatible with the use of external libraries. The idea I had was, if I can inject <pre class="mermaid">some graph</pre> within a docs block, then I can use mermaid.js to render the graph.
But if all html elements are stripped, it won't work, unless we have a way to "mark" specific elements to be rendered.

Could you expand on the reasons why markdown is sanitized? Is it to prevent Javascript injections?
In my understanding, a user who has write access to the docs block would also have write access to the index.html, so if they wanted to inject Javascript they would be able to. But perhaps I don't get the full picture here!

[dbeatty10]

@matthieucan thanks for explaining the push/pull here between stripping HTML elements and marking a specific chunk of markdown to be rendered by mermaid.js.

I don't actually know the full story of why the markdown is sanitized. It was added in one-line PR here about 1 year ago.

@iknox-fa is there any background you can share about why sanitize: true was added? And which alternatives might be possible to replace its intent with some other mechanism?

[dbeatty10]

@matthieucan I connected with some folks internally to learn more. The summary is that we just don't allow arbitrary HTML/JS anymore because it's not a security best practice.

So at this point, we definitely want to keep the sanitization in. As you noted earlier, the current method is deprecated, so it would be best to replace it with one of the alternatives mentioned above.

I'm not sure if there's still some way to do Mermaid parsing or not, but we need to leave in a sanitization step.

[matthieucan]

@dbeatty10 Thanks for getting back to me!

In the meantime I experimented with using

```mermaid
some graph
```

Which renders into <pre class="language-mermaid">.... That's pretty neat, and I had not tried this earlier. I think that would allow to hook mermaid into a page!

It's unfortunate we can't inject different html nodes into markdown, for other unrelated experiments or styling, but I certainly understand the security concerns there. Perhaps an option to allow it, which would not be the default?

[dbeatty10]

That's awesome news @matthieucan !

For proof-of-concepts (POCs), you could definitely fork this repo and take out the sanitization so you can do unrelated experiments or styling. But we couldn't merge those in without the sanitization.

[matthieucan]

Gotcha, thanks! :)

[github-actions]

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

[matthieucan]

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please comment on the issue or else it will be closed in 7 days.

Still valid