-
-
Notifications
You must be signed in to change notification settings - Fork 527
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
Opt out from sanitize-html safely? (package too big) #429
Opt out from sanitize-html safely? (package too big) #429
Comments
Cool - thanks for the info and package size info. I didn't know the package size would increase so much when we included the sanitization. I'm not familiar with what the issues around injection are, so I'm not sure what action we need to take as a component, balanced against this bundle size issue. @wichniowski and @jgerlier can you give us some idea when/why injection is a issue for a component like this? |
I'm really not an expert on the subject and I don't know a lot about this package, as I was mainly concerned by the huge size increase the dependency update brought in the app I'm working on. However after a quick look in the code and documentation, I understant the I assume it is dangerous to inject plain HTML in the DOM as it can be used to inject a malicious script or something like that. Is the use case here to declare only one ReactTooltip element, and then reuse it with various content just by changing the I think using the DOM to pass attributes between React elements is not really the "React" way to do it, so I don't see any solution allowing to remove the sanitization that doesn't imply passing only a string or a React element / render prop as a placeholder (for example by specifying a mapping with a placeholder-id). |
I'm thinking about reverting the sanitization completely, and putting in the docs the inputs which must be sanitized if they come from users. |
Hello, we're having this exact same issue. Excellent component and dead easy to implement, however the increase in bundle size due to the sanitation makes it a no-go. Hopefully you can either make it optional or remove it. |
@wwayne I haven't had a chance to look at this, but what do you think - is it OK to remove the sanitization and document that if you use 'html' you have to sanitize user input? |
@aronhelser yeah I agree, thank you 👍 |
In the meantime I worked around this issue by making a file that exports |
Remove sanitize html, to reduce our package size. Document the need for sanitization in the readme. Fixes #429
🎉 This issue has been resolved in version 3.9.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# 1.0.0-alpha.1 (2022-04-03) ### Bug Fixes * add aria hidden attribute to style tag ([ReactTooltip#703](https://github.com/CodeForked/react-tooltip/issues/703)) ([d60c2b7](CodeForked/react-tooltip@d60c2b7)) * **aftershow:** call afterShow only after state has fully updated ([54752e8](CodeForked/react-tooltip@54752e8)) * **aphrodite_jss_deprecation:** aphrodite_jss replaced with custom solution ([fcdf7f1](CodeForked/react-tooltip@fcdf7f1)) * **aphrodite_jss_deprecation:** aphrodite_jss replaced with custom solution ([92fcf5b](CodeForked/react-tooltip@92fcf5b)) * **build:** removing single quotes on cpy for windows shell ([ReactTooltip#632](https://github.com/CodeForked/react-tooltip/issues/632)) ([9c280af](CodeForked/react-tooltip@9c280af)) * **colors:** allow customizable text, background, border, arrow colors ([9a85253](CodeForked/react-tooltip@9a85253)) * **compability:** add polyfill and change styles ([ReactTooltip#706](https://github.com/CodeForked/react-tooltip/issues/706)) ([b6e9a1c](CodeForked/react-tooltip@b6e9a1c)) * deleting warning in peer dependencies ([f30ae74](CodeForked/react-tooltip@f30ae74)) * do not delay show if tooltip is already shown ([ReactTooltip#676](https://github.com/CodeForked/react-tooltip/issues/676)) ([e8b9d84](CodeForked/react-tooltip@e8b9d84)) * **domexception:** revert previous changed for unexpected behavior ([85e38bb](CodeForked/react-tooltip@85e38bb)), closes [ReactTooltip#667](https://github.com/CodeForked/react-tooltip/issues/667) * effect and type not properly applied at first render ([a8d0e51](CodeForked/react-tooltip@a8d0e51)) * **event:** expose the original event to `afterShow` and `afterHide` ([e2f973e](CodeForked/react-tooltip@e2f973e)) * **example:** 'made dev' works again, small fixes. ([7b286bb](CodeForked/react-tooltip@7b286bb)), closes [ReactTooltip#328](https://github.com/CodeForked/react-tooltip/issues/328) [ReactTooltip#341](https://github.com/CodeForked/react-tooltip/issues/341) * **example:** <p> warning from react, make text match code. ([7c4c979](CodeForked/react-tooltip@7c4c979)) * **examples:** add SVG example ([72a98d7](CodeForked/react-tooltip@72a98d7)) * fix ie edge CustomEvent bug ([ReactTooltip#567](https://github.com/CodeForked/react-tooltip/issues/567)) ([b7f04f7](CodeForked/react-tooltip@b7f04f7)), closes [ReactTooltip#498](https://github.com/CodeForked/react-tooltip/issues/498) * **getPosition Util:** Remove shouldUpdatePlace check from position check ([1f8a054](CodeForked/react-tooltip@1f8a054)), closes [ReactTooltip#574](https://github.com/CodeForked/react-tooltip/issues/574) * **getposition:** properly determine parents with will-change: transform ([3a76250](CodeForked/react-tooltip@3a76250)) * **getPosition:** updated getPosition to fix 'maximum update depth' ([8fda305](CodeForked/react-tooltip@8fda305)) * **githubPage:** updating github page build in travis ([87b810a](CodeForked/react-tooltip@87b810a)) * **html:** remove sanitize-html-react, reduce package size ([177ac11](CodeForked/react-tooltip@177ac11)), closes [ReactTooltip#429](https://github.com/CodeForked/react-tooltip/issues/429) * **index.js:** add missing argument so tooltip hides. ([4d3661b](CodeForked/react-tooltip@4d3661b)) * **index.js:** fix exception when testing with Jest ([ReactTooltip#682](https://github.com/CodeForked/react-tooltip/issues/682)) ([f885f1f](CodeForked/react-tooltip@f885f1f)) * **index.js:** fix state initialization ([69dea07](CodeForked/react-tooltip@69dea07)) * **index.js:** Replaced the deprecated `componentWillReceiveProps`. ([80b71ed](CodeForked/react-tooltip@80b71ed)) * **index.js:** Use correct orientation when mouse enters ([4a0da8b](CodeForked/react-tooltip@4a0da8b)), closes [ReactTooltip#388](https://github.com/CodeForked/react-tooltip/issues/388) * install dependencies in example travis ([7ba8b28](CodeForked/react-tooltip@7ba8b28)) * **isCapture:** better guard that preserves logic ([28b8493](CodeForked/react-tooltip@28b8493)) * **isCapture:** guard use of currentTarget ([4f89a2d](CodeForked/react-tooltip@4f89a2d)) * **lint:** styles are now linted with stylelint ([3c17198](CodeForked/react-tooltip@3c17198)) * made it possible to pass uuid instead of generating one internally ([ReactTooltip#583](https://github.com/CodeForked/react-tooltip/issues/583)) ([083edfb](CodeForked/react-tooltip@083edfb)), closes [ReactTooltip#580](https://github.com/CodeForked/react-tooltip/issues/580) * mark prop-types and uuid as external to avoid bundling them ([ReactTooltip#582](https://github.com/CodeForked/react-tooltip/issues/582)) ([fb60855](CodeForked/react-tooltip@fb60855)) * modifying example ([9dc0b2e](CodeForked/react-tooltip@9dc0b2e)) * **no_var:** no vars allowed ([c591804](CodeForked/react-tooltip@c591804)) * **overridePosition:** providing currentEvent in overridePosition ([ReactTooltip#563](https://github.com/CodeForked/react-tooltip/issues/563)) ([3e75a09](CodeForked/react-tooltip@3e75a09)), closes [ReactTooltip#513](https://github.com/CodeForked/react-tooltip/issues/513) * performance issue caused by excessive use of clearTimeout/Interval ([22aea50](CodeForked/react-tooltip@22aea50)) * providing currentTarget in overridePosition ([ReactTooltip#564](https://github.com/CodeForked/react-tooltip/issues/564)) ([22c3bac](CodeForked/react-tooltip@22c3bac)) * **pr:** package.json fix; refactoring to exclude dependencies ([fdc17d4](CodeForked/react-tooltip@fdc17d4)) * release event listners ([ReactTooltip#534](https://github.com/CodeForked/react-tooltip/issues/534)) ([7cc1203](CodeForked/react-tooltip@7cc1203)) * **selector:** Add support for shadow DOM elements ([99be4d1](CodeForked/react-tooltip@99be4d1)) * **selector:** lint fixes ([873c2a8](CodeForked/react-tooltip@873c2a8)) * set aria-describedby value wrong when custom id ([a04d26c](CodeForked/react-tooltip@a04d26c)) * **showtooltip:** check if tooltipRef is undefined ([ReactTooltip#623](https://github.com/CodeForked/react-tooltip/issues/623)) ([f63eab2](CodeForked/react-tooltip@f63eab2)) * skip warning in example ([a555060](CodeForked/react-tooltip@a555060)) * **src/index.js:** add accessibility support for tabbing ([ReactTooltip#695](https://github.com/CodeForked/react-tooltip/issues/695)) ([ae936a5](CodeForked/react-tooltip@ae936a5)) * **src/index.js:** hide tooltip if blurred (tabbed out) ([ReactTooltip#699](https://github.com/CodeForked/react-tooltip/issues/699)) ([e0a2a1d](CodeForked/react-tooltip@e0a2a1d)) * **src/index.js:** Overwrite `delayHide` on scroll ([7a2d0b3](CodeForked/react-tooltip@7a2d0b3)), closes [ReactTooltip#474](https://github.com/CodeForked/react-tooltip/issues/474) * **staticMethods:** fixing IE event bug ([ReactTooltip#569](https://github.com/CodeForked/react-tooltip/issues/569)) ([9acc591](CodeForked/react-tooltip@9acc591)) * string into example ([356821b](CodeForked/react-tooltip@356821b)) * **style injection:** change style injection default root ([a00c5b7](CodeForked/react-tooltip@a00c5b7)), closes [ReactTooltip#665](https://github.com/CodeForked/react-tooltip/issues/665) * **styles:** add styles for shadow dom ([00d1539](CodeForked/react-tooltip@00d1539)), closes [ReactTooltip#597](https://github.com/CodeForked/react-tooltip/issues/597) * **styles:** change style injection way ([ReactTooltip#668](https://github.com/CodeForked/react-tooltip/issues/668)) ([1e10cce](CodeForked/react-tooltip@1e10cce)), closes [ReactTooltip#650](https://github.com/CodeForked/react-tooltip/issues/650) * **tooltip:** sanitize HTML to prevent XSS ([182df11](CodeForked/react-tooltip@182df11)) * **type:** added role property to types ([ReactTooltip#679](https://github.com/CodeForked/react-tooltip/issues/679)) ([9b49395](CodeForked/react-tooltip@9b49395)) * **type:** Fix global method parameter type ([ReactTooltip#585](https://github.com/CodeForked/react-tooltip/issues/585)) ([5e2b8db](CodeForked/react-tooltip@5e2b8db)) * **types:** adding types filename to package ([ReactTooltip#579](https://github.com/CodeForked/react-tooltip/issues/579)) ([05d8de2](CodeForked/react-tooltip@05d8de2)) * **types:** adding typescript d.ts file into dist ([e6300f7](CodeForked/react-tooltip@e6300f7)), closes [ReactTooltip#579](https://github.com/CodeForked/react-tooltip/issues/579) * update uuid module ([d937e59](CodeForked/react-tooltip@d937e59)) * updating styles using transferSas ([f2f7804](CodeForked/react-tooltip@f2f7804)) * using css into example ([7d343af](CodeForked/react-tooltip@7d343af)) * using sass styles with rollup ([bb6fe48](CodeForked/react-tooltip@bb6fe48)) * **uuid:** Use uuid package for unique class names ([ReactTooltip#566](https://github.com/CodeForked/react-tooltip/issues/566)) ([c2c2243](CodeForked/react-tooltip@c2c2243)) * validate lint in pretest ([ad7add0](CodeForked/react-tooltip@ad7add0)) * Merge pull request ReactTooltip#550 from wwayne/refactoring ([4609833](CodeForked/react-tooltip@4609833)), closes [ReactTooltip#550](https://github.com/CodeForked/react-tooltip/issues/550) ### Features * adding typescript type defs ([ReactTooltip#571](https://github.com/CodeForked/react-tooltip/issues/571)) ([cb2b921](CodeForked/react-tooltip@cb2b921)) * **clickable-prop:** add clickable prop ([a75b2be](CodeForked/react-tooltip@a75b2be)), closes [ReactTooltip#417](https://github.com/CodeForked/react-tooltip/issues/417) * **component:** adding "padding" property to customize padding style ([9ae765a](CodeForked/react-tooltip@9ae765a)) * convert to typescript ([dc547c1](CodeForked/react-tooltip@dc547c1)) * getContent(dataTip) ([8bfbfc9](CodeForked/react-tooltip@8bfbfc9)) * **getContent:** update Travis, trigger semantic-release ([9617267](CodeForked/react-tooltip@9617267)) * **overridePosition:** Add "overridePosition" property to handle border cases and customize position ([ccb8b58](CodeForked/react-tooltip@ccb8b58)) * **overridePosition:** Added example ([7df8454](CodeForked/react-tooltip@7df8454)) * Small bug fix to previous commit ([19a8a67](CodeForked/react-tooltip@19a8a67)) * The only way to support styling react-tooltips with a strict csp is to inject the style.css di ([cf105a1](CodeForked/react-tooltip@cf105a1)), closes [ReactTooltip#340](https://github.com/CodeForked/react-tooltip/issues/340) * **tooltip:** Add ability to hover on tooltip. Provide optional delay of updating so if the mouse p ([79342ce](CodeForked/react-tooltip@79342ce)) ### Performance Improvements * **Use sanitize-html-react instead of sanitize-html to avoid useless server side dependencies:** Us ([4b84caa](CodeForked/react-tooltip@4b84caa)), closes [ReactTooltip#424](https://github.com/CodeForked/react-tooltip/issues/424) ### BREAKING CHANGES * Updating readme for demo
Hi, first of all many thanks for this package making life easier for us!
I noticed that our total bundle size suddenly increased by 50%, and after some detective work I pinpointed the issue to the html-sanitizer package added here: #422
This addition made react-tooltip 7(!) times bigger in size and unfortunately that is a dealbreaker for us. Now this was improved a bit by replacing it with sanitize-html-react (#426) but it's still very big for a tooltip component (for our usecase).
I found this tool which gives a graph of the size changes, I don't know the accuracy but it should give an idea at least: https://bundlephobia.com/result?p=react-tooltip@3.8.0
I understand that this is for security purposes, but is there a way to safely opt out from this massive package, we don't really need to let users write the actual html, but we do use some markup where we insert strings from users.
Sorry for potentially stupid question since I don't know the code very well, but is there a way to avoid dangerousslySetInnerHtml and simply render children instead? If that removes the need for sanitizing.
Thanks
The text was updated successfully, but these errors were encountered: