Generate hashes for external scripts and styles #87

sersorrel · 2021-03-17T02:17:14Z

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:

Currently, only hashes for things that end up in the generated HTML at build-time are added to the CSP. Is that actually correct? Webpack's style-loader injects CSS at runtime; that seems like it would cause issues unless 'self' is also present.
I've hacked on 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, unless createWebpackConfig also just adds mini-css-extract-plugin to plugins)
Is the manipulating of filenames I'm doing in processCsp/getFilesToHash correct? I don't know if it will work properly with non-default publicPath.
Is the new hash produced by default (e.g. for index.bundle.js in the tests) likely to break anything? Should this be behind a config flag?

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've written tests to cover the new code and functionality included in this PR.
I've read, agree to, and signed the Contributor License Agreement (CLA).

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.

CLAassistant · 2021-03-17T02:17:21Z

All committers have signed the CLA.

codecov · 2021-03-17T02:18:39Z

Codecov Report

Merging #87 (bbc072a) into master (d4cf1dd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #87   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          108       154   +46     
  Branches        19        28    +9     
=========================================
+ Hits           108       154   +46

Impacted Files	Coverage Δ
test-utils/webpack-helpers.js	`100.00% <ø> (ø)`
plugin.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4cf1dd...bbc072a. Read the comment docs.

AnujRNair · 2021-04-08T22:02:25Z

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 :)

sschaefer-rms

Looks good to me!

AnujRNair

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 the fileName.

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

AnujRNair · 2021-04-20T19:06:23Z

plugin.jest.js

          " 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) => {


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?

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?

plugin.js

AnujRNair · 2021-04-20T19:19:49Z

plugin.jest.js

          " 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) => {


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 link index-1.bundle.js and a csp hash for index-1.bundle.js

webpack entry point index-2.js.

Generates HTML: index-2.html that has an external link index-2.bundle.js and a csp hash for index-2.bundle.js

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')

test-utils/webpack-helpers.js

sersorrel · 2021-04-24T01:03:12Z

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.

sersorrel · 2021-05-07T22:01:50Z

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...)

Nantris · 2021-12-23T21:37:33Z

Any movement on this?

@AnujRNair would this be mergable if the requested changes were made?

@melloware I know you were also interested.

melloware · 2021-12-23T22:27:48Z

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.

sersorrel · 2021-12-25T02:39:35Z

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.)

melloware · 2021-12-30T14:58:06Z

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!

anacierdem

We should find another way of manipulating URLs instead of using path

anacierdem · 2022-01-11T07:21:05Z

plugin.js

+    // 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))


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.

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?

anacierdem · 2022-01-11T07:21:33Z

plugin.js

+    this.publicPath = htmlPluginData.assets.publicPath;
+    if (this.hashEnabled['script-src'] !== false) {
+      this.scriptFilesToHash = htmlPluginData.assets.js.map((filename) =>
+        path.relative(this.publicPath, filename)


Similarly, using path utils breaks absolute URLs here as well.

anacierdem · 2022-01-12T08:31:30Z

I have also found another issue with this change. HtmlWebpackPlugin is using PROCESS_ASSETS_STAGE_OPTIMIZE_INLINE processAssets stage which runs before PROCESS_ASSETS_STAGE_OPTIMIZE_HASH of the RealContentHashPlugin which is enabled by default on production builds. It effectively replaces hashes in content after the csp hashes are generated, making them stale. So this will not work correctly on production builds.

melloware · 2022-01-12T12:57:38Z

@anacierdem you can try my custom plugin if you want this is all working properly. https://www.npmjs.com/package/@melloware/csp-webpack-plugin

anacierdem · 2022-01-12T13:30:23Z

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!

melloware · 2022-01-12T13:36:52Z

Can you shed some light on your Firefox CSP issues?

anacierdem · 2022-01-13T08:17:07Z

Basically it behaves like it is unable to calculate the same hash for scripts with src and integrity when strict-dynamic is enabled for script-src. See the long running Firefox bug here. Inline scripts without an integrity work fine so we are using an inline loader script to load external resources as a workaround until it is fixed.

melloware · 2022-01-13T13:03:20Z

Basically it behaves like it is unable to calculate the same hash for scripts with src and integrity when strict-dynamic is enabled for script-src. See the long running Firefox bug here. Inline scripts without an integrity work fine so we are using an inline loader script to load external resources as a workaround until it is fixed.

@anacierdem that is weird I am using my version of the plugin and Firefox is working fine with the integrity hashes and strict-dynamic??? I am using Firefox 96.0 (64 bit). ???

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.

sersorrel · 2022-01-17T00:35:53Z

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.

sersorrel added 3 commits March 17, 2021 00:26

Update existing tests to include bundle hash

591276c

Each test generates an index.bundle.js; the hash of this is now included in the CSP.

Add test for external script/style hash generation

ee19fd5

This adds css-loader and mini-css-extract-plugin in order to test hash generation for CSS files.

sschaefer-rms previously approved these changes Apr 16, 2021

View reviewed changes

AnujRNair suggested changes Apr 20, 2021

View reviewed changes

AnujRNair mentioned this pull request Apr 20, 2021

[Security] Nonce reuse #82

Open

9 tasks

Nantris mentioned this pull request Apr 21, 2021

CSP improvement opportunities reZach/secure-electron-template#53

Open

sersorrel added 3 commits May 7, 2021 14:18

Add comments per review

df3f1c9

Add test for multiple HtmlWebpackPlugin instances per review

8bb9860

Add integrity attribute when generating hashes for linked scripts/styles

ccce97b

sersorrel dismissed sschaefer-rms’s stale review via ccce97b May 7, 2021 21:54

sersorrel added 2 commits May 7, 2021 23:17

Fix ESLint errors

597e4d1

Do not add integrity attributes if the entire plugin is disabled

bbc072a

sersorrel force-pushed the hash-external-scripts branch from cb95347 to bbc072a Compare May 11, 2021 20:37

melloware mentioned this pull request Dec 23, 2021

Add hashes to external scripts (CSP3) #50

Open

9 tasks

anacierdem suggested changes Jan 11, 2022

View reviewed changes

sersorrel closed this Jan 17, 2022

unverbraucht mentioned this pull request Oct 17, 2023

Port over external CSP3 hash support melloware/csp-webpack-plugin#137

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate hashes for external scripts and styles #87

Generate hashes for external scripts and styles #87

sersorrel commented Mar 17, 2021

CLAassistant commented Mar 17, 2021 •

edited

codecov bot commented Mar 17, 2021 •

edited

AnujRNair commented Apr 8, 2021

sschaefer-rms left a comment

AnujRNair left a comment

AnujRNair Apr 20, 2021

sersorrel May 7, 2021

AnujRNair Apr 20, 2021

sersorrel May 7, 2021

sersorrel commented Apr 24, 2021

sersorrel commented May 7, 2021

Nantris commented Dec 23, 2021 •

edited

melloware commented Dec 23, 2021

sersorrel commented Dec 25, 2021

melloware commented Dec 30, 2021 •

edited

anacierdem left a comment

anacierdem Jan 11, 2022

Nantris Jan 17, 2022

anacierdem Jan 11, 2022

anacierdem commented Jan 12, 2022 •

edited

melloware commented Jan 12, 2022

anacierdem commented Jan 12, 2022

melloware commented Jan 12, 2022

anacierdem commented Jan 13, 2022 •

edited

melloware commented Jan 13, 2022 •

edited

sersorrel commented Jan 17, 2022

Generate hashes for external scripts and styles #87

Generate hashes for external scripts and styles #87

Conversation

sersorrel commented Mar 17, 2021

Summary

Requirements (place an x in each [ ])

CLAassistant commented Mar 17, 2021 • edited

codecov bot commented Mar 17, 2021 • edited

Codecov Report

AnujRNair commented Apr 8, 2021

sschaefer-rms left a comment

Choose a reason for hiding this comment

AnujRNair left a comment

Choose a reason for hiding this comment

AnujRNair Apr 20, 2021

Choose a reason for hiding this comment

sersorrel May 7, 2021

Choose a reason for hiding this comment

AnujRNair Apr 20, 2021

Choose a reason for hiding this comment

sersorrel May 7, 2021

Choose a reason for hiding this comment

sersorrel commented Apr 24, 2021

sersorrel commented May 7, 2021

Nantris commented Dec 23, 2021 • edited

melloware commented Dec 23, 2021

sersorrel commented Dec 25, 2021

melloware commented Dec 30, 2021 • edited

anacierdem left a comment

Choose a reason for hiding this comment

anacierdem Jan 11, 2022

Choose a reason for hiding this comment

Nantris Jan 17, 2022

Choose a reason for hiding this comment

anacierdem Jan 11, 2022

Choose a reason for hiding this comment

anacierdem commented Jan 12, 2022 • edited

melloware commented Jan 12, 2022

anacierdem commented Jan 12, 2022

melloware commented Jan 12, 2022

anacierdem commented Jan 13, 2022 • edited

melloware commented Jan 13, 2022 • edited

sersorrel commented Jan 17, 2022

Requirements (place an `x` in each `[ ]`)

CLAassistant commented Mar 17, 2021 •

edited

codecov bot commented Mar 17, 2021 •

edited

Nantris commented Dec 23, 2021 •

edited

melloware commented Dec 30, 2021 •

edited

anacierdem commented Jan 12, 2022 •

edited

anacierdem commented Jan 13, 2022 •

edited

melloware commented Jan 13, 2022 •

edited