1858078 - Add CSP support to JSON modules

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Nicolò Ribaudo]

The HTML spec will be updated to use a different destination when fetching JSON and CSS modules: https://github.com/whatwg/html/pull/9486, https://github.com/whatwg/fetch/pull/1691

Tests are being written at https://github.com/web-platform-tests/wpt/pull/41665

As far as I know Firefox has not implemented yet import attributes, but I'm opening this issue to make sure that this changed is taken into account.

Comment 1

[Dan Minor [:dminor]]

Hi Jonatan, is this bug still relevant, or did we fix this as part of the main json modules work?

Comment 2

[Jonatan Klemets [:jon4t4n]]

Hi Dan,

I completely missed this bug. Thanks for the ping. Looks like we are failing some of the web platform tests added in the PR linked above related to rel="preload" as="json" and Request.destination.

I'll take a closer look.

Comment 3

[Dan Minor [:dminor]]

Thank you :)

Comment 4

[Jonatan Klemets [:jon4t4n]]

Attachment: WIP: Bug 1858078: WIP

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Jonathan:
This is the try result of wpt from your patch,
https://treeherder.mozilla.org/jobs?repo=try&revision=8206b54859d0f281b418fd757de1253c6ec71fd3
Can you check the failures in
/content-security-policy/connect-src/connect-src-json-import-blocked.sub.html
/fetch/api/request/destination/fetch-destination.https.html

I'll check the failures in /preload

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1858078 - WIP, Preload a JSON module

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1858078 - WIP update ini

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

try result: https://treeherder.mozilla.org/jobs?repo=try&revision=c40754040d7b19695f2a9af3070b4452becedbb1

/content-security-policy/connect-src/connect-src-json-import-blocked.sub.html: need to check why "securitypolicyviolation" event doesn't notify.
https://searchfox.org/mozilla-central/rev/b2d5ed9e8997e0c95c9243ffebd8573daade7eb3/testing/web-platform/tests/content-security-policy/connect-src/connect-src-json-import-blocked.sub.html#17

/preload/preload-csp.sub.html: Gecko fails on this test, and seems the problem is not just for import-attributes but also for the fetch() API.

Comment 9

[Jonatan Klemets [:jon4t4n]]

Thanks for the help.

I looked at the securitypolicyviolation issue, and as far as I can tell, we end up with aSendViolationReports == false here. So, we never end up calling AsyncReportViolation. If I remove the if, the test passes. We are also calling AsyncReportViolation from a few other places, so I need to take a look to see if the place I found is the correct one or if we should end up calling AsyncReportViolation somewhere else. However, that is a problem for tomorrow.

Comment 10

[Jonatan Klemets [:jon4t4n]]

Okay, found the real issue. I only added a single nsIContentPolicy::TYPE_JSON, and we used that for preload requests too. We need a second nsIContentPolicy for the preload.

Comment 11

[Jonatan Klemets [:jon4t4n]]

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=c44f7386fa2fb20b2e74ac3853e8fff9a98dc7d6

Comment 12

[Jonatan Klemets [:jon4t4n]]

I broke preload/supported-as-values.html last night. It should be fixed now. Let's see if the try push agrees with me.

https://treeherder.mozilla.org/jobs?repo=try&revision=2eecbd4f2d6cf8f6fe299db9bf2deaecd733bb59

Comment 13

[Jonatan Klemets [:jon4t4n]]

Ignore comment 12 from yesterday (broke another test case).

I am confused at the moment. The changes in D225695 (everything folded into a single patch to make it easier for now) work okay and only fail preload/supported-as-values.html. (This try run: https://treeherder.mozilla.org/jobs?repo=try&revision=c44f7386fa2fb20b2e74ac3853e8fff9a98dc7d6)

The thing I don't understand is the difference between fetch-destination.https.html and supported-as-values.html. Both create a link (<link href="..." rel="preload" as="json">) and add it to the DOM. The only difference is that one test case adds it to the head while the other adds it to the body. As far as I can tell, we end up in HTMLLinkElement::BindToTree in both cases, so I don't see why one should create a load and the other not.

And now, while writing this comment, I noticed if (node.as != "JSON") { resolve() } in the fetch-destination.https.html test case. I tried it in Chrome and accessing node.as right after node.as = "json" returns an empty string. I think this corresponds to this text in the spec: https://html.spec.whatwg.org/multipage/links.html#translate-a-preload-destination.

So, this is currently broken:

let node = document.createElement("link");
node.rel = "preload";
node.as = "json";
console.log(node.as); // Chrome = "", Firefox = "json"

That said, I still find the test case really confusing. Why even have the stuff below the if when the promise is resolved immediately?

I'll pick this back up again later this week/early next, but wanted to write this comment as a reminder to myself, or if someone else wants to take a look.

---

fetch-destination.https.html (https://searchfox.org/mozilla-central/rev/783f3fca1dda58353f7d3075744dd48b66e00e5e/testing/web-platform/tests/fetch/api/request/destination/fetch-destination.https.html#269-283)

// ...
let node = frame.contentWindow.document.createElement("link");
node.rel = "preload";
node.as = "json";
if (node.as != "json") {
  resolve();
}
node.onload = resolve;
node.onerror = reject;
node.href = "dummy.json?t=2&dest=json";
frame.contentWindow.document.body.appendChild(node);
// ...

supported-as-values.html (https://searchfox.org/mozilla-central/rev/783f3fca1dda58353f7d3075744dd48b66e00e5e/testing/web-platform/tests/preload/supported-as-values.html#18,29-40)

// ...
const link = document.createElement("link");
link.href = new URL("/common/echo.py?content=nothing", location.href).href;
link.rel = "preload";
link.as = as;
document.head.append(link);
await new Promise(resolve => {
    t.step_timeout(resolve, 1000);
    link.addEventListener("load", resolve);
    link.addEventListener("error", resolve);
});
const resources = performance.getEntriesByName(link.href);
assert_equals(resources.length, expected);
// ...

Comment 14

[Jonatan Klemets [:jon4t4n]]

I should have checked the status in other browsers before trying to make all tests pass.

preload/reflected-as-value.html is broken in all other browsers. If we don't return "json" when link.as is accessed, then the <link rel=preload as=json> test in fetch-destination.https.html passes because of the early resolve that I was confused about in comment 13.

preload/preload-type-match.html is also broken in all other browsers (json tests).

It feels like <link rel=preload as=json> is still up for discussion [1] and maybe will be replaced by modulepreload. In Chrome, <link rel="modulepreload" href="./test.json" as="json"> works but prints a warning in the console that "json" is not a valid as value. I may be misinterpreting the spec, but to my understanding modulepreload should only work for script-like destinations [2], and "json" is not script-like [3]. There is an open issue about adding as=json to modulepreload [4].

Maybe we should ignore the preload stuff for now and split this bug into two parts to get the CSP stuff landed and then fix the preload parts when the spec "issues" are resolved?

Yoshi, what do you think?

[1] https://github.com/whatwg/html/pull/10212
[2] https://html.spec.whatwg.org/multipage/links.html#link-type-modulepreload
[3] https://fetch.spec.whatwg.org/#request-destination-script-like
[4] https://github.com/whatwg/html/issues/10233

Comment 15

[Dan Minor [:dminor]]

I think if the test is broken in other browsers, we should just disable the test for now and file a follow up bug to fix it once the spec issues are resolved.

Comment 16

[Jonatan Klemets [:jon4t4n]]

Try: https://treeherder.mozilla.org/jobs?repo=try&revision=7ea4518cd019e9af9cc522fb9b13a1557bf3d571

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I don't know the why the if (node.as != "..") check are needed in fetch-destination-https.html, I'll also check that.
Feel free to split some work to a new bug. We can check whether the test itself or the spec is wrong.

(I assume you still want to take this bug? Phabicator set the assignee to me by accident?)

Comment 18

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1858078 - Part 1: Add nsIContentPolicy::{TYPE_JSON,TYPE_INTERNAL_JSON_PRELOAD} r=#devtools-reviewers!,#dom-core!,#necko-reviewers!,evilpie,#extension-reviewers!

Comment 19

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1858078 - Part 2: Return correct nsIContentPolicy when fetching json modules r=#dom-core!,#spidermonkey-reviewers

Comment 20

[Jonatan Klemets [:jon4t4n]]

Attachment: WIP: Bug 1858078 - Part 3: Set correct accept header when fetching JSON

Comment 21

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1858078 - Part 3: Update WPT expectations r=#spidermonkey-reviewers

Comment 22

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1858078 - Part 4: Add WPT for checking headers when fetching a JSON module r=evilpie

The main intent of this patch is to add test coverage for Sec-Fetch-Dest: json because an earlier patch in this stack added that functionality.

Comment 23

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1858078 - Part 5: Add JSON tests for webRequest.ResourceType r=robwu

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84c35d414bff
Part 1: Add nsIContentPolicy::{TYPE_JSON,TYPE_INTERNAL_JSON_PRELOAD} r=extension-reviewers,webidl,devtools-reviewers,dom-core,smaug,robwu,tschuster,nchevobbe,farre
https://hg.mozilla.org/integration/autoland/rev/51bc119bb24b
Part 2: Return correct nsIContentPolicy when fetching json modules r=spidermonkey-reviewers,dom-core,hsivonen,allstarschh
https://hg.mozilla.org/integration/autoland/rev/3c8813a04ff4
Part 3: Update WPT expectations r=spidermonkey-reviewers,allstarschh
https://hg.mozilla.org/integration/autoland/rev/6f85e1b1b058
Part 4: Add WPT for checking headers when fetching a JSON module r=tschuster
https://hg.mozilla.org/integration/autoland/rev/5a7f19a52182
Part 5: Add JSON tests for webRequest.ResourceType r=robwu
https://hg.mozilla.org/integration/autoland/rev/1d0c3134e782
apply code formatting via Lando

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/49662 for changes under testing/web-platform/tests

Comment 26

[amarc]

https://hg.mozilla.org/mozilla-central/rev/84c35d414bff
https://hg.mozilla.org/mozilla-central/rev/51bc119bb24b
https://hg.mozilla.org/mozilla-central/rev/3c8813a04ff4
https://hg.mozilla.org/mozilla-central/rev/6f85e1b1b058
https://hg.mozilla.org/mozilla-central/rev/5a7f19a52182
https://hg.mozilla.org/mozilla-central/rev/1d0c3134e782

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot