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

chore: move from sanitize-html to js-xss #1213

Merged
merged 1 commit into from
Sep 20, 2024
Merged

chore: move from sanitize-html to js-xss #1213

merged 1 commit into from
Sep 20, 2024

Conversation

chohner
Copy link
Member

@chohner chohner commented Sep 19, 2024

I realized that our dependency to sanitize rendered markdown causes a huge chunk of runtime warnings:

image

This is due to a transitive postcss dependency, see this issue.

There are two options:

  • use an isomorphic library (this PR)
  • render & sanatize markdown on server (preferred but larger architectural change)

Impact:

  • tests pass & visual inspection looks good ✅
  • 14% reduction in gzipped bundle size:
    441.4 kB -> 381.12 kB

@chohner chohner changed the title chore: move from sanitize-html to xss chore: move from sanitize-html to js-xss Sep 19, 2024
@aaschlote
Copy link
Contributor

Can you please add tests for the h elements? It was added to be allowed recently but we don't have tests. And also a specific tests for removed some html tag element, like code? We don't have tests for that.

Besides that, the changes looks fine!

@chohner
Copy link
Member Author

chohner commented Sep 19, 2024

Can you please add tests for the h elements? It was added to be allowed recently but we don't have tests. And also a specific tests for removed some html tag element, like code? We don't have tests for that.

Besides that, the changes looks fine!

Good idea - I might do this before & outside of this PR

@pgurusinga
Copy link
Member

pgurusinga commented Sep 19, 2024

nice one, it seems the new package is trustworthy https://socket.dev/npm/package/xss, can we add ADR for this? 🎉

Copy link

Copy link
Contributor

@aaschlote aaschlote left a comment

Choose a reason for hiding this comment

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

LGTM

@chohner chohner merged commit a8940a0 into main Sep 20, 2024
19 checks passed
@chohner chohner deleted the replaceSanatizeDep branch September 20, 2024 15:44
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.

3 participants