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

Tweak request to make integration with webpack-subresource-integrity easy #330

Closed
SPSpwetter opened this issue May 26, 2016 · 14 comments
Closed

Comments

@SPSpwetter
Copy link

SPSpwetter commented May 26, 2016

I got sub resource integrity checking working using this slightly hacky code:

waysact/webpack-subresource-integrity#3

The hacky part is that I hard coded my file name template (chunk.[name].bundle.js] in order to build the key for compilation.assets. If htmlWebpackPlugin.files.chunks[chunk].assetKey had the asset key, that would make it less hacky. Or maybe i'm doing it wrong.

@SPSpwetter SPSpwetter changed the title Tweak request to make integration with webpack-subresource-integrity Tweak request to make integration with webpack-subresource-integrity easy May 26, 2016
@jantimon
Copy link
Owner

Well as inject:true is the default you might want to go with it and add a hook to alter the scripts.

@jantimon
Copy link
Owner

Take a look at the 'html-webpack-plugin-before-html-generation' it will pass you all information you need.

@SPSpwetter
Copy link
Author

Ok, I'm willing to do that work, but you have to understand that I didn't understand what you said having only started working with webpack a month ago in an effort to migrate a Makefile+cat build to something more reasonable. Could you spell out step by step a bit more what you think I should do?

@SPSpwetter
Copy link
Author

I started down the path of what I thought you were asking me to do, but it was a 1-line change to the plugin to squirrel away the base name of the file when building the chunk hash, and I couldn't figure out how to debug a plugin very effectively never having written a webpack plugin before, though the README.md on this project was a good start.

So I submitted the above pull request instead. With this pull request, adding integrity checks or anything that can be read from compilation.assets is just:

integrity = "<%= compilation.assets[htmlWebpackPlugin.files.chunks[chunk].basename].integrity %>"

In short:

You should accept the above pull request because:

  1. It's benign, and simple.
  2. It makes a whole collection of things that might want to extend the html output easier, if they can just annotate compilation.assets.

@jantimon
Copy link
Owner

@SPSpwetter
Copy link
Author

SPSpwetter commented Jun 8, 2016

Then someone needs 3 plugins+template to implement SRI:

  1. The SRI plugin.
  2. HTML webpack
  3. My hook plugin.
  4. Custom templates.

I figured having the base name available was a generally useful enhancement that would enable people to do whatever they needed in a template by writing a plugin to enhance the compilation object, where they could pick it up later.

If you want it to work with injection, I could also modify the injection to see if the integrity value is present for an asset and modify the injection tag there. Then no template is necessary. That would be nice because then SRI only requires #1 and #2 above, if you want SRI, you calculate it.

@jantimon
Copy link
Owner

jantimon commented Jun 9, 2016

Some reasons why this is not my preferred way to solve this:

  1. assets.chunks are legacy from 1.0 and might become deprecated as we have assets.js and assets.css now - so I would rather not extend them any further.
  2. inject:true is the default behaviour and it would not work with your approach

With your solution it would take a inject:false, a custom template, the sri plugin and the html webpack plugin.

I would propose to add a hook to the SRI plugin then we could have a configuration like this to get it working:

plugins: [
        new WebpackHtmlPlugin(),
        new SriPlugin(['sha256', 'sha384']),
    ]

What do you think?

@SPSpwetter
Copy link
Author

SPSpwetter commented Jun 9, 2016

Simpler would definitely be better, it would be best to just have two plugins, but I wasn't sure how to mutate the <script> tags after they'd already been injected. In the sample you sent me, you're doing a search/replace on the <head tag, but that seems kind of hacky and error prone to me.

It would have to find the already added script tags, parse them to get the path, look them up in the compilation object, then write out the integrity attribute. Ick.

If you can give me more of a hint on a way to change the script tags more elegantly, I can look into adding a hook to the SRI plugin instead.

Thinking about it, a cleaner way would be to allow hooks to add additional attributes prior to injection to assets.js and assets.css in assets.attributes[]. The injection code could then loop over any any additional attributes and inject them. It would be slightly more complexity on line 454 and 459 of index.js but not too bad.

attr = assets.additions[scriptPath]
if (attr)
{
additionalAttributes = Object.keys(attr).map( function(key) {key + '="'+attr[key]+"'"'}).join(" ")
}

Do you like that?

@jantimon
Copy link
Owner

jantimon commented Jun 9, 2016

Oh I like that idea - checkout #345

@SPSpwetter
Copy link
Author

Oh, great! I'll work on the SRI plugin hook on the assumption this will go in. Much cleaner than my idea in the details.

@jantimon
Copy link
Owner

jantimon commented Jun 9, 2016

Cool please let me know if it doesn't work properly

@SPSpwetter
Copy link
Author

SRI Plugin mods built, see PR: waysact/webpack-subresource-integrity#5

@jantimon
Copy link
Owner

Integrated in webpack-subresource-integrity 0.4.0

@lock
Copy link

lock bot commented May 31, 2018

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants