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 inside settings to fetch the descendants of module worker in the worker module script graph #9571

Open
allstarschh opened this issue Jul 31, 2023 · 18 comments

Comments

@allstarschh
Copy link

(This was the idea from @codehag and @smaug---- when they implemented module workers in Firefox,
but @codehag is on personal leave so I file the spec issue on her behalf.)

When fetch a module worker script graph, the specification uses the same environment settings object, a.k.a fetchClient to fetch the top-level module worker script and its descendants. To put it short, the specification requires to use the document's CSP to fetch the top-level module worker script and its descendants (static import).

The behavior is tested in WPT/dedicated-worker-import-csp.html

From @codehag and @smaug---- 's point of view, after the top-level module worker script is fetched, the worker will initialize its CSP list through the HTTP response, so it makes sense to use the worker's own CSP to fetch the descendants of the module worker. Also, this will align worker's static import with worker's dynamic import/importScripts(), all of them will use worker's CSP.

As mentioned above, this behavior is tested in interop-2023 of WPT, but Chrome/Firefox/Webkit have different behavior on this,
https://wpt.fyi/results/workers/modules/dedicated-worker-import-csp.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-modules

So I filed an issue here to open a discussion about this subject.

CC @domenic on the editor of this HTML spec
@hiroshige-g on the Chrome team
@Constellation on the Webkit team
@codehag @smaug---- @jonco3 and @zcorpan on the Firefox team

@domenic
Copy link
Member

domenic commented Aug 1, 2023

Making this into a two-stage fetch that behaves half one way, and half another, doesn't make any sense to me.

@jonco3
Copy link

jonco3 commented Aug 1, 2023

The benefits of this approach are:

  • the worker's client doesn't need to know what modules it uses as it only need to allow the top level script in its CSP
  • both static and dynamic module imports use the same CSP

Making this into a two-stage fetch that behaves half one way, and half another, doesn't make any sense to me.

Sure, the first fetch by necessity is different but everything after that uses the same settings. Having static and dynamic imports behave differently doesn't really make sense either.

@domenic
Copy link
Member

domenic commented Aug 1, 2023

Having static and dynamic imports diverge makes perfect sense! The former is part of the initialization, initiated by the outside, and is done before the worker even exists (in some implementations, before the thread has even been spun up or the global object allocated). The latter is initiated by the inside, after initialization.

In general, the outer page is initiating the initial graph fetch, and its CSP and security policies should apply. The inner worker is initiating dynamic imports, and so its security policies should apply.

Trying to say that the worker code initiated the static imports seems incorrect, in that sense. It's the outer page that kicked it off.

@jonco3
Copy link

jonco3 commented Aug 1, 2023

These sound like implementation details though. I think authors of worker code will be surprised if static and dynamic imports work differently.

Trying to say that the worker code initiated the static imports seems incorrect

In order to present a simpler conceptual model to web authors IMO it does makes more sense for this to behave as if the top level module is loading its static imports, even if that's not how it works internally.

@domenic
Copy link
Member

domenic commented Aug 1, 2023

I think the authors of outer-page code will be surprised if the first fetch they kick off follow different security policies from the rest of the ones that follow it.

Indeed, many web developers are used to an isomorphism: bundled code behaves the same as unbundled code. You are proposing to break that. So if anyone is presenting a more complex mental model, I believe it's your proposal.

@jonco3
Copy link

jonco3 commented Aug 1, 2023

There is only be a single fetch when the worker code is bundled, and this would be unchanged. In unbundled code static imports would use a different CSP. I don't think this makes bundled code behave differently from unbundled code any more than it does already by virtue of that fact that it doesn't make any subsequent fetches.

@zcorpan
Copy link
Member

zcorpan commented Aug 17, 2023

I can see that it may be surprising to some that there's a difference between child fetches in the initial module graph and dynamic imports, but still, I agree with @domenic. Especially this I think is compelling:

Indeed, many web developers are used to an isomorphism: bundled code behaves the same as unbundled code.

Also, as a data point, importScripts() uses the worker's settings object, and AFAIK we haven't heard complaints about it.

@annevk
Copy link
Member

annevk commented Aug 17, 2023

Making this into a two-stage fetch that behaves half one way, and half another, doesn't make any sense to me.

However, this is what we do for referrers, presumably? The top-level file is after all what is referring the imported files. And Origin doesn't end up differing because we require things to be same origin. What if we didn't require same origin, though? Not entirely clear to me who should carry the authority in that case. I wonder if @wanderview thought about this case.

@annevk
Copy link
Member

annevk commented Aug 31, 2023

@youennf presented me with a case for the two-stage fetch in line with my thinking about authority above.

In particular, what if the top-level file is a data: URL? In that case you would not want the scripts it references to be fetched with the authority of the parent document. In much the same way as you would not want that to happen with a data: URL <iframe>. As such, while the data: URL top-level file is fetched with the authority of the parent document, any files the data: URL top-level file referenced should be fetched with the authority of an opaque origin.

cc @nicolo-ribaudo

@annevk annevk added the agenda+ To be discussed at a triage meeting label Aug 31, 2023
@domenic
Copy link
Member

domenic commented Sep 1, 2023

In particular, what if the top-level file is a data: URL? In that case you would not want the scripts it references to be fetched with the authority of the parent document.

Why not?

When I type

import 'data:text/javascript,import "./foo.html"';

in my parent document, I think it's the parent document that can be said to be fetching foo.html.

@annevk
Copy link
Member

annevk commented Sep 1, 2023

In that case you don't establish a new environment. But with a worker you do. And the environment that is established is that of an opaque origin. But then somehow that opaque origin is fetching resources with the authority (i.e., cookies, service worker(maybe not, given that matches on the URL?), no CORS) of the parent document. I think that goes against expectations. It would make sense if a data: URL worker got the origin of the parent document, but we decided against that.

@domenic
Copy link
Member

domenic commented Sep 1, 2023

I think my argument still makes sense with a worker.

const w = new Worker('data:text/javascript,import "./foo.js"', { type: "module" });

I think the outer environment is what is causing foo.js to be fetched.

@annevk
Copy link
Member

annevk commented Sep 1, 2023

@domenic and if it was not a data: URL but a cross-origin URL? I just don't think the argument holds. And where you previously might have thought that a data: URL worker was safe, with module workers suddenly the data: URL itself needs to be vetted. That doesn't seem sound.

@youennf
Copy link

youennf commented Sep 13, 2023

I do not really see why we would treat workers and iframes differently in how they handle some of their subresources.
Consistency will gain us simplicity here.

For instance, if two contexts create the same shared worker at the same time, there is an inherent race on how/where the shared worker main script will be fetched.
We could avoid those differences when fetching children scripts given we have the shared worker context.

Also, from a service worker perspective, the shared worker main script fetch clientId is the ID of the context creating the shared worker object.
We do not want to use the same ID for shared worker children script fetches given the service worker might already know that this context is gone when these fetches are exposed to the service worker. Using the ID of the shared worker context does not expose this potential inconsistency.

@past past removed the agenda+ To be discussed at a triage meeting label Sep 15, 2023
@zcorpan
Copy link
Member

zcorpan commented Sep 19, 2023

After reviewing the above comments and discussing with @smaug---- I've changed my mind and think it's better to make static and dynamic imports' subresources both use the worker's settings object, since it's a new environment.

The bundled/unbundled expectation may be broken, but it already doesn't always apply. For example the top-level CSP might allow only some origins, and bundling code would make everything use one origin.

allstarschh added a commit to allstarschh/wpt-metadata that referenced this issue Sep 21, 2023
…dules

Fixing web-platform-tests/interop#406

The test [dedicated|shared]-worker-import-csp.html will inject CSP
headers on the top-level document for static import.

https://github.com/web-platform-tests/wpt/blob/256877037fa53c4c90e48e7171a69ea0cef0d3be/workers/modules/dedicated-worker-import-csp.html#L21

And the top-level module worker script will 'static import' its
descendants.
https://github.com/web-platform-tests/wpt/blob/master/workers/modules/resources/static-import-remote-origin-script-worker.sub.js

But due to the spec issue whatwg/html#9571
There isn't an agreement between browser vendors on which [settings object](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)
to use when fetching the descendants of module worker.

https://wpt.fyi/results/workers/modules/dedicated-worker-import-csp.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/shared-worker-import-csp.html?label=experimental&label=master&aligned

So removing these two tests from interop-2023-modules.
(According to the README.md in wpt-metadata, when a 'label' is used,
'subtest' is omitted, so I remove these two files from
interop-2023-modules completely)
allstarschh added a commit to allstarschh/wpt-metadata that referenced this issue Sep 21, 2023
…om interop-2023-modules

Fixing web-platform-tests/interop#406

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

https://github.com/web-platform-tests/wpt/blob/master/workers/modules/dedicated-worker-import-data-url-cross-origin.html#L13

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

https://github.com/web-platform-tests/wpt/blob/master/workers/modules/resources/static-import-script-block-cross-origin.js

The script "export-block-cross-origin.js" is same-origin with the
document, but not same-origin with the worker script which is loaded by
data: URI.

Due to the spec issue, whatwg/html#9571
There isn't an agreement between browser vendors on which [settings object](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)
to use when fetching the descendants of module worker.

https://wpt.fyi/results/workers/modules/dedicated-worker-import-data-url-cross-origin.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/shared-worker-import-data-url-cross-origin.html?label=experimental&label=master&aligned

So removing these two tests from interop-2023-modules.
(According to the README.md in wpt-metadata, when a 'label' is used,
'subtest' is omitted, so I remove these two files from
interop-2023-modules completely)
allstarschh added a commit to allstarschh/wpt-metadata that referenced this issue Sep 21, 2023
…worker-import-data-url.window.html from interop-2023-modules

Fixing web-platform-tests/interop#406

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

https://github.com/web-platform-tests/wpt/blob/2cd449b9df6f92862abac0143ac6fe674fd5278e/workers/modules/resources/import-test-cases.js#L13
https://github.com/web-platform-tests/wpt/blob/2cd449b9df6f92862abac0143ac6fe674fd5278e/workers/modules/resources/static-import-redirect-worker.js#L3

The script "export-on-load-script.js" is same-origin with the
document, but not same-origin with the worker script which is loaded by
data: URI.

Due to the spec issue, whatwg/html#9571
There isn't an agreement between browser vendors on which [settings object](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)
to use when fetching the descendants of module worker.

https://wpt.fyi/results/workers/modules/dedicated-worker-import-data-url.any.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/dedicated-worker-import-data-url.any.worker.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/shared-worker-import-data-url.window.html?label=experimental&label=master&aligned

So removing these three tests from interop-2023-modules.
(According to the README.md in wpt-metadata, when a 'label' is used,
'subtest' is omitted, so I remove these three files from
interop-2023-modules completely)
nt1m pushed a commit to web-platform-tests/wpt-metadata that referenced this issue Sep 21, 2023
…dules

Fixing web-platform-tests/interop#406

The test [dedicated|shared]-worker-import-csp.html will inject CSP
headers on the top-level document for static import.

https://github.com/web-platform-tests/wpt/blob/256877037fa53c4c90e48e7171a69ea0cef0d3be/workers/modules/dedicated-worker-import-csp.html#L21

And the top-level module worker script will 'static import' its
descendants.
https://github.com/web-platform-tests/wpt/blob/master/workers/modules/resources/static-import-remote-origin-script-worker.sub.js

But due to the spec issue whatwg/html#9571
There isn't an agreement between browser vendors on which [settings object](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)
to use when fetching the descendants of module worker.

https://wpt.fyi/results/workers/modules/dedicated-worker-import-csp.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/shared-worker-import-csp.html?label=experimental&label=master&aligned

So removing these two tests from interop-2023-modules.
(According to the README.md in wpt-metadata, when a 'label' is used,
'subtest' is omitted, so I remove these two files from
interop-2023-modules completely)
nt1m pushed a commit to web-platform-tests/wpt-metadata that referenced this issue Sep 21, 2023
…om interop-2023-modules

Fixing web-platform-tests/interop#406

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

https://github.com/web-platform-tests/wpt/blob/master/workers/modules/dedicated-worker-import-data-url-cross-origin.html#L13

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

https://github.com/web-platform-tests/wpt/blob/master/workers/modules/resources/static-import-script-block-cross-origin.js

The script "export-block-cross-origin.js" is same-origin with the
document, but not same-origin with the worker script which is loaded by
data: URI.

Due to the spec issue, whatwg/html#9571
There isn't an agreement between browser vendors on which [settings object](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)
to use when fetching the descendants of module worker.

https://wpt.fyi/results/workers/modules/dedicated-worker-import-data-url-cross-origin.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/shared-worker-import-data-url-cross-origin.html?label=experimental&label=master&aligned

So removing these two tests from interop-2023-modules.
(According to the README.md in wpt-metadata, when a 'label' is used,
'subtest' is omitted, so I remove these two files from
interop-2023-modules completely)
nt1m pushed a commit to web-platform-tests/wpt-metadata that referenced this issue Sep 21, 2023
…worker-import-data-url.window.html from interop-2023-modules

Fixing web-platform-tests/interop#406

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

https://github.com/web-platform-tests/wpt/blob/2cd449b9df6f92862abac0143ac6fe674fd5278e/workers/modules/resources/import-test-cases.js#L13
https://github.com/web-platform-tests/wpt/blob/2cd449b9df6f92862abac0143ac6fe674fd5278e/workers/modules/resources/static-import-redirect-worker.js#L3

The script "export-on-load-script.js" is same-origin with the
document, but not same-origin with the worker script which is loaded by
data: URI.

Due to the spec issue, whatwg/html#9571
There isn't an agreement between browser vendors on which [settings object](https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object)
to use when fetching the descendants of module worker.

https://wpt.fyi/results/workers/modules/dedicated-worker-import-data-url.any.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/dedicated-worker-import-data-url.any.worker.html?label=experimental&label=master&aligned
https://wpt.fyi/results/workers/modules/shared-worker-import-data-url.window.html?label=experimental&label=master&aligned

So removing these three tests from interop-2023-modules.
(According to the README.md in wpt-metadata, when a 'label' is used,
'subtest' is omitted, so I remove these three files from
interop-2023-modules completely)
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 26, 2023
…d be allowed.] failed as expected in [shared|dedicated]-worker-import-data-url-cross-origin.html.ini. r=dom-worker-reviewers,asuth

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

According to the current spec, the script "export-block-cross-origin.js"
is in the same origin of the document, therefore it should be loaded.

But with Gecko's implementation, the script "export-block-cross-origin.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187900
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 26, 2023
…cted in dedicated-worker-import-data-url.any.js and shared-worker-import-data-url.window.js. r=dom-worker-reviewers,asuth

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

According the current spec, the redirected 'export-on-load-script.js' is
in the same-origin with the document, therefore it should be loaded.

But with Gecko's implementation, the redirected script "export-on-load-script.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

(The first test case, which tests static import without redirect, the
script 'export-on-load-script.js' could be loaded by Gecko because the
script has the CORS header.)

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187901
@nyaxt
Copy link
Member

nyaxt commented Sep 26, 2023

I discussed this with Chromium team members (@nhiroki, @hiroshige-g, @yoshisatoyanagisawa and @domenic).

We prefer the current spec, since it enables faster performance (and simplicity of the Chromium code).

Chromium has an optimization (called PlzWorker) which initiates the module tree fetches outside of the renderer process. This plays nicer with the current spec, which is to use the document fetch client. With the proposed change, we would need to introduce a synchronization point, where we would have the worker context ready for CSP, and then process the submodule fetches. This is still implementable, but it will come with additional complexity and also performance hit.

aosmond pushed a commit to aosmond/gecko that referenced this issue Sep 27, 2023
…d be allowed.] failed as expected in [shared|dedicated]-worker-import-data-url-cross-origin.html.ini. r=dom-worker-reviewers,asuth

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

According to the current spec, the script "export-block-cross-origin.js"
is in the same origin of the document, therefore it should be loaded.

But with Gecko's implementation, the script "export-block-cross-origin.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187900
aosmond pushed a commit to aosmond/gecko that referenced this issue Sep 27, 2023
…cted in dedicated-worker-import-data-url.any.js and shared-worker-import-data-url.window.js. r=dom-worker-reviewers,asuth

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

According the current spec, the redirected 'export-on-load-script.js' is
in the same-origin with the document, therefore it should be loaded.

But with Gecko's implementation, the redirected script "export-on-load-script.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

(The first test case, which tests static import without redirect, the
script 'export-on-load-script.js' could be loaded by Gecko because the
script has the CORS header.)

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187901
@smaug----
Copy link

Some Chromium internal optimization isn't really a good reason for the spec to behave certain way.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Sep 29, 2023
…d be allowed.] failed as expected in [shared|dedicated]-worker-import-data-url-cross-origin.html.ini. r=dom-worker-reviewers,asuth

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

According to the current spec, the script "export-block-cross-origin.js"
is in the same origin of the document, therefore it should be loaded.

But with Gecko's implementation, the script "export-block-cross-origin.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187900

UltraBlame original commit: 6e401be8c58633271e8a0813f2b9e98df1d5af8e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Sep 29, 2023
…cted in dedicated-worker-import-data-url.any.js and shared-worker-import-data-url.window.js. r=dom-worker-reviewers,asuth

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

According the current spec, the redirected 'export-on-load-script.js' is
in the same-origin with the document, therefore it should be loaded.

But with Gecko's implementation, the redirected script "export-on-load-script.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

(The first test case, which tests static import without redirect, the
script 'export-on-load-script.js' could be loaded by Gecko because the
script has the CORS header.)

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187901

UltraBlame original commit: 460e36a5e5aaca8ac8102a05e4357315e2c18583
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Sep 29, 2023
…d be allowed.] failed as expected in [shared|dedicated]-worker-import-data-url-cross-origin.html.ini. r=dom-worker-reviewers,asuth

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

According to the current spec, the script "export-block-cross-origin.js"
is in the same origin of the document, therefore it should be loaded.

But with Gecko's implementation, the script "export-block-cross-origin.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187900

UltraBlame original commit: 6e401be8c58633271e8a0813f2b9e98df1d5af8e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Sep 29, 2023
…cted in dedicated-worker-import-data-url.any.js and shared-worker-import-data-url.window.js. r=dom-worker-reviewers,asuth

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

According the current spec, the redirected 'export-on-load-script.js' is
in the same-origin with the document, therefore it should be loaded.

But with Gecko's implementation, the redirected script "export-on-load-script.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

(The first test case, which tests static import without redirect, the
script 'export-on-load-script.js' could be loaded by Gecko because the
script has the CORS header.)

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187901

UltraBlame original commit: 460e36a5e5aaca8ac8102a05e4357315e2c18583
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Sep 30, 2023
…d be allowed.] failed as expected in [shared|dedicated]-worker-import-data-url-cross-origin.html.ini. r=dom-worker-reviewers,asuth

In the [static import script from data: URL should be allowed.] case,
The worker will load the data URI, which imports "/resources/static-import-script-block-cross-origin.js".

And static-import-script-block-cross-origin.js will static import 'export-block-cross-origin.js'

According to the current spec, the script "export-block-cross-origin.js"
is in the same origin of the document, therefore it should be loaded.

But with Gecko's implementation, the script "export-block-cross-origin.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187900

UltraBlame original commit: 6e401be8c58633271e8a0813f2b9e98df1d5af8e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Sep 30, 2023
…cted in dedicated-worker-import-data-url.any.js and shared-worker-import-data-url.window.js. r=dom-worker-reviewers,asuth

In this test, The worker will load a data URI, which import another test
script statically, and the imported script will import 'export-on-load-script.js',
which will be served with the CORS header 'Access-Control-Allow-Origin: *' in
/workers/modules/resources/export-on-load-script.js.headers

In the test case [Static import (redirect).], 'export-on-load-script.js' will be
redirected, and the redirected script doesn't have the CORS header.

According the current spec, the redirected 'export-on-load-script.js' is
in the same-origin with the document, therefore it should be loaded.

But with Gecko's implementation, the redirected script "export-on-load-script.js"
is cross-origin with the main worker script (which is loaded by data URI),
so the script is blocked.

(The first test case, which tests static import without redirect, the
script 'export-on-load-script.js' could be loaded by Gecko because the
script has the CORS header.)

Mark this test case as failed according to the spec issue
whatwg/html#9571

Differential Revision: https://phabricator.services.mozilla.com/D187901

UltraBlame original commit: 460e36a5e5aaca8ac8102a05e4357315e2c18583
@annevk annevk added the agenda+ To be discussed at a triage meeting label Oct 5, 2023
@past past removed the agenda+ To be discussed at a triage meeting label Nov 2, 2023
@codehag
Copy link

codehag commented Dec 11, 2023

Hi everyone, thanks for discussing this. I recently returned from parental leave and after a bit of time reviewing this I think can clarify a bit.

There are two issues here: one is the underlying principle, which is being discussed above. We might summarise that as “How should this work and why?”. The other is a functional issue, that is: at the moment browser have had to choose whether they support the CSP behaviour, or if they support service worker interception of modules. I think this is due to a collision in the spec.

As mentioned above, this behavior is tested in interop-2023 of WPT, but Chrome/Firefox/Webkit have different behavior on this,
https://wpt.fyi/results/workers/modules/dedicated-worker-import-csp.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-modules

To be more precise here: Firefox and Safari have the same behaviour, Chrome has different behaviour. Firefox and safari both pass the service workers interception test, which chrome does not. Conversely they both fail the CSP tests. When I was implementing this behavior, I found that issue was related to the environment. Using the outer environment passes the CSP tests (as spec'd), using the inner environment passes the service worker interception test.

In my initial question in the original issue I raised the issue around service workers, I'll add more detail here:

The problem in a bit more detail:
What is happening in the test is that the responsible environment for the loading of the static files is the outer environment, that of the document, rather than the dedicated worker environment. I believe this results in the chrome bug currently being investigated, where the source is “uncontrolled”, are due to the same behavior. After some testing, I noted that if the service worker has the capacity to intercept the parent document, then the worker will also be intercepted. This aligns with my reading of the specification, where the client being used for service worker interception is the outer client, because this is the only one used by the fetch. During my testing, I noted also that while you can get the first test to pass by controlling the document, the subsequent tests (around static and dynamic import) still fail in chrome, so there may be more going on here. An alternative could always be to say "the test is wrong", but I think everyone here will agree that rewriting code to be inside of iframes in order to get service worker integration to work is not very developer friendly.

The solution (as I understand it):
The fetch for the worker top level script and all subsequent static module scripts will need to be modified so that both the inner and outer environments are known. In other words, we would need to change the request so that it is aware of both fetchClient (the outer environment) and settingsObject (the inner environment). This would allow the eventual service worker fetch handler to get the right client.

I think.

If I am right about this, we need to change the spec in order to support both the currently tested CSP behavior and the currently tested service worker behavior, and that change is to allow fetch to be aware of two environments for workers. This seems very complicated, and the other solution is to simply use the inner environment rather than the outer environment. This is what brings up the philosophical issue that we are all discussing here. This is my perspective of the current discussion.

For our part, in February, after discussion internally (@smaug----, @asuth, @evilpie, myself and others), we decided that for the time being breaking service worker interception for all static imports represented a more serious issue than a more conservative CSP for loading static imports. This deviates from the spec'd behavior in HTML but fulfills the expectation of the service worker and also works in a way that users would expect (a similar argument to the bundling argument above being made in favor of CSP). I think we should go with the inner environment of the worker instead of the more complicated refactoring to fetching. It is cleaner, and there are good arguments in favor of the inner environment and breaking with the bundling expectation.

As an outsider, when I was reviewing how CSP is handled for workers, I found it surprising that static imports would use the document’s CSP, for the same reason brought up by @youennf. In particular, I couldn’t justify the current behaviour through the bundling argument, because static imports can import arbitrary urls, this isn’t the case for bundled code which is by definition a single file. This is why I think the bundling argument ultimately falls apart and, given the additional complexity of the service worker interception, I think we should instead be using the inner environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

11 participants