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

Does integrity="" intentionally not work on module <script>s? #2382

Closed
domenic opened this Issue Feb 22, 2017 · 16 comments

Comments

6 participants
@domenic
Member

domenic commented Feb 22, 2017

I just noticed today that https://html.spec.whatwg.org/#prepare-a-script step 21.6 passes in the integrity metadata for classic scripts but not module scripts.

Maybe this is intentional, because it's not super-clear which of the fetched scripts in the graph it would apply to? For example, nonce="" and crossorigin="" apply to everything in the graph, so maybe it'd be weird to only apply integrity="" to the top-level script? I dunno if that reasoning is sound though...

If this is intentional, we should add a note clarifying.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Feb 23, 2017

Member

Not being able to declare integrity for module subresources is a problem, but the folks designing modules were not keen on my input that you needed to be able to control the fetch layer declaratively. I guess we could still offer it for the top-level fetch though and provide the same kind of guarantees as you get with CSS...

Member

annevk commented Feb 23, 2017

Not being able to declare integrity for module subresources is a problem, but the folks designing modules were not keen on my input that you needed to be able to control the fetch layer declaratively. I guess we could still offer it for the top-level fetch though and provide the same kind of guarantees as you get with CSS...

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Feb 23, 2017

Member

I agree with @annevk.

CCing the folks responsible for SRI: @mozfreddyb, @devd, @fmarier, @metromoxie.

Member

mikewest commented Feb 23, 2017

I agree with @annevk.

CCing the folks responsible for SRI: @mozfreddyb, @devd, @fmarier, @metromoxie.

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Feb 23, 2017

Member

(Longer term, it would be probably be useful to somehow define fetch options for importScripts and import.)

Member

mikewest commented Feb 23, 2017

(Longer term, it would be probably be useful to somehow define fetch options for importScripts and import.)

@devd

This comment has been minimized.

Show comment
Hide comment
@devd

devd Feb 23, 2017

I think for now, for SRIv1 this is not a big deal. For styles too, there are many things we don't allow setting integrity metadata. It would be nicer if it just did support it so that we don't have to aptch it on later. So, I guess I agree with Anne!

I wonder how this works with the require-sri directives. If I turned on require-sri-for script, will the module load succeed or fail?

devd commented Feb 23, 2017

I think for now, for SRIv1 this is not a big deal. For styles too, there are many things we don't allow setting integrity metadata. It would be nicer if it just did support it so that we don't have to aptch it on later. So, I guess I agree with Anne!

I wonder how this works with the require-sri directives. If I turned on require-sri-for script, will the module load succeed or fail?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 23, 2017

Member

So just to be clear, for everyone agreeing with Anne, it sounds like we should wire it up so integrity="" applies to the top-level fetch when fetching a module script graph? I can do that pretty easily.

I wonder how this works with the require-sri directives. If I turned on require-sri-for script, will the module load succeed or fail?

Presumably they'd act just like CSS background-image: url(...) or @import "..." do, where those don't have any integrity metadata. Do those fail, or succeed?

Member

domenic commented Feb 23, 2017

So just to be clear, for everyone agreeing with Anne, it sounds like we should wire it up so integrity="" applies to the top-level fetch when fetching a module script graph? I can do that pretty easily.

I wonder how this works with the require-sri directives. If I turned on require-sri-for script, will the module load succeed or fail?

Presumably they'd act just like CSS background-image: url(...) or @import "..." do, where those don't have any integrity metadata. Do those fail, or succeed?

@devd

This comment has been minimized.

Show comment
Hide comment
@devd

devd Feb 23, 2017

@mozfreddyb and @mikewest probably know.. I am in the "TIL module script graph" phase right now :)

devd commented Feb 23, 2017

@mozfreddyb and @mikewest probably know.. I am in the "TIL module script graph" phase right now :)

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Mar 6, 2017

Member

@domenic: Yes. I think it makes sense to use the integrity attribute to govern the content of the top-level fetch, and to rely on an as-yet-to-be-invented mechanism for integrity checks on branch-level fetches.

With regard to require-sri-for, yes, those internal branch requests would fail. That's an argument for y'all to get to work on SRI2, I think. :)

Member

mikewest commented Mar 6, 2017

@domenic: Yes. I think it makes sense to use the integrity attribute to govern the content of the top-level fetch, and to rely on an as-yet-to-be-invented mechanism for integrity checks on branch-level fetches.

With regard to require-sri-for, yes, those internal branch requests would fail. That's an argument for y'all to get to work on SRI2, I think. :)

@mozfreddyb

This comment has been minimized.

Show comment
Hide comment
@mozfreddyb

mozfreddyb Mar 7, 2017

It seems this discussion achieved rough consensus without my comments, but here it is:
I agree with @annevk. For now, the integrity attribute should work for module top-level fetches.

We'll have to figure other loads out in the future (CSS @import, url(), importScripts etc.). As implemented (and intended) require-sri-for will break: There is no integrity metadata provided (as there's no way to provide it), so verification is can't succeed.

mozfreddyb commented Mar 7, 2017

It seems this discussion achieved rough consensus without my comments, but here it is:
I agree with @annevk. For now, the integrity attribute should work for module top-level fetches.

We'll have to figure other loads out in the future (CSS @import, url(), importScripts etc.). As implemented (and intended) require-sri-for will break: There is no integrity metadata provided (as there's no way to provide it), so verification is can't succeed.

@domenic domenic assigned domenic and unassigned mikewest May 4, 2017

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford May 5, 2017

Is there any existing proposal or discussion for how to handle SRI for module dependencies? Ideally it should be possible to declaratively include this information in the page or server response, just like we provide preload metadata currently.

guybedford commented May 5, 2017

Is there any existing proposal or discussion for how to handle SRI for module dependencies? Ideally it should be possible to declaratively include this information in the page or server response, just like we provide preload metadata currently.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 5, 2017

Member

No, it's basically up to whatever #2547 ends up being. We should maybe keep that open?

Member

annevk commented May 5, 2017

No, it's basically up to whatever #2547 ends up being. We should maybe keep that open?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic
Member

domenic commented May 5, 2017

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford May 5, 2017

@domenic @annevk I'd strongly avoid inlining the integrity into the ES module syntax, the reason being that the parent module is now tied to the cache of its child. This is a worst-case caching scenario, and we can and should do better than that for production modules workflows.

guybedford commented May 5, 2017

@domenic @annevk I'd strongly avoid inlining the integrity into the ES module syntax, the reason being that the parent module is now tied to the cache of its child. This is a worst-case caching scenario, and we can and should do better than that for production modules workflows.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 5, 2017

Member

@guybedford right, if you just let the service worker do all request management you can also have a central cache of integrity data. I agree that the design of integrity as it stands today is not ideal and it's not just a problem for JavaScript.

Member

annevk commented May 5, 2017

@guybedford right, if you just let the service worker do all request management you can also have a central cache of integrity data. I agree that the design of integrity as it stands today is not ideal and it's not just a problem for JavaScript.

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford May 6, 2017

@annevk we shouldn't have to rely on service worker either just to get performant and secure module workflows in browsers though. Would it be possible to come to a declarative solution here that doesn't tie the integrity to the parent module source, but rather to the top-level page itself?

guybedford commented May 6, 2017

@annevk we shouldn't have to rely on service worker either just to get performant and secure module workflows in browsers though. Would it be possible to come to a declarative solution here that doesn't tie the integrity to the parent module source, but rather to the top-level page itself?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 6, 2017

Member

It's possible, but I don't see it happening without going down the imperative path first.

Member

annevk commented May 6, 2017

It's possible, but I don't see it happening without going down the imperative path first.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 6, 2017

Member

It's also not really a module script-specific issue. We'd want to solve it for CSS, images, etc.

Member

domenic commented May 6, 2017

It's also not really a module script-specific issue. We'd want to solve it for CSS, images, etc.

domenic added a commit that referenced this issue Sep 14, 2017

Make integrity="" work on module scripts
The primary normative content of this commit is that it fixes #2382 by
passing the integrity metadata to the fetch call for the top-level
module script, in <script type=module>.

However, the way it does this is via a larger refactoring, which is
setting the stage for #2315. It creates a new struct, the script fetch
options, which is now shared by both module and classic scripts. Storing
this for classic scripts is not currently useful, but will be for #2315
when, via import(), classic scripts are able to import module scripts,
and need these fetch options to do so.

This will also be useful for when we revive #2383, as
<link rel=modulepreload> can have referrerpolicy="" specified on it,
which will need to be passed down. (It would also be useful if we ever
do w3c/webappsec-referrer-policy#96 and add
referrerpolicy="" to <script>.) With this structure in place, it's a
simple matter of adding a referrer policy item to the script fetch
options.

domenic added a commit that referenced this issue Oct 3, 2017

Make integrity="" work on module scripts
The primary normative content of this commit is that it fixes #2382 by
passing the integrity metadata to the fetch call for the top-level
module script, in <script type=module>.

However, the way it does this is via a larger refactoring, which is
setting the stage for #2315. It creates a new struct, the script fetch
options, which is now shared by both module and classic scripts. Storing
this for classic scripts is not currently useful, but will be for #2315
when, via import(), classic scripts are able to import module scripts,
and need these fetch options to do so.

This will also be useful for when we revive #2383, as
<link rel=modulepreload> can have referrerpolicy="" specified on it,
which will need to be passed down. (It would also be useful if we ever
do w3c/webappsec-referrer-policy#96 and add
referrerpolicy="" to <script>.) With this structure in place, it's a
simple matter of adding a referrer policy item to the script fetch
options.

@domenic domenic closed this in 9275d95 Oct 3, 2017

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