Enhance security: code fixes and dependency upgrades #134
Conversation
There was a problem hiding this comment.
+1 on the updates, but I'd recommend importing sanitize specifically, because it sometimes makes it easier for security sniffers to detect that sanitization is in use.
However — where is DOMPurify coming from? It's not a dependency of this plugin and not listed in package.json. I suspect it's being provided by another plugin, and we're getting lucky that the global is present -- but we should include it in our build, for safety. It won't increase bundle size much and it'll make this plugin independent from others.
| @@ -14,7 +15,7 @@ const PostListItem = ( { post, author, thumbnail, postTypeObject, isSelected, on | |||
| <label htmlFor={ `select-post-${post.id}` }> | |||
| { thumbnail | |||
| ? <img | |||
| alt={ post.title.rendered } | |||
| alt={ DOMPurify.sanitize( post.title.rendered ) } | |||
There was a problem hiding this comment.
Nitpick, better to import { sanitize } from DOMPurify; and then call these as alt={ sanitize( post.title.rendered ) }
(Although, I'm confused why we need to do this -- React should already be escaping the alt attribute. The main security issue with React is when you set __dangerouslySetInnerHtml. Are we certain we need to double-sanitize alt strings?)
There was a problem hiding this comment.
@kadamwhite thanks for the review.
I applied the solution you suggested on both, but could not yet test it, as I'm setting up a local environment to use the plugin.
I got this solution of sanitizing the variable and using DOMPurify from the security audit:
https://semgrep.dev/r?q=typescript.react.security.audit.react-dangerouslysetinnerhtml.react-dangerouslysetinnerhtml
There was a problem hiding this comment.
We don't have linting actions in place, and I noticed that eslint is complaining about sanitize:
sanitize not found in 'dompurify' eslint (import/named)
I'm looking into that. Thanks.
There was a problem hiding this comment.
@MiguelAxcar I think that the issue you're seeing is related to my comment,
where is
DOMPurifycoming from? It's not a dependency of this plugin and not listed inpackage.json
We can't use this module in the bundled code unless we can guarantee it exists. I was suggesting we add dompurify as a package dependency, which should allow the named imports sniff to detect it, and also fix a build error:
Build errors that turned out not to be caused by a missing dependency
Child editor:
Time: 7313ms
3 assets
WARNING in ./js/post-select/containers/term-select-form-field.js
Module Warning (from ./node_modules/eslint-loader/dist/cjs.js):
Cannot read property 'range' of null
Occurred while linting /Users/kadam/hm/gh/hm-gutenberg-tools/js/post-select/containers/term-select-form-field.js:3
ERROR in ./js/post-select/components/post-list-item.js
Module Error (from ./node_modules/eslint-loader/dist/cjs.js):
/Users/kadam/hm/gh/hm-gutenberg-tools/js/post-select/components/post-list-item.js
2:10 error sanitize not found in 'dompurify' import/named
✖ 1 problem (1 error, 0 warnings)
ERROR in ./js/post-select/components/selection-item.js
Module Error (from ./node_modules/eslint-loader/dist/cjs.js):
/Users/kadam/hm/gh/hm-gutenberg-tools/js/post-select/components/selection-item.js
2:10 error sanitize not found in 'dompurify' import/named
✖ 1 problem (1 error, 0 warnings)
Please add dompurify to the project using npm.
Edit: The package has been there all along, and the syntax error was the result of my suggestion to destructure sanitize. Interestingly, neither import DOMPurify from 'dompurify' nor import { sanitize } from DOMPurify are the recommended way to consume the package as a ES module, but the * as DOMPurify method doesn't work locally, and using it as the default export does.
| "webpack-cli": "^3.3.12", | ||
| "webpack-dev-server": "^3.11.0" | ||
| }, | ||
| "dependencies": { | ||
| "dompurify": "^3.0.6", |
There was a problem hiding this comment.
I'm sorry, @MiguelAxcar , I don't know how I missed that you had already installed this 🤦 🤔
I also now understand that the errors we're seeing are because I steered you wrong: DOMPurify wants to be imported like this,
import * as DOMPurify from 'dompurify';so then we would call it as DOMPurify.sanitize() in the way you did before. But, that doesn't actually work for me locally, probably because of the older Node environment; so, we should actually go back to what you had in the first place, import DOMPurify from 'dompurify';. 😬
Best way to get things working is probably to revert 7d95e8c and then everything should work.
Apologies for the off-target guidance!
This reverts commit 7d95e8c.
This PR intends to add security enhancements listed on Security Review Summary - T335004 - 2023-09-26 (internal client ticket #922)
Changes added
post.title.renderedto avoid XSS attacksnpm audit fixVulnerable packages
SAST Audit findings
js/post-select/components/post-list-item.jsdangerouslySetInnerHTMLwithpost.title.rendered- Potential XSS risk (Details)js/post-select/components/selection-item.jsdangerouslySetInnerHTMLwithpost.title.rendered- Potential XSS risk (Details)