Skip to content

Add "json" destination for "connect-src"#611

Merged
antosart merged 1 commit intow3c:mainfrom
nicolo-ribaudo:json-destination
Oct 30, 2023
Merged

Add "json" destination for "connect-src"#611
antosart merged 1 commit intow3c:mainfrom
nicolo-ribaudo:json-destination

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented Jul 25, 2023 •
edited
Loading

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
Copy Markdown
Member Author

nicolo-ribaudo commented Jul 25, 2023

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

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jul 25, 2023
Merged
7 tasks
@annevk
Copy link
Copy Markdown
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
Copy Markdown
Member

antosart commented Jul 26, 2023

The compilation issue should be fixed by #613.

@antosart
Copy link
Copy Markdown
Member

antosart commented Jul 26, 2023 •
edited
Loading

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.)

@antosart antosart mentioned this pull request Jul 27, 2023
Merged
@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Jul 31, 2023

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
Copy Markdown
Member

antosart commented Jul 31, 2023 •
edited
Loading

@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
Copy Markdown
Member

annevk commented Oct 29, 2023

The Fetch PR has been merged now @antosart.

@antosart
Copy link
Copy Markdown
Member

antosart commented Oct 30, 2023

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
@nicolo-ribaudo @github-actions
Add "json" destination for "connect-src" (#611)
e8e51ed 
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
@nicolo-ribaudo
Implement "json" fetch destination for JSON module scripts
7eed8ca 
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

Reviewers

@antosart antosart antosart approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CSS and JSON module scripts

3 participants

@nicolo-ribaudo @annevk @antosart