Set creation URL of worker settings to final URL after redirect #12038
Set creation URL of worker settings to final URL after redirect
#12038domfarolino merged 1 commit intowhatwg:mainfrom
Conversation
To determine the request referrer for a worker global scope [1] it uses the creation URL. This URL is initialized for workers in "set up a worker environment settings object" [2] which is called at the start of "run a worker" [3]. However, per the WPT tests [4] the referrer should use the final URL after redirection. This is available in "processResponseConsumeBody" [5] where we already update the URL for the worker. To match browser behavior, we should also update the creation URL here. [1]: https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer [2]: https://html.spec.whatwg.org/multipage/workers.html#script-settings-for-workers:concept-environment-creation-url [3]: https://html.spec.whatwg.org/multipage/workers.html#worker-processing-model:set-up-a-worker-environment-settings-object [4]: https://wpt.fyi/results/fetch/api/basic/request-referrer-redirected-worker.html?label=experimental&label=master&aligned [5]: https://html.spec.whatwg.org/multipage/workers.html#worker-processing-model:fetching-scripts-processcustomfetchresponse
|
Discovered this while correcting the determining of the referrer value in Servo for workers: servo/servo#41458 |
|
Turns out that this patch is wrong, see tests in web-platform-tests/wpt#56921 Instead, the request referrer should use https://html.spec.whatwg.org/multipage/webappapis.html#api-base-url instead of creation URL, as that's what browsers actually do. That also makes a lot more sense to me. |
Based on an investigation as to why Servo doesn't use the correct URL per the spec [1] I thought that HTML was incorrectly setting the creation URL [2]. However, after writing a WPT test [3] it turns out that the creation URL is correctly set in HTML, but incorrectly used in Referrer Policy. Browsers don't use the creation URL to determine the referrer policy, per another WPT test [4]. Instead of the creation URL, browsers use the url of the worker, which is the same as the API base URL [5]. [1]: servo/servo#41458 [2]: whatwg/html#12038 [3]: web-platform-tests/wpt#56921 [4]: https://wpt.fyi/results/fetch/api/basic/request-referrer-redirected-worker.html?label=master&label=experimental&aligned [5]: https://html.spec.whatwg.org/multipage/workers.html#script-settings-for-workers:api-base-url
|
Opened a PR for that spec: w3c/webappsec-referrer-policy#177 |
See w3c/webappsec-referrer-policy#177 (comment) for why I don't think we can do that. I think the direction of this PR might be right after all. |
|
Other specifications such as the reporting API rely on the creation URL being the URL prior to redirects. See step 2 of https://w3c.github.io/reporting/#generate-a-report That's what web-platform-tests/wpt#56921 confirms is the existing behavior in browsers. |
|
Per our investigation in w3c/webappsec-referrer-policy#177 it turns out that this PR is valid after all. Servo passes all existing WPT tests including the new one with this change. |
|
I think this PR is in the right direction. But please restore the PR template and fill everything necessary out, including a link to any tests that you write, and ideally even those whose expectations might change due to this PR. It would be great to know if any engines currently rely on the state of things before this PR, or if this PR is simply fixing a spec bug that engines don't have. I assume the latter. |
|
@domfarolino Do you know anybody from Webkit/Gecko who can review this change? |
|
@asutherland or @bakulf Can you review from gecko side? |
|
The spec change makes sense. Our reporting API situation is not great for the test case in the sense that it doesn't work[1], but I manually ran the test under rr (mozilla pernosco trace) to understand the situation better and that did allow me to validate that I believe we would pass the test if our reporting API glue was working. Specifically, for the 2 worker spawns we see an aViolationEventInit.mDocumentURI of:
In that second case if we weren't updating after the redirect, we would be seeing 1: We do enable our reporting API impl in-tree for WPTs and we do try and report the error, but in our workers impl the fetch() call is remoted from the content process to the parent process and gets reported in the parent process but nothing propagates it back down to the content process AFAICT. |
|
I suppose this solution is okay, but this is really a bandaid solution. The real problem is that we are creating the environment before we have a response. We should really wait for the response and only then create an environment. (There's some older issues explaining that.) |
|
@annevk ack, do you consider that blocking here? Unfortunately I don't know what the implications are for such an overhaul and Servo is currently not in a position to prototype with that. We have other fundamental projects going on before we can make that happen. |
This originally was part of a FIXME in step 5 of "setup a worker environment settings object". Since the worker global scope's url isn't populated at this point, we set the creation_url as soon as it is. This step will be added to the spec in whatwg/html#12038. After this change, all the WPT subtests in... https://wpt.live/referrer-policy/gen/worker-classic.http-rp/no-referrer-when-downgrade/fetch.http.html ...will pass!
This originally was part of a FIXME in step 5 of "setup a worker environment settings object". Since the worker global scope's url isn't populated at this point, we set the creation_url as soon as it is. This step will be added to the spec in whatwg/html#12038. After this change, all the WPT subtests in... https://wpt.live/referrer-policy/gen/worker-classic.http-rp/no-referrer-when-downgrade/fetch.http.html ...will pass!
domfarolino
left a comment
domfarolino
left a comment
There was a problem hiding this comment.
I also think this is a bandaid, but I think this is better than nothing, so I'm OK with landing it. I'll give @annevk a day to object, but otherwise I think we're alright to land this.
|
It's fine. We're tracking the root cause through #5362 and I suspect some other related issues. |
|
@theIDinside the test uses ReportingObserver to obtain the report, not an endpoint. As far as I understand the spec, ReportingObservers are available in HTTP context. The test passes in both Chromium and Servo. |
Deleted the comments, as they were just red herrings. |
6 participants
To determine the request referrer for a worker global scope 1
it uses the creation URL. This URL is initialized for workers in
"set up a worker environment settings object" 2 which is
called at the start of "run a worker" 3.
However, per the WPT tests 4 the referrer should use the
final URL after redirection. This is available in
"processResponseConsumeBody" 5 where we already
update the URL for the worker.
To match browser behavior, we should also update the
creation URL here.
ReportingObserversupportWorker(See WHATWG Working Mode: Changes for more details.)
/workers.html ( diff )