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

Configure nonce also for preloaded scripts #85

Closed
wants to merge 2 commits into from

Conversation

jukben
Copy link

@jukben jukben commented Feb 10, 2021

Summary

Solving #84

Does it make sense for you? Should I write test for this it? 👍

Requirements (place an x in each [ ])

@CLAassistant
Copy link

CLAassistant commented Feb 10, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #85 (1d65071) into master (9b670ad) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 1d65071 differs from pull request most recent head 6781a60. Consider uploading reports for the commit 6781a60 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master       #85   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          108       108           
  Branches        19        19           
=========================================
  Hits           108       108           
Impacted Files Coverage Δ
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 c4c9771...6781a60. Read the comment docs.

@AnujRNair
Copy link
Contributor

Thanks for the contribution!

It looks like you can also use preload links to load web workers, and these should be added to the CSP via the child-src directive. At the moment, this change will always add the nonce to the script-src directive.

I think we could solve for this in a few ways:

  • Add an additional option to enable generation for preload links, and add them to the script-src. However, this won't work for applications that want to preload both js files and workers
  • Ask developers to add a data tag to links that they want to preload and add to the script-src. We could then target it with link[rel="preload"][as="script"][data-csp="script-src"]. This would then be extendible to workers in the future, and allow us to add [data-csp="child-src"] to generate those nonces too.

I'm leaning towards the second given it would result in a correct csp implementation, and is flexible for the future. Do you agree?

If so, could you make the change and add tests?

Thanks!

@jukben
Copy link
Author

jukben commented Jun 24, 2021

Hey @AnujRNair sorry for getting back so late I somehow missed the notification 🤦 .

So if I understand correctly you propose to document possibility to add data-csp="script-src" tag to links (for now). Thus the usage would be something like (in my case #84 see the original issue)

<link rel="preload" href="..." as="script" crossorigin="anonymous" data-csp="script-src">

In the future we could introduce that data-csp="child-src" and reiterate on it, but that's something we should wait once the real need is there, do I understand it correctly? If so, I agree, let's descope the complexity now 👍

If we are on the same page, I will wrap it up with test and we are ready to go 🚀

EDIT: On the other hand if it's just about adding link[rel="preload"][as="script"][data-csp="child-src"] to child-src we can do that within this PR I guess. Oh it looks we don't operate with child-src in the code yet so I would indeed descope it form this as possible followup which can be prioritized once there's need for it (It might be from us as well 😆). 👍

@AnujRNair
Copy link
Contributor

Sorry for the delay - yes this is what I was thinking!
If you wouldn't mind adding this in with test, that would be great
Thanks

You can generate nonce also for preloaded scripts by using data-csp
attribute
@jukben
Copy link
Author

jukben commented Sep 17, 2021

Hey @AnujRNair I think this should do the trick 🚀

@melloware
Copy link

@jukben OK I forked this repo and published it to NPM including this change.

GitHub: https://github.com/melloware/csp-webpack-plugin

NPM: https://www.npmjs.com/package/@melloware/csp-webpack-plugin

@jukben jukben closed this Mar 15, 2022
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.

4 participants