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

Review usage of dangerouslySetInnerHTML #526

Open
nickdaugherty opened this issue Sep 13, 2018 · 3 comments · May be fixed by #530
Open

Review usage of dangerouslySetInnerHTML #526

nickdaugherty opened this issue Sep 13, 2018 · 3 comments · May be fixed by #530
Labels
Status: has patch Type: enhancement
Milestone
2.0

Comments

@nickdaugherty
Copy link
Contributor

@uxcitizen noticed that there are several spots in this plugin using React's dangerouslySetInnerHTML.

https://github.com/Automattic/liveblog/search?q=dangerouslysetinnerhtml&unscoped_q=dangerouslysetinnerhtml

How many of those can we get rid of with refactoring?

Obviously some places are showing pre-rendered HTML from the server. In those cases, we should be sanitizing the HTML to strip out any unexpected or disallowed tags and attributes before passing it to dangerouslySetInnerHTML to reduce the potential for abuse.

Ideally we have 0 of these calls, but for the ones we can't get away from, we should be sure everything is sanitized as far as possible.
@cain
Copy link
Contributor

cain commented Sep 14, 2018 •
edited
Loading

Seems like all of the WordPress endpoints return HTML, it's not possible without using dangerouslySetInnerHTML

But you are right, some sanitizing needs to be done. https://github.com/cure53/DOMPurify works well.

@davidsword
Copy link
Contributor

I'll look into this!

@cain
Copy link
Contributor

cain commented Sep 25, 2018

@davidsword looks really good!

@davidsword davidsword linked a pull request Sep 25, 2018 that will close this issue
Open
@philipjohn philipjohn added this to the 2.0 milestone Sep 27, 2018
@philipjohn philipjohn mentioned this issue Oct 16, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Status: has patch Type: enhancement
Projects
1.10 - Maintenance Release
Status: To do
Milestone
2.0
Development

Successfully merging a pull request may close this issue.

add sanitization to dangerouslySetInnerHTML values
5 participants
@GaryJones @philipjohn @nickdaugherty @davidsword @cain