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 - tentatively supportive
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

LGTM with some final nits. Hooray!

source

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

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

smaug---- · 2024-04-24T22:04:58Z

@mozfreddyb

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

annevk · 2024-04-30T06:32:10Z

source

+        attribute, then set <var>options</var>'s <span
+        data-x="concept-script-fetch-options-integrity">integrity metadata</span> to the result of
+        <span>resolving a module integrity metadata</span> with <var>url</var> and
+        <var>settings object</var>.</p>


Wouldn't it be cleaner for this to branch on options integrity metadata being the empty string? I guess it's technically different. So either way we should probably test what integrity="" plus import map integrity does.

I think this way would be clearer to devs, but can be convinced either way.
It is tested in https://chromium-review.googlesource.com/c/chromium/src/+/5441822 (lines 55 and 110)

Lines 55 and 110 of which file?

Apologies, I thought I linked to the file directly: https://chromium-review.googlesource.com/c/chromium/src/+/5441822/30/third_party/blink/web_tests/external/wpt/import-maps/nonimport-integrity.html

source

This reverts commit b4e94b6.

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 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

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}

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>

Add integrity metadata to importmaps

28534a4

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

yoavweiss added 6 commits April 13, 2024 11:10

Enforce module integrity

35f2e23

Only take in resolved URLs as integrity keys

50ff448

Split descendant script fetch options into its own algorithm

2ead28f

Speculative fix for conformance issues

d61e57c

Align with impl and vice-versa

899f1be

Define a valid integrity hash

3df60e7

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

Open

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

Address some review comments

4b1ef84

yoavweiss commented Apr 15, 2024

View reviewed changes

More review comments addressed

bf29ff8

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 Outdated Show resolved Hide resolved

Revamp the referencingScript related condition

5360deb

domenic added the agenda+ To be discussed at a triage meeting label Apr 22, 2024

domenic approved these changes Apr 24, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Outdated Show resolved Hide resolved

Review comments

febcce1

yoavweiss commented Apr 24, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Outdated Show resolved Hide resolved

This was referenced Apr 24, 2024

Add support for subresource integrity in imported module scripts denoland/deno#23521

Open

Add support for subresource integrity in imported module scripts nodejs/node#52662

Open

yoavweiss mentioned this pull request Apr 24, 2024

Add support for subresource integrity in imported module scripts mdn/mdn#541

Open

Try to make WATTSI happy

7c0ce99

past mentioned this pull request Apr 25, 2024

Upcoming WHATNOT meeting on 4/25/2024 #10287

Closed

annevk reviewed Apr 30, 2024

View reviewed changes

yoavweiss added 2 commits April 30, 2024 10:09

Serialize URLs in the map

b4e94b6

Revert "Serialize URLs in the map"

4a476c6

This reverts commit b4e94b6.

past mentioned this pull request May 3, 2024

Upcoming WHATNOT meeting on 5/2/2024 #10298

Closed

mozfreddyb reviewed May 3, 2024

View reviewed changes

source Show resolved Hide resolved

s/to/be/

15726a7

nicolo-ribaudo mentioned this pull request May 8, 2024

Get the base URL from the script rather than Module/Script Record #10332

Merged

past mentioned this pull request May 9, 2024

Upcoming WHATNOT meeting on 5/9/2024 #10318

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

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 left a comment

yoavweiss left a comment

smaug---- commented Apr 24, 2024

annevk Apr 30, 2024

yoavweiss Apr 30, 2024

annevk Apr 30, 2024

yoavweiss Apr 30, 2024

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

Are you sure you want to change the base?

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

Conversation

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

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 left a comment

Choose a reason for hiding this comment

yoavweiss left a comment

Choose a reason for hiding this comment

smaug---- commented Apr 24, 2024

annevk Apr 30, 2024

Choose a reason for hiding this comment

yoavweiss Apr 30, 2024

Choose a reason for hiding this comment

annevk Apr 30, 2024

Choose a reason for hiding this comment

yoavweiss Apr 30, 2024

Choose a reason for hiding this comment

yoavweiss commented Apr 10, 2024 •

edited by pr-preview bot