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

use DOMParse in pasteHTML to protect against dangerous html #990

Closed
wants to merge 2 commits into from
Closed

use DOMParse in pasteHTML to protect against dangerous html #990

wants to merge 2 commits into from

Conversation

benbro
Copy link
Contributor

@benbro benbro commented Sep 19, 2016

fix #981

@benbro
Copy link
Contributor Author

benbro commented Sep 19, 2016

Only tested with Firefox 48.

@jhchen
Copy link
Member

jhchen commented Sep 21, 2016

It seems to have not passed the unit test for Chrome+Firefox on all operating systems.

@benbro
Copy link
Contributor Author

benbro commented Sep 21, 2016

@is it possible that it's something specific to the tests?
Maybe something in initialize?
https://github.com/quilljs/quill/blob/develop/test/helpers/unit.js#L104
Not sure what's the difference because initialize just set the container's html.

Testing with this PR produce the correct delta when using:

quill.pasteHTML('<p class="ql-align-center">Test</p>');

or

<div class="standalone-container">
  <div id="snow-container"><p class="ql-align-center">Test</p></div>
</div>

Assuming that it'll work, do you want to use DOMParser or close the PR?
I think it's different from dangerouslySetInnerHTML in React because Quill use Delta which I think are safe.
Only pasteHTML was unsafe.

I'm not using pasteHTML myself. I created this PR to help with the discussion.

@jhchen
Copy link
Member

jhchen commented Sep 24, 2016

If DOMParser works without issue it would be a good idea to use it. I do not want to rename back to pasteHTML though. I want to set the expectation that the HTML you hand Quill should already be safe, and while Quill should do things safely itself, it is not the security solution to unsafe input.

@benbro
Copy link
Contributor Author

benbro commented Sep 24, 2016

Do you have a suggestion about the failing tests?

@jhchen
Copy link
Member

jhchen commented Sep 25, 2016

It looks like the reason is getComputedStyle cannot be used. Quill uses this to figure out where to place newlines and sometimes when applications paste HTML that contain classes and <style> tags.

@benbro
Copy link
Contributor Author

benbro commented Sep 25, 2016

I see. This example gives me:
'inline' in Firefox 49 on Windows 7
empty string in Chrome 53 on Windows 7
'block' in IE11 on Windows 7
http://codepen.io/anon/pen/EgWRrV

@jhchen
Copy link
Member

jhchen commented Sep 29, 2016

In light of this I'm not seeing a way to use DOMParser.

@jhchen jhchen closed this Sep 29, 2016
@benbro
Copy link
Contributor Author

benbro commented Sep 29, 2016

Thanks

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

Successfully merging this pull request may close these issues.

pasteHTML should prevent script execution
2 participants