Bug 177959 - Support integrity="" on module scripts
Summary: Support integrity="" on module scripts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 178162
Blocks: 147340
  Show dependency treegraph
 
Reported: 2017-10-05 12:29 PDT by Domenic Denicola
Modified: 2017-10-12 06:12 PDT (History)
13 users (show)

See Also:


Attachments
Patch (85.71 KB, patch)
2017-10-11 08:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (107.36 KB, patch)
2017-10-11 12:09 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (108.68 KB, patch)
2017-10-12 03:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Domenic Denicola 2017-10-05 12:29:34 PDT
This was recently added to the spec in https://github.com/whatwg/html/commit/9275d955dcd604e959cfcc672e0c234b1b8c00db . It applies to the top-level fetch only.

Spec: https://html.spec.whatwg.org/#prepare-a-script step 18 and step 22.6.

Test: http://w3c-test.org/html/semantics/scripting-1/the-script-element/module/integrity.html
Comment 1 Radar WebKit Bug Importer 2017-10-09 12:52:53 PDT
<rdar://problem/34892850>
Comment 2 Yusuke Suzuki 2017-10-11 08:48:16 PDT
Created attachment 323420 [details]
Patch

WIP: Works in GTK with WPT. Need to add files to Xcode project file.
Comment 3 Yusuke Suzuki 2017-10-11 12:09:18 PDT
Created attachment 323439 [details]
Patch
Comment 4 Sam Weinig 2017-10-11 22:37:24 PDT
Comment on attachment 323439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323439&action=review

Thanks so much for tackling this!

The high level approach I was thinking of taking was to to pass integrity down from LoadableModuleScript -> LoadableScript -> ScriptElementCachedScriptFetcher -> CachedScriptFetcher (and modify LoadableClassicScript to also pass it down). Then in ScriptModuleLoader::notifyFinished, get the integrity off the CachedScriptFetcher which is already available from the CachedModuleScriptLoader.

What is the benefit of adding the new 'parameters' object in addition to the existing 'scriptFetcher'?

> Source/JavaScriptCore/ChangeLog:21
> +        can use thsi parameters to fetch modules.

thsi -> this.
Comment 5 Yusuke Suzuki 2017-10-11 22:56:05 PDT
Comment on attachment 323439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323439&action=review

> Source/JavaScriptCore/ChangeLog:18
> +        We separately pass this parameters to the pipeline along with the script fetcher.
> +        The script fetcher is only one for module graph since this is the initiator of
> +        this module graph loading. On the other hand, this parameters is for each
> +        module fetching.

Yeah, that's super good question!
We add `parameters` in addition to script fetcher. This is because script fetcher is a `per-module-graph`.
This script fetcher is shared among all the modules in the given module graph.
While it is sufficient to pass parameters to top-level-module's fetching, it is not enough for the future extension.

In the future, we will investigate a way to pass parameters to each non-top-level module.
At that time, this `parameters` should be per-module. This is because `integrity` value should be different for each module.
For example, we will accept some form of syntax to add parameters to `import`. Some proposed syntax is like https://discourse.wicg.io/t/specifying-nonce-or-integrity-when-importing-modules/1861

import "./xxx.js" integrity "xxxxxxx"

In this case, this `parameters` will be passed to "./xxx.js" module fetching. This `parameters` should be different from the one of top-level-module's one. That's why we need per-module `parameters` and why this patch adds `parameters` to the module pipeline.

On the other hand, we also want to keep script fetcher. This `per-module-graph` thing is important to offer module-graph-wide information.
For example, import.meta would have `import.meta.scriptElement`, which is the script element fetching the module graph including this.
That's why we keep both.
https://github.com/tc39/proposal-import-meta

>> Source/JavaScriptCore/ChangeLog:21
>> +        can use thsi parameters to fetch modules.
> 
> thsi -> this.

Oops, fixing.
Comment 6 Yusuke Suzuki 2017-10-11 23:08:01 PDT
Comment on attachment 323439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323439&action=review

> Source/JavaScriptCore/builtins/ModuleLoaderPrototype.js:250
> +                return this.requestSatisfy(depKey, @undefined, fetcher).then((entry) => {

And this is important point. When fetching dependent modules, currently we pass `@undefined` as `parameters`.
This should be changed to per-module parameters once the way to take per-module parameters are defined in the spec.
Comment 7 Yusuke Suzuki 2017-10-12 03:45:52 PDT
Created attachment 323519 [details]
Patch
Comment 8 Sam Weinig 2017-10-12 05:14:13 PDT
Comment on attachment 323519 [details]
Patch

Thanks for the detailed explanation! Looks great.
Comment 9 Yusuke Suzuki 2017-10-12 05:30:13 PDT
Comment on attachment 323519 [details]
Patch

Thank you for your reviews ;)
Comment 10 WebKit Commit Bot 2017-10-12 06:12:53 PDT
Comment on attachment 323519 [details]
Patch

Clearing flags on attachment: 323519

Committed r223237: <https://trac.webkit.org/changeset/223237>
Comment 11 WebKit Commit Bot 2017-10-12 06:12:55 PDT
All reviewed patches have been landed.  Closing bug.