-
Notifications
You must be signed in to change notification settings - Fork 39
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
Generate hashes for external scripts and styles #87
Conversation
On beforeAssetTagGeneration, we collect a list of files known to Webpack that we might need to include hashes of in the CSP. Later, in beforeEmit, we check which of those files ended up in the generated HTML, and calculate the hashes of those files to include in the CSP.
Each test generates an index.bundle.js; the hash of this is now included in the CSP.
This adds css-loader and mini-css-extract-plugin in order to test hash generation for CSS files.
Codecov Report
@@ Coverage Diff @@
## master #87 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 108 154 +46
Branches 19 28 +9
=========================================
+ Hits 108 154 +46
Continue to review full report at Codecov.
|
Thanks for this contribution, and sorry for the delay in getting back to you! I plan on reviewing this over the next week, and will post comments in the PR :) |
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the submission! Again, huge apologies for the delay in reviewing this.
I've added a few comments inline for changes that need to be made.
On top of this, it looks like we still need to add the integrity
attribute to all script and style tags that have a hash generated for them. Preference for this plugin is for it to work as a standalone plugin, without the need for interacting with another plugin such as the subresource-integrity one that you've linked. Without this, it may provide a false sense of security to devs who only use this plugin and see that hashes are being generated for external scripts, but the integrity attributes are not being added to the script tags.
For the bullets you've asked for feedback on:
- Yes, this is correct.
style-loader
injects css at runtime, and is something we cannot generate a hash for. In fact, we shouldn't generate a hash for this, as it is not something we can precompute at compile time - The additions look good to me - I've added a few inline comments there for you
- This looks ok to me - it looks like webpack is doing the same here - simply concatenating
publicPath
with thefileName
.
NB: This link in general might also help us figure out how to add the integrity
attribute to script tags, or webpack might even do it for us in some specific use case, which we can leverage
- I think this is OK. We might want to have a setting to decide on whether to hash these external script and style tags, but this is something we can add at a later date, provided we can solve the
integrity
attribute challenge
" style-src 'unsafe-inline' 'self' 'unsafe-eval' 'sha256-MqG77yUiqBo4MMVZAl09WSafnQY4Uu3cSdZPKxaf9sQ=' 'nonce-mockedbase64string-3'"; | ||
|
||
expect(csps['index.html']).toEqual(expected); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('inserts hashes for linked scripts and styles from the same Webpack build', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add another test which checks that external scripts that are not generated by this webpack compilation don't have a hash added to the csp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't 100% on what you meant by this – as in external scripts linked directly from the HTML (which I think should be covered by this test, since it uses external-scripts-styles.html
)? or as in multiple webpack runs or something?
" style-src 'unsafe-inline' 'self' 'unsafe-eval' 'sha256-MqG77yUiqBo4MMVZAl09WSafnQY4Uu3cSdZPKxaf9sQ=' 'nonce-mockedbase64string-3'"; | ||
|
||
expect(csps['index.html']).toEqual(expected); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('inserts hashes for linked scripts and styles from the same Webpack build', (done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add another test which checks that external scripts that are not generated by this webpack compilation don't have a hash added to the csp?
Could we add another test that ensures that if a single webpack compilation generates multiple (2 will do) instances of HtmlWebpackPlugin, each with different assets in them, the correct hashes are only added to the correct meta tags?
e.g.:
- webpack entry point
index-1.js
.- Generates HTML:
index-1.html
that has an external linkindex-1.bundle.js
and a csp hash forindex-1.bundle.js
- Generates HTML:
- webpack entry point
index-2.js
.- Generates HTML:
index-2.html
that has an external linkindex-2.bundle.js
and a csp hash forindex-2.bundle.js
- Generates HTML:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added: https://github.com/slackhq/csp-html-webpack-plugin/pull/87/files#diff-fc33293bee015873cf89f2cff90af742b95c8a5117d0ee47f75f8ba2dcd4579bR189 (it('only inserts hashes for linked scripts and styles from the same HtmlWebpackPlugin instance'
)
Thanks for the review! It might be a while before I get time to update this PR, sorry – I have a bunch of university deadlines in the next few weeks. |
still lots of uni work, but here are some changes :) (argh, now I remember I forgot to run eslint, let me fix those problems now...) |
cb95347
to
bbc072a
Compare
Any movement on this? @AnujRNair would this be mergable if the requested changes were made? @melloware I know you were also interested. |
I am 100% interested in this. In fact so interested I am considering forking this lib and applying this patch and releasing it to NPM. I don't want to but if I have no other choice I might do it. |
it's been a while since I wrote this, but as far as I remember, I addressed all the feedback to the best of my ability. if more changes are needed, I'd prefer to avoid needing to test them myself, if possible – I no longer have a development environment set up for the project I was testing this on. (I'd be entirely happy for someone else to take the existing work and get it into a mergeable state.) |
OK I forked this repo and published it to NPM including these changes. GitHub: https://github.com/melloware/csp-webpack-plugin NPM: https://www.npmjs.com/package/@melloware/csp-webpack-plugin @slapbox I would much appreciate you giving it a try. I am seeing one weird issue where my JS code integrity hash is not the same as Google is Calculating. I am using Create React App so I have a feeling the JS is being changed somehow AFTER the plugin has calculated the integrity hash. Edit: I fixed the integrity issues by switching internally to this plugin: https://www.npmjs.com/package/webpack-subresource-integrity Everything seems to be working great for my projects now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should find another way of manipulating URLs instead of using path
// get all the shas for scripts and styles generated and linked to by this HtmlWebpackPlugin instance | ||
const linkedScriptShas = this.scriptFilesToHash | ||
.filter((filename) => | ||
includedScripts.includes(path.join(this.publicPath, filename)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path.join
is eating up the double slash in the protocol if the publicPath
is an absolute URL like https://...
. I think It is not safe to use path utils for manipulating URLs in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What can publicPath
be besides an absolute URL like http://....
? Do we need to accommodate non-URL filepaths? Is there a use case for that in production environments?
this.publicPath = htmlPluginData.assets.publicPath; | ||
if (this.hashEnabled['script-src'] !== false) { | ||
this.scriptFilesToHash = htmlPluginData.assets.js.map((filename) => | ||
path.relative(this.publicPath, filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, using path utils breaks absolute URLs here as well.
I have also found another issue with this change. |
@anacierdem you can try my custom plugin if you want this is all working properly. https://www.npmjs.com/package/@melloware/csp-webpack-plugin |
We have already developed an alternative fork also fixing Firefox CSP-3 issues, which is mostly specific to our use case at the moment. Thank you for the heads up! |
Can you shed some light on your Firefox CSP issues? |
Basically it behaves like it is unable to calculate the same hash for scripts with |
@anacierdem that is weird I am using my version of the plugin and Firefox is working fine with the EDIT: Nevermind I use NONCE's for scripts and Hashes for Integrity so it works fine. Its only a bug if you use HASH's and not NONCE's. |
I don't have the bandwidth or the desire to make the suggested changes. Anyone should feel free to modify this PR and resubmit it, the code is MIT-licensed. |
Summary
CSP 3.0 permits hashes in the CSP to match external scripts. This PR generates those hashes for scripts that Webpack knows about (option 2 in #50 (comment)).
NB: as I understand it, it's required that external scripts have an
integrity
attribute in order to be permitted via CSP hash, so this is only useful in conjunction with something like webpack-subresource-integrity.Fixes #50 (see also #35).
Things I'd particularly like feedback on:
style-loader
injects CSS at runtime; that seems like it would cause issues unless'self'
is also present.createWebpackConfig
to allow customising the Webpack config a bit more – is there a better way of structuring this? (mini-css-extract-plugin complains if the loader is configured but the plugin isn't, so I don't think it's possible to just unconditionally include the CSS rule for every test, unlesscreateWebpackConfig
also just adds mini-css-extract-plugin toplugins
)processCsp
/getFilesToHash
correct? I don't know if it will work properly with non-defaultpublicPath
.index.bundle.js
in the tests) likely to break anything? Should this be behind a config flag?Requirements (place an
x
in each[ ]
)