Skip to content
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

Add support for importmap integrity #28253

Merged
merged 1 commit into from
May 22, 2024

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented May 7, 2024

339bcec

Add support for importmap integrity
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

ac14677

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug ❌ πŸ§ͺ wpe-wk2 βœ… πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
❌ πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-cairo
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress ❌ πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ›  jsc-armv7
βœ… πŸ›  watch-sim ❌ πŸ§ͺ jsc-armv7-tests

@yoavweiss yoavweiss requested a review from a team as a code owner May 7, 2024 20:55
@yoavweiss
Copy link
Contributor Author

Apologies for my noobness, but my git-webkit script didn't seem to do the right thing when uploading this.
Also, I still need to update the test expectations for the new tests.

@Ahmad-S792 Ahmad-S792 added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label May 7, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 7, 2024
@yoavweiss yoavweiss changed the title Update importmap tests and spec references Add test expectations May 8, 2024
@yoavweiss yoavweiss changed the title Add test expectations Update importmap tests and spec references May 8, 2024
@yoavweiss yoavweiss marked this pull request as draft May 13, 2024 05:32
@yoavweiss yoavweiss changed the title Update importmap tests and spec references Add support for importmap integrity May 13, 2024
Source/JavaScriptCore/runtime/ImportMap.cpp Outdated Show resolved Hide resolved
FAIL Modulepreload was not loaded as its integrity check was not ignored assert_unreached: Should have rejected: undefined Reached unreachable code
PASS Modulepreload was loaded as its correct integrity attribute was not ignored
PASS Modulepreload was loaded as its empty integrity attribute was not ignored
FAIL Modulepreload was not loaded as its bad integrity attribute was not ignored assert_unreached: Should have rejected: undefined Reached unreachable code
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails because module preload integrity is not supported.

@@ -29,6 +29,10 @@ PASS Script load (url:https://localhost:9443/service-workers/service-worker/reso
PASS Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test27)
PASS Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test28)
PASS Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test29)
PASS Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_module)
FAIL Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_modulepreload) assert_equals: integrity of Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_modulepreload) must be sha384-foobar. expected "sha384-foobar" but got ""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails because of modulepreload integrity is not supported in this PR.

@@ -29,6 +29,10 @@ PASS Script load (url:https://localhost:9443/service-workers/service-worker/reso
PASS Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test27)
PASS Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test28)
PASS Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test29)
PASS Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_module)
FAIL Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_modulepreload) assert_equals: integrity of Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_modulepreload) must be sha384-foobar. expected "sha384-foobar" but got ""
FAIL Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_moduleimport) assert_equals: integrity of Module Script load (url:https://localhost:9443/service-workers/service-worker/resources/sample?test_moduleimport) must be sha384-foobar. expected "sha384-foobar" but got ""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails because of the way I implemented static import integrity (only on the response side)

@@ -90,7 +90,67 @@
destination: 'script',
message: `Script load (url:${actual_url})`
};
frame.contentWindow.load_script_with_integrity(actual_url, integrity);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a manual import, as importing the entire service-workers directory introduced too much unrelated noise

rejectWithFetchError(*m_context, WTFMove(promise), ExceptionCode::TypeError, "Cannot load script due to integrity mismatch"_s);
return;
}
String integrity = downcast<Document>(*m_context).globalObject()->importMap().getIntegrity(sourceURL);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enforces integrity checks on the response even in cases where the integrity value is not propagated through the request (static imports).

@yoavweiss yoavweiss marked this pull request as ready for review May 14, 2024 06:22
@yoavweiss
Copy link
Contributor Author

There are still a few open questions (as comments), but I'd love an initial review of the approach.

Copy link
Contributor Author

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing!

Source/JavaScriptCore/runtime/ImportMap.cpp Outdated Show resolved Hide resolved
Source/JavaScriptCore/runtime/ImportMap.h Outdated Show resolved Hide resolved
Source/JavaScriptCore/runtime/ImportMap.h Outdated Show resolved Hide resolved
Source/WebCore/dom/ScriptElement.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-reviewer r+ on the tests, with that additional one I suggested.

@marcoscaceres
Copy link
Contributor

@yoavweiss, ping me if/when you need me to add it to the merge queue.

@marcoscaceres marcoscaceres added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 22, 2024
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
@webkit-commit-queue
Copy link
Collaborator

Committed 279096@main (339bcec): https://commits.webkit.org/279096@main

Reviewed commits have been landed. Closing PR #28253 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 22, 2024
@webkit-commit-queue webkit-commit-queue merged commit 339bcec into WebKit:main May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
7 participants