Optionally sanitize links rendered by the links extension

[joelreske]

closes #2068

• Allows a sanitize function to be provided to the links extension, with a quick explanation of why it's important
• Adds sanitization with a common library for the demo

[netlify]

✔️ Deploy Preview for tiptap-embed ready!

🔨 Explore the source changes: 1d66d08

🔍 Inspect the deploy log: https://app.netlify.com/sites/tiptap-embed/deploys/61943d8d099c560007d1656a

😎 Browse the preview: https://deploy-preview-2169--tiptap-embed.netlify.app

[hanspagel]

Thanks! Naive question: Is it a good idea to do this on every re-render? Or should this be done initially and then as part of the commands?

[joelreske]

Hi! A bit of a tradeoff - I think having it in the render step is the safest. There would be no way to get around it, whereas I think with commands if a new one was registered they would all have to take sanitization into account? Definitely could be wrong there though.

In terms of performance, I'd be interested in a caching option (could save the key and resolved value on the mark object I think?).

These sanitize functions are really meant to be very simple string operations (example here) so I'm not very concerned but it might be good.

[philippkuehn]

Hmm, alternatively it would be possible to track every inserted link (as a plugin – not within commands) so we can sanitize it once if needed. With your solution a malicious link would still be present in the document forever. What do think?

[joelreske]

Hmm, alternatively it would be possible to track every inserted link (as a plugin – not within commands) so we can sanitize it once if needed. With your solution a malicious link would still be present in the document forever. What do think?

An interesting idea! I do think that we could solve a similar problem by having a way of generally sanitizing stored documents, potentially on edit, but I do think of this solution being a bit differently.

This PR is really meant to be a 'last line of defense' for harmful links, the user of the option shouldn't have to worry about whether a sanitization step was done before the document being rendered was stored. The concern with XSS is not so much that we always guarantee sanitized data in our DB, but rather that we are protected from malicious data that might find its way somehow into our system when rendering.

Does that distinction make sense for what you were thinking? I think that we could theoretically add both and we'd be addressing two separate but important things.

[franciscolourenco]

@joelreske thank you for the PR!

Any ETA to adopt this PR? And shouldn't TipTap be safe agains XSS by default, instead of providing this protection as an option which is off by default?

[franciscolourenco]

Sanitization on link insertion wouldn't solve this issue, if sanitization is not done server side. Another option would be to pass the whole html by a library like DOMPurify, before passing it to TipTap.

Any ETA for the adoption of this PR or an equivalent solution? Thanks!

[joelreske]

Hi @franciscolourenco , I need to put together another version of this which caches so that it's not computing every render.

I do think that TipTap should provide this behavior by default, but I think users should have the option of disabling or replacing the sanitization function.

In terms of server-side sanitization, note from earlier:

The concern with XSS is not so much that we always guarantee sanitized data in our DB, but rather that we are protected from malicious data that might find its way somehow into our system when rendering.

[bkempe]

@bdbch I wonder about the state of this?

[franciscolourenco]

As a workaround, we are currently using DOMPurify.sanitize(html, { USE_PROFILES: { html: true } }) before passing the html to TipTap.

As a word of caution, @braintree/sanitize-url has disclosed security vulnerabilities, and I wouldn't rely on it to sanitize urls. Moreover, for TipTap's use case, I don't see a reason not to use the simpler and safer alternative, which is to use a protocol whitelist (http://|https://|).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[janthurau]

fixed by #4602 (Thanks @joelreske for raising this two years ago!)