Add subresource integrity support for ES modules, through importmaps #10269

yoavweiss · 2024-04-10T03:02:35Z

SRI support for ES module imports would enable using them in documents that require SRI for certain scripts for security or privacy reasons.

At least two implementers are interested (and none opposed):
- Chromium - I'm implementing this.
- WebKit - supportive, I'm implementing.
Tests are written and can be reviewed and commented upon at:
Implementation bugs are filed:
MDN issue is filed: Add support for subresource integrity in imported module scripts mdn/mdn#541
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

/infrastructure.html ( diff )
/links.html ( diff )
/scripting.html ( diff )
/webappapis.html ( diff )

domenic · 2024-04-11T05:51:50Z

Some initial comments:

I'd envisioned the keys being URLs, not specifiers. https://github.com/guybedford/import-maps-extensions?tab=readme-ov-file#integrity isn't super-clear (/cc @guybedford) but the extra indirection of the specifiers doesn't seem like a great idea to me here. I'm open to being persuaded though.
Make sure that when you do the actual integrity checking, it applies to all imports, not just dynamic ones. (It looks like there's no integrity checking in the spec PR yet, but in the Chromium draft CL WPTs I only see dynamic import tests.)

yoavweiss · 2024-04-11T06:24:21Z

Thanks for taking a look!

I'd envisioned the keys being URLs, not specifiers. https://github.com/guybedford/import-maps-extensions?tab=readme-ov-file#integrity isn't super-clear (/cc @guybedford) but the extra indirection of the specifiers doesn't seem like a great idea to me here. I'm open to being persuaded though.

That's a good point, and I'm currently using the resolved URLs as the keys to the internal lookup, which is admittedly a bit awkward. Moving to use the URLs explicitly makes sense to me, and will simplify the code a bit.

It looks like there's no integrity checking in the spec PR yet

Good point, I missed adding it. Will do!

in the Chromium draft CL WPTs I only see dynamic import tests

Yeah, I was planning on testing static imports as well (but ran out of power on the flight :P). Will add that before this will all be ready for review.

yoavweiss · 2024-04-15T07:44:54Z

@domenic - I believe this is now ready for review. Also, I marked Chromium as supportive given that I'm implementing this, but let me know if that requires more explicit support from the Chrome team.

yoavweiss · 2024-04-15T07:49:05Z

(Also, PR preview doesn't seem to update to the latest version for some reason)

domenic

Exciting!

source

yoavweiss

Thanks for reviewing!! Addressing some of the comments, to clear things up for the rest :)

source

SRI support for ES modules enables using them in documents that require SRI for certain scripts for security reasons, as well as with the move overarching require-sri-for CSP directive. This CL implements whatwg/html#10269 based on https://github.com/guybedford/import-maps-extensions#integrity Change-Id: Ida563334048d013ffc658f9783f9401930dd4689 Bug: 334251999

source

hiroshige-g · 2024-04-16T08:00:04Z

The explainer says:

The "integrity" for any module request is looked up from this import maps integrity attribute whenever there is no integrity specified.
<script type="module" integrity="..."> integrity attribute on a module script will take precedence over the import map integrity.
The import map integrity will only apply to modules and not other assets.

But in the current PR (as well as the draft CL) the import-map-originating hashes are NOT applied to

<script src="a.js" type="module">
<link rel=modulepreload href="a.js">

Is this intentional, or spec/impl should be fixed?

yoavweiss · 2024-04-16T08:15:27Z

Is this intentional, or spec/impl should be fixed?

The functionality is intentional (as discussed with @domenic), but I missed the fact that the explainer doesn't align with what we decided on. But I guess we could also modify the spec/impl to handle that case.

@domenic - WDYT?

domenic · 2024-04-16T08:46:01Z

I slightly lean toward the version we have now where it only applies to "imports", not other "module loads". But, I don't feel strongly. @guybedford and other @whatwg/modules people, do you have thoughts?

SRI support for ES modules enables using them in documents that require SRI for certain scripts for security reasons, as well as with the move overarching require-sri-for CSP directive. This CL implements whatwg/html#10269 based on https://github.com/guybedford/import-maps-extensions#integrity Change-Id: Ida563334048d013ffc658f9783f9401930dd4689 Bug: 334251999

guybedford · 2024-04-16T18:11:45Z

Has been great to follow this PR, excited to see this one.

I think supporting the import map integrity for top-level script loads would be useful. If we are to consider the import map as the source of truth, then updating the import map is enough to update all usage, without having to update every module script reference point. So it would be a net reduction in workflow friction. Implementation would be as described where explicit integrity on a tag acts as an override. If we ever support explicit integrity in import attributes one might expect similar there as well so it isn't necessarily unique to the top-level.

If going in that direction, preloads could be nice to support as well, to avoid needing to prune the import map integrity to the non-preloaded graph when generating preloads.

While we are working through edge cases - I assume this all applies equally to JSON imports? It would be good to ensure that is tested as well.

domenic

Pretty much all minor editorial things at this point, although we have a normative discussion about the interaction with other entry points too.

source

domenic · 2024-04-17T02:25:32Z

I think supporting the import map integrity for top-level script loads would be useful. If we are to consider the import map as the source of truth, then updating the import map is enough to update all usage, without having to update every module script reference point. So it would be a net reduction in workflow friction. Implementation would be as described where explicit integrity on a tag acts as an override. If we ever support explicit integrity in import attributes one might expect similar there as well so it isn't necessarily unique to the top-level.

I find this relatively compelling and in the absence of any counterarguments I'd switch to weakly being in favor of supporting this case.

I think the parts of the spec to modify are just the call sites of "fetch an external module script graph" and "fetch a modulepreload module script graph".

If going in that direction, preloads could be nice to support as well, to avoid needing to prune the import map integrity to the non-preloaded graph when generating preloads.

To be clear, I think we should support modulepreload, but not preload. (Or prefetch, or fetch(), or other call sites.)

If this is too confusing then maybe it's reason to reconsider... After all, we need to be cautious about @hiroshige-g's concern at https://groups.google.com/a/chromium.org/g/blink-dev/c/O2UR3kb-HcI/m/tWgtOEixAQAJ .

While we are working through edge cases - I assume this all applies equally to JSON imports? It would be good to ensure that is tested as well.

+1. It should work per spec, and adding one or two tests for JSON and CSS module scripts would be great.

yoavweiss

Thanks!!

source

SRI support for ES modules enables using them in documents that require SRI for certain scripts for security reasons, as well as with the move overarching require-sri-for CSP directive. This CL implements whatwg/html#10269 based on https://github.com/guybedford/import-maps-extensions#integrity Change-Id: Ida563334048d013ffc658f9783f9401930dd4689 Bug: 334251999

yoavweiss · 2024-04-17T07:14:38Z

+1. It should work per spec, and adding one or two tests for JSON and CSS module scripts would be great.

I agree, but it seems like CSS and JSON module scripts are not at all tested with import maps (unless I'm missing something).

yoavweiss · 2024-04-17T07:23:42Z

I think the parts of the spec to modify are just the call sites of "fetch an external module script graph" and "fetch a modulepreload module script graph".

It might be cleaner to override the integrity metadata inside these calls in case it's the empty string (rather than at the call sites). WDYT?

domenic · 2024-04-17T07:54:47Z

It might be cleaner to override the integrity metadata inside these calls in case it's the empty string (rather than at the call sites). WDYT?

I think only the call sites will know whether the attribute is absent (should fall back) or the empty string (should probably use the empty string)?

This reverts commit b4e94b6.

yoavweiss · 2024-05-15T08:40:47Z

@annevk - any blockers/concerns for landing this?

…ules, using importmaps, a=testonly Automatic update from web-platform-tests Subresource Integrity support for ES modules, using importmaps (#45638) SRI support for ES modules enables using them in documents that require SRI for certain scripts for security reasons, as well as with the move overarching require-sri-for CSP directive. This CL implements whatwg/html#10269 based on https://github.com/guybedford/import-maps-extensions#integrity I2P: https://groups.google.com/a/chromium.org/g/blink-dev/c/O2UR3kb-HcI/m/7Jh7_GYsAAAJ?utm_medium=email&utm_source=footer Change-Id: Ida563334048d013ffc658f9783f9401930dd4689 Bug: 334251999 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5441822 Reviewed-by: Domenic Denicola <domenic@chromium.org> Commit-Queue: Yoav Weiss (@Shopify) <yoavweiss@chromium.org> Cr-Commit-Position: refs/heads/main@{#1297376} Co-authored-by: Yoav Weiss <yoavweiss@chromium.org> -- wpt-commits: 7daf23a6329f4577bc3723d5e25eae8eae26e710 wpt-pr: 45638

https://bugs.webkit.org/show_bug.cgi?id=272884 Reviewed by Ryosuke Niwa. Imported ES modules can't currently have integrity checks, which means they can't be used in sites where integrity checks are a necessity, for security and privacy reasons. This implements such support, by adding an "integrity" section to import maps. See whatwg/html#10269 * LayoutTests/TestExpectations: Ignored console logs to avoid flakiness * LayoutTests/imported/w3c/web-platform-tests/import-maps/WEB_FEATURES.yml: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/data-driven/resources/test-helper.js: (createTestIframe): Updated through import. * LayoutTests/imported/w3c/web-platform-tests/import-maps/dynamic-integrity-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/dynamic-integrity.html: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/no-referencing-script-integrity-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/no-referencing-script-integrity-valid-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/no-referencing-script-integrity-valid.html: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/no-referencing-script-integrity.html: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/nonimport-integrity-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/nonimport-integrity.html: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/static-integrity-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/static-integrity.html: Added. * LayoutTests/imported/w3c/web-platform-tests/import-maps/w3c-import.log: Imports. * LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-resources.https-expected.txt: Updated. * LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-resources.https.html: Updated to cover Request.integrity. * LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-request-resources-iframe.https.html: Updated to cover Request.integrity. * LayoutTests/platform/glib/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-resources.https-expected.txt: Updated. * Source/JavaScriptCore/runtime/ImportMap.cpp: (JSC::ImportMap::resolveImportMatch): Typos and spec link. (JSC::parseURLLikeModuleSpecifier): Typos and spec link. (JSC::ImportMap::resolve const): Typos and spec link. (JSC::normalizeSpecifierKey): Typos and spec link. (JSC::sortAndNormalizeSpecifierMap): Typos and spec link. (JSC::ImportMap::registerImportMap): Add parsing for the integrity section. (JSC::ImportMap::getIntegrity const): Getter for integrity based on URL. * Source/JavaScriptCore/runtime/ImportMap.h: * Source/WebCore/bindings/js/ScriptModuleLoader.cpp: (WebCore::ScriptModuleLoader::importModule): Add integrity to outgoing requests. (WebCore::ScriptModuleLoader::notifyFinished): Enforce integrity from the importmap on responses, even if integrity wasn't present in the request. Needed for static imports triggered by JSCore. * Source/WebCore/dom/ScriptElement.cpp: (WebCore::ScriptElement::requestModuleScript): Add integrity to outgoing requests for top-level modules, if they don't already have an integrity attribute. Canonical link: https://commits.webkit.org/279096@main

yoavweiss · 2024-05-29T09:12:25Z

Two weeks have almost passed :)
@beurdouche - any feedback before we merge this?

beurdouche · 2024-05-29T14:17:59Z

Two weeks have almost passed :) @beurdouche - any feedback before we merge this?

Hi Yoav, thanks, just emailed the stakeholders on our side to collect a final round of opinions.

domenic · 2024-05-30T02:06:29Z

Since all the requirements of our process have been met, I plan to merge this in ~24 hours.

yoavweiss marked this pull request as draft April 10, 2024 03:03

yoavweiss changed the title ~~Add integrity metadata to importmaps~~ Add subresource integrity support for ES modules, through importmaps Apr 15, 2024

yoavweiss marked this pull request as ready for review April 15, 2024 07:43

This was referenced Apr 15, 2024

Importmap integrity mozilla/standards-positions#1010

Open

Importmap integrity WebKit/standards-positions#335

Closed

domenic requested changes Apr 15, 2024

View reviewed changes

yoavweiss mentioned this pull request Apr 15, 2024

Importmap integrity w3ctag/design-reviews#944

Closed

1 task

yoavweiss commented Apr 15, 2024

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Apr 15, 2024

Subresource Integrity support for ES modules, using importmaps web-platform-tests/wpt#45638

Merged

yoavweiss requested a review from domenic April 15, 2024 16:17

hiroshige-g reviewed Apr 16, 2024

View reviewed changes

source Show resolved Hide resolved

domenic reviewed Apr 17, 2024

View reviewed changes

yoavweiss commented Apr 17, 2024

View reviewed changes

yoavweiss added 11 commits May 15, 2024 10:23

Review comments

46f4881

Add integrity check to non-imports

caa8fd7

conformance

e76e595

self-nits

378a725

remove spurious newline

2089578

Review comments

889da3a

Try to make WATTSI happy

ab8a46d

Serialize URLs in the map

e3ad1b7

Revert "Serialize URLs in the map"

5d90fbe

This reverts commit b4e94b6.

s/to/be/

2101f91

Rebase

5996082

yoavweiss force-pushed the import_map_integrity branch from 15726a7 to 5996082 Compare May 15, 2024 08:36

yoavweiss mentioned this pull request May 16, 2024

Upcoming WHATNOT meeting on 5/16/2024 #10340

Closed

past removed the agenda+ To be discussed at a triage meeting label May 16, 2024

yoavweiss mentioned this pull request May 22, 2024

Importmap integrity section mdn/content#33712

Merged

domenic added addition/proposal New features or enhancements topic: script labels May 31, 2024

domenic merged commit b2fdca1 into whatwg:main May 31, 2024
2 checks passed

yoavweiss mentioned this pull request Sep 11, 2024

Revive require-sri-for? w3c/webappsec#655

Open

bakkot mentioned this pull request Sep 28, 2024

dynamic import with import() lacks subresource integrity tc39/ecma262#3432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subresource integrity support for ES modules, through importmaps #10269

Add subresource integrity support for ES modules, through importmaps #10269

yoavweiss commented Apr 10, 2024 •

edited by pr-preview bot

Loading

domenic commented Apr 11, 2024

yoavweiss commented Apr 11, 2024

yoavweiss commented Apr 15, 2024

yoavweiss commented Apr 15, 2024

domenic left a comment

yoavweiss left a comment

hiroshige-g commented Apr 16, 2024

yoavweiss commented Apr 16, 2024

domenic commented Apr 16, 2024

guybedford commented Apr 16, 2024

domenic left a comment

domenic commented Apr 17, 2024

yoavweiss left a comment

yoavweiss commented Apr 17, 2024

yoavweiss commented Apr 17, 2024

domenic commented Apr 17, 2024

yoavweiss commented May 15, 2024

yoavweiss commented May 29, 2024

beurdouche commented May 29, 2024

domenic commented May 30, 2024

Add subresource integrity support for ES modules, through importmaps #10269

Add subresource integrity support for ES modules, through importmaps #10269

Conversation

yoavweiss commented Apr 10, 2024 • edited by pr-preview bot Loading

domenic commented Apr 11, 2024

yoavweiss commented Apr 11, 2024

yoavweiss commented Apr 15, 2024

yoavweiss commented Apr 15, 2024

domenic left a comment

Choose a reason for hiding this comment

yoavweiss left a comment

Choose a reason for hiding this comment

hiroshige-g commented Apr 16, 2024

yoavweiss commented Apr 16, 2024

domenic commented Apr 16, 2024

guybedford commented Apr 16, 2024

domenic left a comment

Choose a reason for hiding this comment

domenic commented Apr 17, 2024

yoavweiss left a comment

Choose a reason for hiding this comment

yoavweiss commented Apr 17, 2024

yoavweiss commented Apr 17, 2024

domenic commented Apr 17, 2024

yoavweiss commented May 15, 2024

yoavweiss commented May 29, 2024

beurdouche commented May 29, 2024

domenic commented May 30, 2024

yoavweiss commented Apr 10, 2024 •

edited by pr-preview bot

Loading