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.

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

Clarify the XSS risk of KEEP_CONTENT #489

Closed
TazmanianDI opened this issue Nov 18, 2020 · 3 comments
Closed

Clarify the XSS risk of KEEP_CONTENT #489

TazmanianDI opened this issue Nov 18, 2020 · 3 comments

Comments

@TazmanianDI
Copy link

This issue proposes a feature which...

clarifies this example and comment in the main documentation.

// keep an element's content when the element is removed (default is true, careful, minor XSS risks here)
var clean = DOMPurify.sanitize(dirty, {KEEP_CONTENT: false});

It's a little ambiguous here if the "minor XSS risk" refers to "default is true" or if it refers to this specific example where it's setting the value to false. I would assume that the default setting of true is where the minor XSS risk occurs which is a little troubling because it sounds like the default settings for this library has a minor XSS risk but doesn't that defeat the point? Do we need to set KEEP_CONTENT: false to avoid this minor XSS risk?

Can you perhaps clarify this and expand on what the risk is?

@cure53
Copy link
Owner

cure53 commented Nov 18, 2020

Heya, that's a perfectly valid question :)

In the past, we had one case where an XSS was possible in case KEEP_CONTENT was changed from true to false. We have meanwhile eliminated the root cause of this. But it might - as always - be that there are variations we don't know of yet.

So, while I am not aware of any case where this flag causes XSS, we feel better with mentioning a tiny bit of residual risk here, just because we had a problem with that in the past.

@TazmanianDI
Copy link
Author

Thanks for the quick reply! So the vulnerability occurred with KEEP_CONTENT: false? If so, I would suggest you tweak the comment a little bit since it suggests the opposite.

@cure53
Copy link
Owner

cure53 commented Nov 19, 2020

Yeah, you are right. I think we will remove it for now. Thanks :)

@cure53 cure53 closed this as completed in 89fee39 Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants