-
Notifications
You must be signed in to change notification settings - Fork 122
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.
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
add sanitization to dangerouslySetInnerHTML values #530
base: master
Are you sure you want to change the base?
add sanitization to dangerouslySetInnerHTML values #530
Conversation
There are several spots in this plugin using Reacts `dangerouslySetInnerHTML` attribute to place content from the server. This can easily lead to cross-site scripting (XSS) attacks. Before the HTML is dangerously placed, running it through DOMPurify https://github.com/cure53/DOMPurify helps to sanitize out malicious HTML. iframe (an easy method for XSS), are allowed for the embedded media, however the domain used must be whitelisted - otherwise the iframes src is removed.
@davidsword Looks like you need to build the assets :) |
This looks great! I suspect @cain is correct in needing to rebuild the compiled JS to get it working. One thing that came to mind is, what happens if someone has an extra domain they want whitelisted. For example to their own site or to some other service (that they trust)? Is there a way we could have a filter in the plugin where they could add domains? |
'open.spotify.com', | ||
'player.vimeo.com', | ||
'www.youtube.com', | ||
// Instagram and Twitter don't use iframes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we will strip out Twitter and Instagram embeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how was this list of domains generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
negative. Twitter & Instagram continue to work and can be used.
Specifically, for Tweets: the Rest API has the entry content with <blockquote class="twitter-tweet">...</blockquote>
and then the Twitter embed is fully rendered by using the platform.twitter.com/widgets.js
script that's already enqueued into wp_head()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list of domains was generated taking liveblogs advertised media embed features, adding them to an entry, and seeing what WordPress/oembed gave back. In writing that, I realize I need to take a closer look into these domains (ie: subdomain changes for different regions or languages) and each services documentation to ensure the domains are correct and reliable for each service.
& as pointed out by @sboisvert, a filter to manage this whitelist would be needed.
Chatted with @mjangda on this.
|
* @return {string} sanitized HTML | ||
*/ | ||
export const sanitizeHTML = (dirty) => { | ||
// Whitelist iframes for the plugins 'embded media' feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in embed
Thank you all for the input and questions. I will answer and look into this more this week. |
The custom endpoints on the REST API that are feeding Since XSS prevention is needed and no sanitization is occurring (besides on new entry submissions), wether we do it with PHP at the custom endpoints, or with JS using DOMPurify, it seems breaking changes are inevitable. I'm not sure why things are in this current state, perhaps a broader history lessons needs to happen. It looks like the old PHP version of the plugin did have As for what DOMPurify breaks (as the PR currently is), here are the implicit blacklist for tags & attrs (whitelist: tags & attrs). This PR only varies from the defaults by whitelisting & thanks for bringing up the questions. This PR lacked plugin documentation for what is default white/blacklisted, and Upgrade Notice details on breaking change, and so on. More info needed. As well as having the white/blacklist be filterable, and documentation on that. I will add that in as this goes along. |
@davidsword Any interest in fixing the merge conflicts and finishing this one off? |
This fixes: #526
There are several spots in this plugin using Reacts
dangerouslySetInnerHTML
attribute to place content from the server. This can easily lead to cross-site scripting (XSS) attacks.Before the HTML is dangerously placed, running it through DOMPurify helps to sanitize out malicious HTML.
iframe
's (an easy method for XSS), are allowed for the embedded media, however the domain used must be whitelisted - otherwise thesrc
is removed.The domains for the iframe whitelist reflect the front end "Embed Media" list https://github.com/Automattic/liveblog/blob/master/README.md#embedding-media
<script>
and/or non-whitelisted<iframe>
a user has added themselves in an entry, including content within 'HTML Blocks'. When merged into a upcoming release, this should be mentioned in the Upgrade Notice.(& thank you for ignoring the typo in my branch ;)