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

feat(addStyles): add support for __webpack_nonce__ #319

Merged

Conversation

iamdavidfrancis
Copy link
Contributor

What kind of change does this PR introduce?
Feature to support __webpack_nonce__

Did you add tests for your changes?
Yes, I added a test that verifies the nonce gets set on style tags. (Nonces will not help link tags as nonces are only used to allow inline styles.)

If relevant, did you update the README?
I wasn't sure where in the README I should put a note about supporting __webpack_nonce__, so I have omitted it for now. I can add it in if requested.

Summary
Currently if a site has a strict CSP that disallows unsafe-inline the rendered style tags from this loader will be ignored. Webpack supports adding a nonce via the __webpack_nonce__ property. This change looks for that property and adds the nonce attribute to generated style tags.

More info here: #306

Does this PR introduce a breaking change?
This is only breaking if the package consumer was expecting the styles not to load, so it should not be an issue. If the consumer was already adding a nonce attribute via the options object, then that will take precedence over the __webpack_nonce__ property, so it shouldn't break anyone doing it that way.

@jsf-clabot
Copy link

jsf-clabot commented Apr 23, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 23, 2018

Codecov Report

Merging #319 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #319   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files           4        4           
  Lines          64       64           
  Branches       21       21           
=======================================
  Hits           63       63           
  Misses          1        1

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 e4fa68a...54eb35f. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! Should we add nonce by default, maybe better option for this?

@alexander-akait
Copy link
Member

Also i think we should add this into https://github.com/webpack-contrib/style-loader/blob/master/lib/addStyleUrl.js

@iamdavidfrancis
Copy link
Contributor Author

Adding it to addStyleUrl.js won't actually do anything though, right? It adds a link tag, which doesn't need a nonce. link tags are going to be controlled by what resources are allowed in the CSP.
See here for more info: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src

@alexander-akait
Copy link
Member

alexander-akait commented Apr 24, 2018

@iamdavidfrancis yep, you are right

/cc @webpack-contrib/org-maintainers should we add nonce by default without options?

@michael-ciniawsky michael-ciniawsky changed the title Add support for __webpack_nonce__ feat(addStyles): add support for __webpack_nonce__ May 5, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As already mentioned, maybe better as an option (options.nonce), but on the other hand doesn't seem to hurt to add it be default. Not sure about it...

@michael-ciniawsky michael-ciniawsky added this to the 0.21.0 milestone May 5, 2018
@alexander-akait
Copy link
Member

alexander-akait commented May 5, 2018

@michael-ciniawsky let's leave this as is, nonce is part of attribute tag

@makeable
Copy link

makeable commented Jun 1, 2018

Looking forward to this. It seems the previous workarounds of reusing nonced tags is no longer possible, so this is now the only legitimate fix for using style-loader with strict-dynamic CSP.

@alexander-akait alexander-akait merged commit fc24512 into webpack-contrib:master Jun 1, 2018
@tobias-zucali
Copy link
Contributor

tobias-zucali commented Jun 7, 2018

Thanks for your work, I really need that!

I use webpack to load my app inside an iframe.
It works great for me as long as I use the chrome csp tester plugin, but as soon as I deliver the csp header from the server it fails.
Strangely __webpack_nonce__ cannot be accessed as a global, it needs to be accessed via the window object.
I modified the following function of addStyles.js, it should never reach the log message as getNonce() should return window.__webpack_nonce__ but in my case it logs the message.

Does the Content Security Policy restrict access to global scope? Any ideas?

function getNonce() {
	if (typeof __webpack_nonce__ === 'undefined') {
		return null;
	}

	var nonce = __webpack_nonce__;

	if (!nonce && window.__webpack_nonce__) {
		// This part should never be executed but in my case the message is logged 🤔
		console.log('`__webpack_nonce__` did not return anything but `window.__webpack_nonce__` is set!')
		nonce = window.__webpack_nonce__;
	}

        return nonce
}

@tobias-zucali
Copy link
Contributor

We spent more time on this issue and it works now without changing style-loader.

It is all about how to set __webpack_nonce__ . It is not enough to set window.__webpack_nonce__ .

This way it works:

webpack.config.js:

module.exports = {
  entry: {
    main: 'my/entry/index.js'
  }
}

my/entry/index.js

__webpack_nonce__ = /* getting the nonce from somewhere */
// will compile to `__webpack_require__.nc = /* getting the nonce from somewhere */`

// you must not use import here as it is hoisted and executed first (https://github.com/webpack/webpack/issues/2776)
require('./theRealEntry.js')

./theRealEntry.js

// `__webpack_nonce__` (compiled to `__webpack_require__.nc`is available in the style loader now
import('./styles.css')

// …

@tobias-zucali
Copy link
Contributor

tobias-zucali commented Jul 5, 2018

...and it does not work at all if the HotModuleReplacementPlugin is active as different scopes are used, at least for Webpack 3.11.0. Any ideas if they changed this in version 4?

@plondon
Copy link

plondon commented Jul 23, 2018

@tobias-zucali thank you so much for your comment on HMR. That was the issue I was having. Im using webpack 4 and its still an issue.

@alexander-akait
Copy link
Member

Somebody can create new issue and minimum reproducible test repo?

@plondon
Copy link

plondon commented Jul 24, 2018

@evilebottnawi I think its an issue with HMR not style-loader (or any other loader/project) that uses webpack_nonce. For example, I had the same issue with styled-components until I turned of HMR

@alexander-akait
Copy link
Member

Will be great ensure this, need minimum reproducible test repo

@plondon
Copy link

plondon commented Sep 25, 2018

Nothing stands out in the webpack config.

Maybe the issue is you are not importing the nonce but setting it in App.js (or something similar). See comment here: styled-components/styled-components#887 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants