Add tests for CSS/JSON imports fetch destination and CSP #41665
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
requested review from
andypaicu,
annevk,
hillbrad,
jdm,
mnot,
snuggs,
yoavweiss,
youennf and
yutakahirano
This was referenced Aug 28, 2023
|
@antosart could you review these please as current CSP maintainer? Thanks! |
antosart
reviewed
Sep 4, 2023
| <html> | ||
|
|
||
| <head> | ||
| <meta http-equiv="Content-Security-Policy" content="connect-src 'self' http://{{host}}:{{ports[http][0]}}; script-src 'self' 'unsafe-inline';"> |
There was a problem hiding this comment.
isn't http://{{host}}:{{ports[http][0]}} the same as self?
There was a problem hiding this comment.
I think it's the same, at least when serving as http -- I copied it from various other tests in the same folder such as https://github.com/web-platform-tests/wpt/blob/master/content-security-policy/connect-src/connect-src-beacon-allowed.sub.html
Given that no implementation currently passes these tests, I can't really try what happens if I remove it.
There was a problem hiding this comment.
It's unfortunate that we cannot test it at the moment... But I would just drop this since I am pretty sure it does nothing more than 'self'.
content-security-policy/connect-src/connect-src-json-import-allowed.sub.html
Show resolved
Hide resolved
content-security-policy/connect-src/connect-src-json-import-blocked.sub.html
Show resolved
Hide resolved
| @@ -0,0 +1 @@ | |||
| {} | |||
There was a problem hiding this comment.
since you are reusing this file, I guess you can just put it in the top-level /common
There was a problem hiding this comment.
I now moved the other dummy.json files, but I left this one here because it needs to be in the correct scope for the service worker.
content-security-policy/style-src/import-declaration-style-allowed.sub.html
Outdated
Show resolved
Hide resolved
content-security-policy/style-src/import-declaration-style-blocked.sub.html
Outdated
Show resolved
Hide resolved
nicolo-ribaudo
force-pushed
the
import-json-css-destination
branch
from
2924187
to
918fd3f
Compare
nicolo-ribaudo
force-pushed
the
import-json-css-destination
branch
2 times, most recently
from
53782ef
to
ae17f88
Compare
nicolo-ribaudo
force-pushed
the
import-json-css-destination
branch
from
ae17f88
to
f784549
Compare
|
Thank you @antosart for the review and sorry that it took so long. This should be ready for a re-review now! |
antosart
reviewed
Oct 16, 2023
content-security-policy/connect-src/connect-src-json-import-allowed.sub.html
Outdated
Show resolved
Hide resolved
content-security-policy/connect-src/connect-src-json-import-blocked.sub.html
Outdated
Show resolved
Hide resolved
content-security-policy/style-src/import-style-allowed.sub.html
Outdated
Show resolved
Hide resolved
content-security-policy/style-src/import-style-blocked.sub.html
Outdated
Show resolved
Hide resolved
nicolo-ribaudo
commented
Oct 17, 2023
| <title>connect-src-json-import-allowed</title> | ||
| <meta | ||
| http-equiv="Content-Security-Policy" | ||
| content="connect-src 'self'; script-src http://{{host}}:{{ports[http][0]}}/resources/testharness.js http://{{host}}:{{ports[http][0]}}/resources/testharnessreport.js 'unsafe-inline';" |
There was a problem hiding this comment.
I updated this test so that script-src does not include self and thus the test will fail if a platform mistakenly uses script-src for JSON (or CSS) modules.
antosart
approved these changes
Oct 18, 2023
|
Maybe @annevk can help with the CI failure? :) |
sideshowbarker
merged commit a862c87
into
web-platform-tests:master
16 of 19 checks passed
annevk
pushed a commit
to whatwg/fetch
that referenced
this pull request
Oct 29, 2023
This is needed for whatwg/html#9486. Tests: web-platform-tests/wpt#41665.
annevk
pushed a commit
to whatwg/html
that referenced
this pull request
Oct 29, 2023
Builds on whatwg/fetch#1691. Tests: web-platform-tests/wpt#41665. Fixes #7233.
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Jan 19, 2024
The "Import Attributes" ECMAScript proposal has been updated to allow attributes to affect how modules are fetched, so that HTML can use them to set the proper destination when fetching CSS and JSON modules [1]. This has two major effects: - the `Accept` HTTP header is now specific to the module type, rather than simply being `*/*` - we use the relevant CSP policy rather than always script-src. This patch only implements this change for CSS modules. The change for JSON modules will come in a later patch, as it's more complex because it requires introducing a new fetch destination [2]. It passes the relevant wpt tests [3] when using --js-flags="--harmony_import_attributes. [1]: whatwg/html#9486 [2]: whatwg/fetch#1691 [3]: web-platform-tests/wpt#41665 Bug: 1491336 Change-Id: I4abf09dd828e5ded2b685be6da231c3fca90e19f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4949956 Reviewed-by: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Stephen Chenney <[email protected]> Reviewed-by: Takashi Toyoshima <[email protected]> Commit-Queue: Nicolò Ribaudo <[email protected]> Cr-Commit-Position: refs/heads/main@{#1249592}
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Jan 20, 2024
This patch adds a new `json` fetch destination [fetch-spec-pr], that has
the following characteristics:
- it implies the `Accept: application/json,*/*;q=0.5` HTTP header;
- it uses the `connect-src` CSP directive [csp-spec-pr].
This new destination is used when fetching JSON module scripts, using
`import ... from "/data" with { type: "json" }` [html-spec-pr].
https://crrev.com/c/4949956 implements a similar change for CSS module
scripts, but their implementation is simpler because the `style`
destination already exists.
This patch passes all the relevant WPT tests [wpt-pr] (when using
--js-flags="--harmony_import_attributes), although I had to run them
manually because they have not been merged yet.
This patch does not add support for `<link rel="preload" as="json">`,
which is also introduced by the linked fetch and HTML spec changes.
[fetch-spec-pr]: whatwg/fetch#1691
[csp-spec-pr]: w3c/webappsec-csp#611
[html-spec-pr]: whatwg/html#9486
[wpt-pr]: web-platform-tests/wpt#41665
Bug: 1491336
Change-Id: I6661ddc9be04935e2ee760eb78d1060ae0192a55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4955077
Reviewed-by: Takashi Toyoshima <[email protected]>
Reviewed-by: David Bertoni <[email protected]>
Reviewed-by: Hiroki Nakagawa <[email protected]>
Reviewed-by: Xinghui Lu <[email protected]>
Commit-Queue: Nicolò Ribaudo <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1249822}
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds tests for whatwg/html#9486 and whatwg/fetch#1691. I copied the style of other tests in the same directories.
cc @annevk