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

Replace sanitize-html #4500

Merged
merged 6 commits into from
Mar 23, 2021
Merged

Conversation

lbeaufort
Copy link
Member

@lbeaufort lbeaufort commented Mar 19, 2021

Summary

To silence recurring errors for sanitize-html, how about we switch to dompurify instead?

QUESTIONS

The tests for this kept failing for something I'm not sure is the best expectation:

While escaping " characters, for some reason the text wants (straight double) quotes to be converted to ", (okay, fine, not unexpected) but then the regex removes the & and ; so quotes get turned into quot rather than a legit escaped value. Seems like they should be stripped out or turned into ", ", 0x22, %22…—is there a reason we're going through the trouble of escaping it only to rip it to pieces, too?

I've added a patch for this to pass tests and behave as currently expected but I don't like it.

Are there other characters where this is or should be happening but which aren't in tests?

If we change this, how will it affect site search and filters? I can see removing straight quotes from filters but they help with grouping terms for site search. There are special characters in organizations' names, too: lots of companies with & in their name.

Impacted areas of the application

Anything that uses helpers.sanitizeValue(), which is every typeahead and some tests. Typeaheads including filters, site search, candidate lookups in widgets… but if it's fixed in search + filters, it's likely good everywhere.

I changed the _.isArray() calls to the modern Array.isArray() since it was so easy to take another step from Underscore

Screenshots

No visual changes

Related PRs

None

How to test

  • pull branch
  • npm i
  • npm run test-single (it also does its own build)
  • through unpredictable searches into various typeaheads, especially for search and for filters that create chiclets. For examples of unusual searches, see lines 88-151 of fec/fec/tests/js/helpers.js

Because we've removed sanitize-html, that vulnerability warning should vanish


@rfultz rfultz changed the title [WIP] Replace sanitize-html? Replace sanitize-html Mar 19, 2021
Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Copy link
Contributor

@johnnyporkchops johnnyporkchops 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, great job getting domPurify to work!

@lbeaufort lbeaufort deleted the feature/4026-upgrade-sanitize-html branch June 22, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Snyk: High] Arbitrary Code Execution in sanitize-html (due 10/9/20)
4 participants