final response URL is not propagating through service worker interception correctly

RESOLVED FIXED in Firefox 64

Status

()

defect
P3
normal
RESOLVED FIXED
8 months ago
23 days ago

People

(Reporter: bkelly, Assigned: tt)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Bug 1222008 attempted to implement propagating the final response URL through a service worker interception to the outer response.  Anne found, however, that this doesn't seem to be working right.  A fetch() that is intercepted seems to still use the request.url for the final response.url.

This is captured in this glitch:

https://sw-fetch-final-response-url.glitch.me/

The web console should display:

  ===> Window got response.url: https://sw-fetch-final-response-url.glitch.me/controlled/replaced

But on firefox it currently displays:

  ===> Window got response.url: https://sw-fetch-final-response-url.glitch.me/controlled/test

Interestingly, if the service worker intercepts with a redirected fetch() then the final response URL is propagated.  This is seen in this console output:

  ===> Window got redirected response.url: https://sw-fetch-final-response-url.glitch.me/controlled/replaced

This probably means the dom/fetch code is not looking for URL changes on internal redirects.  The service worker only does a full redirect if the synthesized response has .redirected set to true.

And here is probably the conditional that needs to be removed:

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/fetch/FetchDriver.cpp#1285
(Reporter)

Comment 1

8 months ago
Well, it may be more complicated than that.  Not sure if the mRequest->AddURL() is strictly right for the synthetic response URL case.  That will result in the final Response.redirected always being true when intercepted by the service worker.

In the case of the internal redirect you probably need to replace the request's URL list with the nsIChannel's final URL.
Priority: -- → P3

Comment 2

8 months ago
(https://github.com/web-platform-tests/wpt/pull/12680 tests this, though also tests https://github.com/whatwg/fetch/pull/696 so might not be useful for fixing this in isolation.)
(Assignee)

Comment 3

8 months ago
The behavior looks interesting to me... 

It seems mRequest->AddURL() doesn't work fine since the result in FF for "redirected response.url" is supposed to be the value for "response.url". 

Since I implemented bug 1222008, I think I should fix this
Assignee: nobody → shes050117
(Assignee)

Comment 4

8 months ago
I copied the code in https://sw-fetch-final-response-url.glitch.me/ to my localhost and test it.

I tried to log URL while InternalResponse::IsRedirected() [1] is called (it should be called only while service worker start to synthesize response [2]) and I got two URL lists and each one only has one entry: 

  http://localhost:8080/controlled/replaced

and 

  http://localhost:8080/controlled/redirect-to-replaced

It seems the problem is that somehow service worker didn't overwrite the URL to the right place because it got the expected responses and tried to synthesize them into synthesized responses.

[1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/fetch/InternalResponse.h#335
[2] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/serviceworkers/ServiceWorkerEvents.cpp#373
(Assignee)

Comment 5

8 months ago
I still don't understand why FF shows "Window got redirected response.url: https://sw-fetch-final-response-url.glitch.me/controlled/replaced" which should be the result for "Window got response.url".

I suspected there was an issue that made Service Worker hijacks a wrong channel or treats a wrong channel as an intercepted channel. But, I set a breakpoint in HttpChannelChild::BeginNonIPCRedirect() just before calling HttpChannelChild::SetupRedirect() [1] and the responseURI is "http://localhost:8080/controlled/replaced" for the first hijacked request. 

That probably means that at least we pass the correct input for the redirect and it seems to imply service worker redirect a correct channel to a hijacked channel.

Therefore, the fix should be as bkelly suggested in comment #0 and comment #1. We probably want to remove that check carefully. 

Note that: it seems to me we may consider using another flag to differentiate service worker's internal redirects or the others.

[1] https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/netwerk/protocol/http/HttpChannelChild.cpp#1933
(Assignee)

Comment 6

8 months ago
(In reply to Tom Tung [:tt] from comment #5)
> Therefore, the fix should be as bkelly suggested in comment #0 and comment
I meant, therefore, the issue should just like because the response is marked as a REDIRECT_INTERNAL, the correct response's URL doesn't be propagated to the final response's URL.
(Assignee)

Comment 7

8 months ago
I'm considering to create a URL setter for the internal response in order to overwrite the URL [1] while the request is internally redirected by a service worker. 

Note that:
- If the original request and internally redirected request are different origins the request should fail and the check is in SW, IIRC.
- If the internally redirected request has redirects, it should work fine in current logic.

[1] https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/dom/fetch/FetchDriver.cpp#1285
(Assignee)

Comment 8

8 months ago
Bug 1222008 didn't propagate a sw redirected URL to outer response properly. To
fix that this patch mainly make a redirecting request be overwritten while it's
redirected by a service worker. This patch also add a private setter function
for InternalRequest and a public checking function to avoid that function from
being used widely.

Bug 1486445 - P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r?asuth

This patch add a wpt test to ensure the service worker redirected request URL
is propagated to the outer response.
Attachment #9005612 - Attachment is obsolete: true
(Assignee)

Comment 9

8 months ago
Bug 1222008 didn't propagate a sw redirected URL to outer response properly. To
fix that this patch mainly make a redirecting request be overwritten while it's
redirected by a service worker. This patch also add a private setter function
for InternalRequest and a public checking function to avoid that function from
being used widely.
(Assignee)

Comment 10

8 months ago
This patch add a wpt test to ensure the service worker redirected request URL
is propagated to the outer response.
(Assignee)

Comment 11

8 months ago
To clear the content for my confusion (comment #3 comment #4 comment #5):
I misunderstood the behavior of comment #0. The result of "Window got redirected response.url" is because of the URL has been redirected by the server rather than FF did something wrong.

So my patches simply overwrite the request URL while a service worker does an internal redirect. However, I'm a bit afraid that giving a public setter will make it be abused. So, I added a checker in public and created a private setter instead.
Comment on attachment 9005615 [details]
Bug 1486445 - P1 - Propagate the sw internally redirected URL to fetching request; r?asuth

Andrew Sutherland [:asuth] has approved the revision.
Attachment #9005615 - Flags: review+
Comment on attachment 9005616 [details]
Bug 1486445 - P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r?asuth

Andrew Sutherland [:asuth] has approved the revision.
Attachment #9005616 - Flags: review+

Comment 15

7 months ago
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bd73949fded4
P1 - Propagate the sw internally redirected URL to fetching request; r=asuth

Comment 16

7 months ago
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/78b3718b7ede
P2 - Add a wpt test to verify url of a service worker redirected request is propagated to response; r=asuth
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12985 for changes under testing/web-platform/tests
(Assignee)

Comment 18

7 months ago
Thanks for the review!
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 20

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd73949fded4
https://hg.mozilla.org/mozilla-central/rev/78b3718b7ede
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Upstream PR merged
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12985
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/DH3tBigiT8mMYJnqNmc1Wg)
(Assignee)

Comment 23

7 months ago
We probably want to uplift this to beta since it's a regression. If we indeed want to do that, I will uplift patches to beta a week later if everything is fine.

Marked it leave-open for now.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.