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 dompurify internally to prevent XSS #791

Merged
merged 11 commits into from
Feb 22, 2021
Merged

Use dompurify internally to prevent XSS #791

merged 11 commits into from
Feb 22, 2021

Conversation

samclarke
Copy link
Owner

@samclarke samclarke commented Feb 13, 2021

Based off PR #767 this uses dompurify internally to sanitise all HTML.

It would add ~7kb to the compressed size and add dompurify as a dependency but should make all methods of inserting HTML into the editor safe from XSS.

Closes #767

@github-actions github-actions bot added the lib label Feb 13, 2021
Copy link
Collaborator

@live627 live627 left a comment

Choose a reason for hiding this comment

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

This set of changes fixes the injection problem. Thanks.

I see that you're worried about filesizes like me. I think the cost (7KB) is totality worth it for what the library does.

src/lib/RangeHelper.js Show resolved Hide resolved
src/lib/SCEditor.js Outdated Show resolved Hide resolved
src/lib/SCEditor.js Outdated Show resolved Hide resolved
src/lib/SCEditor.js Outdated Show resolved Hide resolved
src/lib/SCEditor.js Outdated Show resolved Hide resolved
@samclarke
Copy link
Owner Author

I've tested the plugins and formats and so far everything seems to be working with this. I think all that's left is to decide if to allow target=_blank on links and if DOMPurify needs to be exposed or not.

@live627 is there anything you would need the ability to add hooks to DOMPurify for?

@live627
Copy link
Collaborator

live627 commented Feb 18, 2021

I'm of the opinion that things should be modular any any fluff optional and not built in. Certainly the youttube command is an extra goodie that should be a plugin . Same for target=_blank. Built in commands don't use it, so adding support for it should not be in the base editor. Gotta be mindful of scope creep and all that.

I don't currently have a need to add a hook for dompurify. I requested that just in case someone else needs it. I want to preempt a future change to support that. Might as well tackle it now.

@samclarke
Copy link
Owner Author

That's the aim although things like the YouTube command and emoticons support are still included for backwards compatibility as they were included very early on, possibly the first version. At some point they should be moved out though.

I've been thinking about target=blank some more. As it is very common I think the XHTML format should really include support for it but it shouldn't affect BBCode much (at least not in a way that can't be worked around fairly easily) so probably doesn't need to be in the core.

I think it would be better to avoid exposing dompurify if possible so if there isn't a need at the moment I'll keep it private for now. It's easy to make public if ever needed but once exposed it can't be made private again without making a breaking change.

@samclarke samclarke merged commit 1e4625a into master Feb 22, 2021
@samclarke samclarke deleted the dompurify branch February 23, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants