Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move safeHtml to React component. #3335

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Move safeHtml to React component. #3335

wants to merge 5 commits into from

Conversation

monneyboi
Copy link
Contributor

@monneyboi monneyboi commented Sep 26, 2023 •
edited

Fixes #3334. I propose to move the safe html rendering to React for a couple of reasons:

  • bodyHtml is 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.

tillprochaska
tillprochaska requested changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@tillprochaska tillprochaska left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few minor notes:

  1. 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?

  2. Could you add tests (that cover at least the existing Python test cases)? There are few tests at the moment for the frontend codebase, but we try to add them for new stuff and it would be valuable especially for critical stuff like HTML sanitization imo. You can take a look at this test suite for the testing boilerplate: https://github.com/alephdata/aleph/blob/develop/ui/src/components/Timeline/util.test.ts

  3. There’s a reference to safeHtml in ui/src/selectors.js that can be removed, too.

ui/src/viewers/HtmlViewer.jsx Outdated Show resolved Hide resolved
ui/src/viewers/HtmlViewer.jsx Outdated Show resolved Hide resolved
@monneyboi
Copy link
Contributor Author

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.

@monneyboi
Copy link
Contributor Author

The merge conflict seems to be caused by the package-lock.json file not having been updated on the develop branch.

@monneyboi
Copy link
Contributor Author

Merge conflict has been fixed, can this get merged?

@tillprochaska
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tillprochaska tillprochaska Awaiting requested review from tillprochaska

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Some web pages don't render
2 participants
@monneyboi @tillprochaska