html
html copied to clipboard
whatwg
Add subresource integrity support for ES modules, through importmaps
Based on a proposal by @guybedford.
SRI support for ES module imports would enable using them in documents that require SRI for certain scripts for security or privacy reasons.
- [x] At least two implementers are interested (and none opposed):
- Chromium - I'm implementing this.
- WebKit - tentatively supportive, I'm implementing.
- [x] Tests are written and can be reviewed and commented upon at:
- [x] Implementation bugs are filed:
- Chromium: https://issues.chromium.org/issues/334251999
- Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1892199
- WebKit: https://bugs.webkit.org/show_bug.cgi?id=272884
- Deno: https://github.com/denoland/deno/issues/23521
- Node.js: https://github.com/nodejs/node/issues/52662
- [x] MDN issue is filed: https://github.com/mdn/mdn/issues/541
- [x] 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 )
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.)
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.
@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.
(Also, PR preview doesn't seem to update to the latest version for some reason)
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?
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?
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?
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.
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.
+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).
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?
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)?
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)?
Makes sense!
I think supporting the import map integrity for top-level script loads would be useful.
Changed spec, impl and tests to match that.
Two weeks have almost passed :) @beurdouche - any feedback before we merge this?
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.
Since all the requirements of our process have been met, I plan to merge this in ~24 hours.