Cross origin workers should fail to fetch #13671
Conversation
|
Clearly the original author of this test anticipated cross-origin Worker construction to fail in some implementations, hence the |
|
The original test just tested for a sync exception. The async bit was added in https://bugzilla.mozilla.org/show_bug.cgi?id=1218433 and it looks like the "dangling step_func_done" problem also affects the changes made to workers/constructors/SharedWorker/same-origin.html and workers/constructors/Worker/same-origin.html in there. Those tests are also doing the timeout thing in non-Firefox browsers, for the most part. Would you mind fixing them too while you're here? The discussion about what the behavior should be, such as it was, was https://bugzilla.mozilla.org/show_bug.cgi?id=1218433#c16 and https://bugzilla.mozilla.org/show_bug.cgi?id=1218433#c19 and I'm not sure what the spec said at the time.... |
| assert_true(e instanceof Event); | ||
| }); | ||
| } catch (e) { | ||
| assert_unreached("Constructing a cross-origin module worker should not synchronously fail. Its error even should be fired instead."); |
|
So in terms of the spec... https://html.spec.whatwg.org/multipage/workers.html#dom-worker step 1 says:
which seems like the clause the other UAs are using; "policy decision" is pretty broad. In theory, if implementing the spec word for word one would have to do a bunch of work including setting up the worker global before discovering that the load failed and erroring. In practice, of course, one can just queue a task to fire error instead of throwing and not do anything else. |
|
Good catch @bzbarsky. I've updated the tests to still pass in the case where cross-origin workers fail to construct, given the spec text you found. I've also fixed the {Worker, SharedWorker}/same-origin.html tests. PTAL |
| try { | ||
| var worker = new SharedWorker(script, ''); | ||
| var worker = new Worker(script); |
There was a problem hiding this comment.
I also noticed that the Worker constructor test was still testing the SharedWorker constructor, probably as a result of copy paste. This meant all of these tests failed in Safari, a UA that doesn't support SharedWorkers. I've tested these updates and they look good in Chrome, Firefox, and Safari.
| try { | ||
| var worker = new SharedWorker(script, ''); | ||
| var worker = new Worker(script); |
/cc @domenic @bzbarsky @nhiroki
Tackle low-hanging fruit in #13426.
Prior to this PR, testing that cross-origin workers fail was pretty grim, in that the test basically broke for everyone but Firefox, who seems to be the only conforming implementation here.
It is true that cross-origin workers fail to fetch in all or most implementations, but it seems that every failure is some synchronous security error, as opposed to letting the Fetch naturally result in a failure, and going through the normal means of firing "an event named error".
This PR fixes the test for non-Firefox-like impls, and extends it to include module workers. I guess I should follow-up with vendors though, to see if they'd be willing to remove their immediate-erroring behavior in favor of letting Fetch naturally fail the request. Is it possible that vendors see cross-origin workers as a big enough security concern such that it's too big a risk to Fetch the doomed request, lest it somehow slip through the cracks and got sent? I just think its odd that a lot of impls are immediately failing cross-origin worker construction, when the spec does never mentions this. Here's where we do it in Chrome.