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

Use appropriate fetch destination for JSON&CSS modules #9486

Merged

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Jul 4, 2023

Fixes #7233


I'm working on #7233, to bring the last import-attributes-related pieces to the finishing line.

The behavior for CSS modules is easy to define, as it should match how CSS files are loaded in other places: the fetch destination should be "style", which implies that:

On the other hand, I believe there is currently no precedent for loading "generic" (i.e. not something like manifest.json) JSON files, and as such it's less clear what the behavior should be. This PR sets the destination to "" and hard-codes the Accept: application/json,*/*;q=0.5 header (*), and as a consequence:
whatwg/fetch#1691 introduces "json" destination in fetch, so that:

  • JSON modules are fetches with the Accept: application/json,*/*;q=0.5 header (the */* has a q value higher than for CSS modules because there are actually many valid JSON mime types, but we cannot cover them with a glob pattern because it's not possible to indicate "it must end with +json")
  • they use the connect-src CSP policy instead of script-src (Add "json" destination for "connect-src" w3c/webappsec-csp#611), similar to JSON loaded using fetch.

(*) the list of valid JSON mime types is potentially unbounded, since it includes any mime type that ends in +json (ref: JSON mime types). This Accept: is currently hard-coded in HTML, but maybe it should be moved to Fetch by defining a new "json" fetch destination.

Additionally, I need some clarification on the mode of requests for modules coming from workers&friends (related: #3656). The current logic sets the request's mode to same-origin if the destination is worker/sharedworker/serviceworker. This PR changes the destination for CSS/JSON modules, and I'm not sure if it should also affect the request mode or not. (Answer: no - #9486 (comment))

TODO (other specs):


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

@domenic
Copy link
Member

domenic commented Jul 12, 2023

@annevk knows more about Fetch than me, but here's my initial stab at some answers:

I think a destination of "" for JSON is reasonable. If we were to invent a JSON destination, I wonder if it would conflict with any existing practice, or whether it'd make a lot of sense given that most JSON fetches are via fetch()...

If we were to invent a JSON destination, with regard to the Accept header, https://httpwg.org/specs/rfc7231.html#header.accept says

The asterisk "*" character is used to group media types into ranges, with "*/*" indicating all media types and "type/*" indicating all subtypes of that type.

which is a bit unclear to me. Are only the forms type/* and */* acceptable? Or is * a generic wildcard, so we could do application/*+json? I guess a question is whether deployed software can cope, and here's at least one example that can't.

I don't think mode should change. Mode and destination are orthogonal, as far as I know?

I don't think the "Should response to request be blocked due to nosniff?" algorithm is affected. We perform our own custom MIME type checking in HTML, separate from the nosniff infrastructure. Now that we're setting the destination, that checking will sometimes be redundant, but sometimes it won't be. (In particular, our checking always happens, whereas the Fetch spec's checking only happens if X-Content-Type-Options: nosniff is set.)

@nicolo-ribaudo
Copy link
Contributor Author

The question should not be "will servers recognize that application/*+json matches application/xyz+json?" but "will servers fail to serve the response if they receive application/*+json in the list of accepted mime types?". In the @hapi/accept example, application/json, application/*+json, text/json, */* is not worse than just application/json, text/json, */*. The unrecognized pattern is simply ignored.

Reading the linked RFC, only going through the formal syntax definitions and ignoring the prose, I believe that application/*+json is valid however it is an exact mime-type, and not application/<anything>+json (* is a valid character for token, used for subtype: https://httpwg.org/specs/rfc7230.html#field.components). So servers would probably not fail, because Accept: application/*+json is a perfectly valid header, however we would be misusing the mime of that mime type.

(Note: I would love to use application/*+json if it is considered acceptable)

@clshortfuse
Copy link

clshortfuse commented Jul 14, 2023

Have you checked out https://datatracker.ietf.org/doc/html/rfc6838#section-4.2 ?

It says subtypes must start with ALPHA / DIGIT and the +suffix is still part of the subtype.

Servers may be parsing with that in consideration.

@nicolo-ribaudo nicolo-ribaudo force-pushed the css-json-modules-fetch-destination branch from 38059ed to d27d13a Compare July 21, 2023 12:03
@nicolo-ribaudo
Copy link
Contributor Author

I added a commit to use Accept: application/json,text/json,*/*;q=0.5 in JSON modules, since:

  • if the response is a non-JSON file, it will throw
  • CSS modules already send a proper Accept header

I do not have strong opinions regarding what exactly this Accept header should be and what the q parameters should be, other than:

  • it should hint to the server that it expects some sort of JSON
  • it should include some glob to allow any JSON MIME type (such as application/geo+json).

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review July 21, 2023 12:10
@clshortfuse
Copy link

clshortfuse commented Jul 21, 2023

On the q aspect:

Since */* is equal (0.5) and is "less specific" it will should never pass over to more (unspecified) specific mime type unless the server doesn't support sending application/json or text/json. That means even if the server is more equipped to send a more specific mime type (eg: application/geo+json), it won't.

If more than one media range applies to a given type, the most specific reference has precedence.

https://datatracker.ietf.org/doc/html/rfc7231#section-5.3.2

It would be nice to prefer a more specific type of JSON, but I'm not sure how feasible that is, since that brings back the */*+json type. Allowing a custom accept might be something in later like import("foo.json", { with: { type: "json", accept: "application/ld+json" } });. It is possible a client environment may prefer a specific format over another, even if both are JSON.

If */* gets higher preference it might just send application/xml which we don't want, so it's right to not be higher. We want to make sure that if a server prefers to send application/xml for /users/bob, it won't. Servers should never do this because less specificity, but all the same, fetch has: image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5 to request that we much rather get an image/* over */* in a more explicit manner.

When specificity is equal (application/json and text/json), then order is not given priority and the server decides. I lean on application/json being higher since it's more common, the registered one, and meant for processing. text/* implies a human readable version that can be presented as is, which technically means you care about whitespace. Maybe there's a server that has a text/json version available that "beautifies" the JSON.

So maybe application/json;q=0.7,text/json;q=0.6,*/*;0.5

Edit: IIRC, the starting number we base it on (0.5) is when the server decides what % quality can be lost from the original representation of the resource. For example, if the original resource is XML and there's something in the conversion to JSON that gets lost, an at least 50% degradation of quality is acceptable. Any lower than that, and then don't send anything and throw send a 4xx status (I forget which one). The number here is rather opinionated, text/css is 0.1, whereas HTML is 0.9. I don't know what the right number is (or if servers commonly care).

@nicolo-ribaudo
Copy link
Contributor Author

Thank you for the detailed analysis! I made the priorities 1-0.9-0.5, since all the other examples I could find in the fetch spec start from 1.

Since / is equal (0.5) and is "less specific" it will should never pass over to more (unspecified) specific mime type unless the server doesn't support sending application/json or text/json. That means even if the server is more equipped to send a more specific mime type (eg: application/geo+json), it won't.

Given that all the +json formats are handled the same as application/json by browsers this is in practice ok, even if I really wish there was a way to better specify this. I like the idea of potentially adding in the future an accept attribute, if there will be a need for it.

@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented Jul 21, 2023

@annevk @zcorpan What do you think of this change to resolve #7233?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this, much appreciated!

source Outdated
<p class="note"><span data-x="concept-fetch">Fetch</span> sets the (`<code
data-x="http-accept">Accept</code>`, `<code data-x="">text/css,*/*;q=0.1</code>`) header for CSS
modules.</p>
</li>
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to add a "json" destination and keep Fetch responsible for the Accept header. CSP treats "json" the same as the empty string I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Contributor Author

While doing the changes in the fetch spec, I noticed that this would also allow <link rel="preload" as="json" href="...">.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Making preload work seems like a very good thing.

source Outdated Show resolved Hide resolved
@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Jul 25, 2023
@annevk
Copy link
Member

annevk commented Jul 25, 2023

As with whatwg/fetch#1691 I think we can count Chromium and WebKit as supportive based on statements in #7233, but I'll tag some people here just in case.

cc @Constellation @syg @hiroshige-g

@annevk
Copy link
Member

annevk commented Aug 25, 2023

@nicolo-ribaudo any progress on tests and implementation bugs?

@nicolo-ribaudo
Copy link
Contributor Author

I'll write them next week

@annevk annevk added topic: script and removed needs tests Moving the issue forward requires someone to write tests labels Oct 29, 2023
annevk pushed a commit to whatwg/fetch that referenced this pull request Oct 29, 2023
@annevk annevk merged commit 37659e9 into whatwg:main Oct 29, 2023
2 checks passed
@annevk
Copy link
Member

annevk commented Oct 29, 2023

Thank you @nicolo-ribaudo for resolving this longstanding issue with non-JavaScript modules!

@nicolo-ribaudo nicolo-ribaudo deleted the css-json-modules-fetch-destination branch October 30, 2023 08:23
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}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

CSS and JSON module scripts and CSP
4 participants