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

@bwhitty
Copy link

bwhitty commented Sep 25, 2018

And you shouldn't need to do this:
Absolutely agreed, but the first video shows that the same thing @tobias-zucali was encountering was happening to me.

Full client config (we use two webpack setups, one for the server as well):

/* eslint-disable import/no-extraneous-dependencies */
import path from 'path';
import webpack from 'webpack';
import ExtractTextPlugin from 'extract-text-webpack-plugin';
import HtmlWebpackPlugin from 'html-webpack-plugin';
import CleanWebpackPlugin from 'clean-webpack-plugin';
import { ReactLoadablePlugin } from 'react-loadable/webpack';
import { ROUTES } from 'app/constants/routes';
import * as RENDERING from 'app/constants/rendering';
import {
  buildPath,
  codeSplittingManifestFilename,
  distPath,
  inlineSizeLimitInBytes,
  projectRootPath,
  staticAssetTestRegex,
} from 'webpack/common';

export const baseClientConfig = {
  // makes sure that project root is always the contextual base path for all file path resolution
  context: projectRootPath,

  entry: {
    app: [
      // the actual entry point for the client app bundle
      './app/client/index.jsx',
    ],

    style: [
      './public/css/main.scss',
    ],
  },

  output: {
    // automagically generated by webpack, with chunkhash being the long-term cache id generated by
    // the CommonsChunkPlugin
    // https://webpack.js.org/plugins/commons-chunk-plugin/
    filename: '[name].[chunkhash].bundle.js',

    // where to output the actual files
    path: distPath,

    // path will be prepended when a given asset is referenced, such as within the index.ejs tpl
    publicPath: `${ROUTES.STATIC}/`,
  },

  resolve: {
    // make sure we can load .jsx and the likes without provides file extension in import statement
    extensions: ['.js', '.jsx', '.json', '.scss', '.css'],
  },

  module: {
    rules: [
      {
        test: /\.jsx?$/,
        exclude: (/node_modules/),
        loader: 'babel-loader',
        options: {
          // necessary so that webpack does not automatically use .babelrc
          babelrc: false,
          plugins: [
            // allow { ...someObj }
            'transform-object-rest-spread',
            // allow absolute import paths
            ['module-resolver', {
              root: ['.'],
            }],
            // allow jsx
            'transform-react-jsx',
            // allow import()
            'syntax-dynamic-import',
            // automagically configure react-loadable components so they can be picked up on
            // the client properly
            // https://github.com/thejameskyle/react-loadable#picking-up-a-server-side-rendered-app-on-the-client
            'react-loadable/babel',
          ],
          presets: [
            [
              // dynamically configures Babel based on real-world browser usage statistics
              'env', {
                targets: {
                  browsers: ['> 1%'],
                },
                useBuiltIns: true,
                // webpack handles this
                modules: false,
                exclude: [
                  // we're going to target all browsers >1% usage, but explicitly screw IE11 as
                  // it doesn't support generators. if we wanted to support IE11, we'd need
                  // to include the regenerator-runtime which is still licensed weirdly
                  // by Facebook so, for now, screw IE.
                  'transform-regenerator',
                ],
              },
            ],
          ],
        },
      },
      {
        test: /\.css$/,
        use: [
          {
            loader: 'style-loader',
            options: {
              hmr: false,
            },
          },
          'css-loader',
        ],
      },
      {
        test: /\.scss$/,
        loader: ExtractTextPlugin.extract({
          fallback: 'style-loader',
          use: [
            { loader: 'css-loader', options: { importLoaders: 1, minimize: true, sourceMap: true } },
            { loader: 'sass-loader', options: { sourceMap: true } },
          ],
        }),
      },
      {
        test: staticAssetTestRegex,
        use: [{
          loader: 'url-loader',
          options: {
            limit: inlineSizeLimitInBytes,
            name: '[name].[hash].[ext]',
          },
        }],
      },
    ],
  },

  plugins: [
    new CleanWebpackPlugin([distPath], {
      root: projectRootPath,
    }),

    // creates application shell which is just *another* ejs template used by express' view system
    new HtmlWebpackPlugin({
      title: 'Adobe Stock Moderation',
      template: './app/client/shell/index.ejs',
      // emit again as as ejs so that we can use express's ejs view engine
      filename: '../index.ejs',
      // the base route for static assets
      [RENDERING.TEMPLATE_STATIC_ROUTE]: ROUTES.STATIC,
      // write an ejs token write to inject SSR'd markup into the app's mount point
      markupToken: `<%- ${RENDERING.TOKEN_MARKUP} %>`,
      // write an ejs token to inject the base hostname which all static assets reference
      staticAssetsHostToken: `<%- ${RENDERING.TOKEN_STATIC_ASSETS_HOST} %>`,
      // write an ejs token to inject config var name into
      clientConfigVarToken: `<%- ${RENDERING.TOKEN_CLIENT_CONFIG_VAR} %>`,
      // write an ejs token to inject initial redux state, etc., into for the client
      clientConfigToken: `<%- ${RENDERING.TOKEN_CLIENT_CONFIG} %>`,
      // write an ejs token to inject SSR'd, code-split bundles into
      scriptsToken: `<%- ${RENDERING.TOKEN_SCRIPTS} %>`,
      // write an ejs token to inject the nonce
      nonceToken: `<%- ${RENDERING.TOKEN_NONCE} %>`,
      // we do manual injection via htmlWebpackPlugin.files array in our ejs template
      inject: false,
      appShell: {
        // currently disabled by default due to lack of a proper dev/prod setup
        useServiceWorker: false,
      },
      meta: {
        favicon: 'favicon.ico',
        launcherIcon: 'launcher-icon-4x.png',
      },
    }),

    // create the real PWA manifest.json
    new HtmlWebpackPlugin({
      template: './app/client/shell/manifest.json.ejs',
      filename: 'manifest.json',
      inject: false,
    }),

    // specifies react-spectrum theme inclusion/exclusion
    new webpack.DefinePlugin({
      'process.env.SCALE_MEDIUM': 'true',
      'process.env.SCALE_LARGE': 'false',
      'process.env.THEME_LIGHT': 'true',
      'process.env.THEME_LIGHTEST': 'false',
      'process.env.THEME_DARK': 'false',
      'process.env.THEME_DARKEST': 'false',
    }),

    //
    // BEGIN bundle splitting and long-term caching
    //

    // https://github.com/thejameskyle/react-loadable#--------------server-side-rendering
    // creates dynamic module manifest
    new ReactLoadablePlugin({
      filename: path.resolve(buildPath, codeSplittingManifestFilename),
    }),

    // makes sure each split chunk actually takes the name we have given it.
    // we're using this over the HashedModuleIdsPlugin due to recommendations from the following:
    // https://medium.com/webpack/predictable-long-term-caching-with-webpack-d3eee1d3fa31
    new webpack.NamedChunksPlugin((chunk) => {
      // we get chunk names from a couple of places:
      // 1. from an "entry" in this config
      // 2. from an inline "webpackChunkName: 'some-custom-name'" comment directly inside
      //    import() statements
      if (chunk.name) {
        return chunk.name;
      }

      // and if we don't have an explicit chunk name, create a unique one from the chunk's "context"
      return chunk.mapModules(m => path.relative(m.context, m.request)).join('-');
    }),

    // extract common modules, even across async chunks (e.g. using import())
    // https://webpack.js.org/plugins/commons-chunk-plugin/#commons-chunk-for-entries
    // https://medium.com/webpack/bundle-buddy-and-webpack-commons-chunk-101da29166bf
    new webpack.optimize.CommonsChunkPlugin({
      // use all children of the chunk
      children: true,

      // split out the common chunk into a new async common chunk, call it "common"
      async: 'common',

      // 2 children must share the module before it's separated.
      // this should be tweaked if the overhead of an extra network request is worse than
      // the additional parse time of duplicated code
      minChunks: 2,
    }),

    // implicit splitting of all external packages
    // https://webpack.js.org/plugins/commons-chunk-plugin/#passing-the-minchunks-property-a-function
    new webpack.optimize.CommonsChunkPlugin({
      name: 'vendor',
      minChunks(module) {
        return module.context && module.context.indexOf('node_modules') !== -1;
      },
    }),

    // extract webpack's manifest in order for it to not bust the hashed module names every time
    // a new module is introduced (thus changing the internal manifest, thus changing the hash)
    // https://webpack.js.org/guides/caching/#extracting-boilerplate
    new webpack.optimize.CommonsChunkPlugin({
      name: 'manifest',
      minChunks: Infinity,
    }),

    //
    // END bundle splitting and long-term caching
    //
  ],
};

Also thanks a ton @plondon for helping a stranger out

@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