Move safeHtml to React component.
Fixes #3334. I propose to move the safe html rendering to React for a couple of reasons:
-
bodyHtmlis being sent over the wire anyway. - It's ultimately presentation logic
I would love to have used the HTML sanitizer API for this, but that's not yet supported in all major browsers. So i used DOMPurify instead, as proposed by Mozilla in their documentation on safely inserting HTML content in browser extensions.
This move also allowed me to drop the LXML python requirement.
I resolved the proposed changes.
components/common is the location where we usually keep components like SafeHtmlDocument viewer that are included in multiple other components. Could you extract the component into its own file in that directory?
I would argue that this SafeHtmlDocument is not a common component. Common components in my mind would be things that get used throughout the codebase.
After looking into the different "viewers", i would instead argue that this SafeHtmlDocument should actually be the HtmlViewer component, and that the Skeleton logic that's found in most "viewers" should ideally be moved higher up the tree to the DocumentViewMode component instead of being implemented in every viewer itself.
I was looking into refactoring that right away, but i couldn't quickly make sense of where the isPending stuff came from, as it is found all throughout the source.
Therefore i just moved SafeHtmlComponent into it's own common file.
The merge conflict seems to be caused by the package-lock.json file not having been updated on the develop branch.
Merge conflict has been fixed, can this get merged?
Hi @monneyboi, sorry I haven’t replied earlier.
I took another look at this PR and noticed that it doesn’t sanitize some HTML markup properly that can trigger HTTP requests to third parties, e.g. <div background="https://example.org"></div> would trigger an HTTP request to Https://example.org when the preview is displayed.
Also, HTML documents that contain forms seem to be sanitized differently, although I don’t think this would be a security issue.
Although I’m not against moving the HTML sanitization to the client-side per se, unfortunately I currently have limited bandwith to consider all the possible implications that this change might have.
I have however opened a small PR that fixes a bug causing incorrect previews for some web pages. This should fix preview e.g. for politico.eu: #3575