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 "json" destination for "connect-src" #611

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

nicolo-ribaudo
Copy link
Member

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

whatwg/fetch#1691 adds "json" destinatino to the fetch spec (see whatwg/html#9486 and whatwg/html#7233 for motivation). Given that JSON modules are powerless and they they are so far usually loaded using fetch(), the intention is for them to re-use the connect-src CSP policy.

fyi @annevk @domenic

Closes #573.

@nicolo-ribaudo
Copy link
Member Author

The linking errors seem to be unrelated to this change 🤔

➜ make           
bikeshed -f spec ./index.bs
LINE ~1628: Multiple possible 'url' dfn refs for '['/']'.
Arbitrarily chose https://url.spec.whatwg.org/#concept-url
To auto-select one of the following refs, insert one of these lines into a <pre class=link-defaults> block:
spec:url; type:dfn; text:url
spec:pub-manifest; type:dfn; text:url
[=/URL=]
LINE ~1630: Multiple possible 'url' dfn refs for '['/']'.
Arbitrarily chose https://url.spec.whatwg.org/#concept-url
To auto-select one of the following refs, insert one of these lines into a <pre class=link-defaults> block:
spec:url; type:dfn; text:url
spec:pub-manifest; type:dfn; text:url
[=/URL=]
LINK ERROR: Multiple possible 'url' dfn refs for '['/']'.
Arbitrarily chose https://url.spec.whatwg.org/#concept-url
To auto-select one of the following refs, insert one of these lines into a <pre class=link-defaults> block:
spec:url; type:dfn; text:url
spec:pub-manifest; type:dfn; text:url
[=/URL=]
LINE ~4023: Multiple possible 'url' dfn refs for '['/']'.
Arbitrarily chose https://url.spec.whatwg.org/#concept-url
To auto-select one of the following refs, insert one of these lines into a <pre class=link-defaults> block:
spec:url; type:dfn; text:url
spec:pub-manifest; type:dfn; text:url
[=/URL=]
WARNING: W3C policy requires Privacy Considerations and Security Considerations to be separate sections, but you appear to have them combined into one.
 ✔  Successfully generated, with 4 linking errors

@annevk
Copy link
Member

annevk commented Jul 25, 2023

I wonder why this setup doesn't automatically return that as an otherwise clause. Why would we want to fallback to no CSP enforcement? That seems wrong. (To be clear, not a question for OP, but rather for @antosart.)

@antosart
Copy link
Member

The compilation issue should be fixed by #613.

@antosart
Copy link
Member

antosart commented Jul 26, 2023

It would make sense to me that we always fall back to connect-src. Comparing https://fetch.spec.whatwg.org/#concept-request-destination with https://w3c.github.io/webappsec-csp/#effective-directive-for-a-request, all destinations are currently handled apart from "report". (Should CSP apply to CSP reports? I guess so, so that should also be fixed. EDIT: Vendors don't apply CSP to reports, and in fact that sounds totally fine sine report endpoints need to be specified in headers.)

@nicolo-ribaudo
Copy link
Member Author

Thanks @antosart! Do you think this PR should still be (rebased and) merged to have the explicit case, or should I close it given that now we have the default fallback in place?

@antosart
Copy link
Member

antosart commented Jul 31, 2023

@nicolo-ribaudo I think explicitly enumerating is better than relying on the fallback, so yes, let's keep this PR, thanks!

I'm not sure about the state of whatwg/fetch#1691, we should probably wait for that PR to be merged or ready to be merged.

@annevk
Copy link
Member

annevk commented Oct 29, 2023

The Fetch PR has been merged now @antosart.

@antosart
Copy link
Member

Thanks. Merging this now then!

@antosart antosart merged commit 9769609 into w3c:main Oct 30, 2023
github-actions bot added a commit that referenced this pull request Oct 30, 2023
SHA: 9769609
Reason: push, by antosart

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@nicolo-ribaudo nicolo-ribaudo deleted the json-destination branch October 30, 2023 08:22
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 <toyoshim@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Nicolò Ribaudo <nribaudo@igalia.com>
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS and JSON module scripts
3 participants