-
Notifications
You must be signed in to change notification settings - Fork 47
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
webpack-subresource-integrity overwrites integrity values on explicitly set externals #97
Comments
Hi, thanks for your bug report. I've added a simple test case (see above) and it seems to work OK. Would you mind putting together one that fails? See https://github.com/waysact/webpack-subresource-integrity/blob/master/CONTRIBUTING.md Let me know if you have any trouble writing the test case. You can also point me to a sample repository that demonstrates the issue. |
Hi @jscheid! Thanks for your super fast response! Created a sample for demo here: https://github.com/r0bs/sri-null |
@r0bs no problem, thanks for the test repo. The test case had a bug which I didn't notice because the issue only manifests with certain dependency versions. I can reproduce now, aiming to roll a release with the fix tomorrow. |
Fix released in 1.3.1. |
Hey @jscheid ! Thanks for the fix, solves the issue for me. Now explicitly set integrity attributes on external assets don't get overwritten. However, if one decides to purposefully omit the integrity attribute on an external asset, |
Yeah I realized this too. This can be improved still. (Adding an external asset without integrity while using this plugin is pointless: this plugin has no way of knowing which integrity value to use, and wanting to add one without integrity is Achilles' heel. But, we should output a warning (or perhaps even trigger an error with a good message) instead of letting the page crash.) |
Hey @jscheid! Could you check https://github.com/waysact/webpack-subresource-integrity/compare/master...r0bs:feature/%2397?expand=1 ? It isn't working, since a preexisting test (the one you added with the latest release) now fails, missing to get the explicitly set attributes. What am I getting wrong here? |
Hi @r0bs , thanks for taking a shot at it. I'm completely swamped this week and won't get a chance to focus on it, sorry. But one thing I've noticed after a quick glance is that this shouldn't work only for http and https URLs. For one thing there are other URL schemes that, afaik, are also supported by browsers and by SRI, for example ftp. Also it's conceivable that a HWP plugin copies assets straight to the target directory (bypassing webpack) and references them with a relative URL. So I think the logic should be: if there is a HWP asset tag without the integrity attribute and we don't know its integrity value, generate an error. If there is one with integrity attribute and we don't know its integrity value, ignore it. And perhaps as a bonus: if there is one and its integrity value is different from the one we've calculated, ignore it but print a warning. Sorry I can't be of more help right now, but I'm happy to take a closer look next week. |
@r0bs ok, now I've found a few minutes. This is somewhat complicated, I can see why you are confused. Bear with me, I'll try to explain my findings as clearly as possible -- let me know if anything doesn't make sense.
Both HWIA and SRI register the Both use it to add attributes to the tag: HWIA the user-specified ones, SRI the computed integrity (and crossOrigin). Now, what makes this issue so confusing is that the order in which both registered callbacks are invoked differs depending on the Webpack version! Webpack 3 and earlier, SRI gets invoked first, then HWIA. With Webpack 4 it's the other way around, HWIA gets invoked first, then SRI. This also explains why I initially had trouble reproducing your original issue: when SRI gets invoked first, it adds the bogus Now, if you clone this repo and run If you do this:
... then you will get a different error, but I haven't investigated the reason for that error yet. I suppose this isn't terribly helpful, but hopefully at least it aligns expectations with reality? As to what next: it's a bit tricky. I'm not prepared to drop support for older Webpack versions just yet. I think ideally we'd find a way to ensure that SRI always gets invoked after HWIA. Unfortunately, tapable (the system Webpack uses for hook management) doesn't have a way to declare dependencies between hooks, so it'll probably have to be a hack. I'll have to dig deeper into HWIA in order to understand the problem better. Realistically I won't get to that before mid January, though. In the meantime, feel free to give it another shot, perhaps you can find a way to make it work? Let me know if you have questions about any of this. Thanks again for looking into it. (PS: I realize that running the tests for a different set of versions is pretty awkward at the moment, what with having to run |
Closing stale issue, I don't know what else I can do here. Feel free to reopen if this is still an issue. |
I'm using webpack-subresource-integrity to set integrity hashs on local resources. This works fine. At the same time I'm defining externals (including integrity hash) manually with html-webpack-externals-plugin like this:
However, the integrity attribute get's overwritten by webpack-subresource-integrity with null, resulitng in this:
<script type="text/javascript" src="https://code.jquery.com/jquery-3.2.1.js" integrity="null" crossorigin="anonymous"></script>
Is there anyway to prevent this unwanted behavior?
The text was updated successfully, but these errors were encountered: