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

Fix innerHTML XSS vulnerability #341

Merged
merged 5 commits into from
Sep 11, 2023
Merged

Fix innerHTML XSS vulnerability #341

merged 5 commits into from
Sep 11, 2023

Conversation

csculley
Copy link
Collaborator

@csculley csculley commented Sep 6, 2023

Closes #255

This PR fixes the XSS vulnerability issues associated with using innerHTML.

Please let me know if anything needs to be changed. Thank you!

@thexeos
Copy link
Collaborator

thexeos commented Sep 6, 2023

This is a breaking change, so a major version bump and upgrade notes would be needed in readme.

@csculley
Copy link
Collaborator Author

csculley commented Sep 7, 2023

Should we start a new CHANGELOG.md file in the root of the repository? Otherwise, I can add an upgrade notes section to the changelog if you'd like!

@csculley csculley marked this pull request as draft September 7, 2023 01:40
@thexeos
Copy link
Collaborator

thexeos commented Sep 7, 2023

Should we start a new CHANGELOG.md file in the root of the repository? Otherwise, I can add an upgrade notes section to the changelog if you'd like!

I like the README approach. It also means that people that visit npm page would see the important upgrade instructions.

@csculley csculley marked this pull request as ready for review September 7, 2023 16:25
@csculley
Copy link
Collaborator Author

csculley commented Sep 7, 2023

This should be ready to review again! Please let me know if there's anything that should be changed for better code quality or styling. Thank you!

@csculley csculley marked this pull request as draft September 8, 2023 18:05
@csculley csculley marked this pull request as ready for review September 8, 2023 18:24
@csculley
Copy link
Collaborator Author

csculley commented Sep 8, 2023

Alright, I'm feeling good about this one 😄

Copy link
Collaborator

@thexeos thexeos left a comment

Choose a reason for hiding this comment

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

Looks good to me.

In summary:

  • Existing Delta documents will loose their rendered HTML in value, but that is expected, as it may be currently polluted with XSS.
  • New Delta documents will only ever store String (plain text) in value - the documentation reflects it.
  • HTML rendering is still supported through the use of custom blots, that extend the default MentionBlot. They must implement a render method that returns an Element. Functionality of rendering HTML is maintained.
  • Furthermore, the use of custom blot name means that mention objects with custom data payload will be explicitly handled by the custom blot, rather than the default one.
  • I could not find any new XSS vectors (except for the end user introducing one in their render method).

Good work!

@csculley csculley merged commit e85262d into quill-mention:master Sep 11, 2023
@csculley csculley deleted the fix-inner-html branch September 11, 2023 13:18
@MadSpindel
Copy link
Member

Excellent job @csculley! I've now published version 4.0.0 to npm.

@csculley
Copy link
Collaborator Author

Thank you so much!!

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.

Uses innerHTML - potentially allows injection attacks
4 participants