1222008 - propagate final URL from .respondWith() Response back to intercepted channel as a fake redirect

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

A fairly large change was made to the SW spec at the last f2f meeting:

  https://github.com/whatwg/fetch/pull/146

Basically:

1) If the Response passed to .resolveWith() has a .url set, then it should be passed on to the original intercepted channel.  In effect, it should look like a redirect.
2) If Response.url is null, then use the Request.url.  This effectively covers the synthetic Response case.
3) Navigations do not inspect the final URL in the html and fetch spec.  They instead do "manual" redirects and follow Location headers themselves.  So the URL shown in window.location should not be affected by these changes.

In gecko, I think this means:

a) If the Response.url is non-null and different from Request.url, then do a fake redirect like we do for opaque responses today.
b) Otherwise, do nothing
c) null Response.url for non-basic Responses should be treated as an error

We will also have to change a ton of tests.  This is a breaking change in that its observable in a number of places:

* Intercepted fetch() will return a different Response.url
* Intercepted stylesheets will load sub-resources relative to different URL
* Intercepted workers will load relative importScripts() from different URL

I'm marking this postv1, but we should probably do it sooner rather than later.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Note, there is some further discussion about document baseURI here:

  https://github.com/slightlyoff/ServiceWorker/issues/787

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Some release timing feedback from chrome on this change:

"Since M48 already branched, I'd say the earliest is M49 (branchpoint in Jan, reaches stable in Mar), though it could easily be a later milestone."

I think to match we would need to get it into 45.  Unfortunately, that does not seem feasible given we only have 2 more weeks before merge.

We should try to get this in to 46.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Hopefully we can answer the open spec questions today.  Namely, should document.baseURI be updated for the final URI?  I think yes, but we need to see.

Comment 4

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #3)
> Hopefully we can answer the open spec questions today.  Namely, should
> document.baseURI be updated for the final URI?  I think yes, but we need to
> see.

Agreed.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I believe we have finally decided to always propagate back Response.url as the baseURI of the outer resource:

  https://github.com/slightlyoff/ServiceWorker/issues/787

Comment 6

[(no longer active)]

Setting a needinfo as a reminder.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is that different from behaving like a redirect?

I.e. are we inventing a new type of response that HTTP can't produce, and that is specific to service workers?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

It's the same as a redirect for everything except a navigate.  For a navigate we keep the location bar matching the request URL.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So for navigations there would be things that service-workers can do, but that HTTP-servers can't do?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One followup question is what happens if the resulting page calls pushState or replaceState? Does that change both the baseURI as well as the document URI? Or does it just update the document URI?

Usually the two URIs are the same and both are updated when pushState/replaceState is called.

Another question is what happens if the resulting page contains a <base href>? Does that override the baseURI set by the service-worker? Or the other way around?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> So for navigations there would be things that service-workers can do, but
> that HTTP-servers can't do?

Yes, which it can already do without this change.  Today service worker interception replaces navigation content without changing the location bar.  This bug would just update the baseURI so relative resources continue to load correctly.

(In reply to Jonas Sicking (:sicking) from comment #10)
> One followup question is what happens if the resulting page calls pushState
> or replaceState? Does that change both the baseURI as well as the document
> URI? Or does it just update the document URI?

pushState/replaceState would function normally after the page is loaded.  It would change both.

> Another question is what happens if the resulting page contains a <base
> href>? Does that override the baseURI set by the service-worker? Or the
> other way around?

<base> work normally.  It would override the baseURI.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think it's accurate to say that a SW can already do things that HTTP-servers can't.

Today when a SW responds with a resource from a different URL it acts just like if a HTTP server had loaded content from the other URL and responded with it.

What we're talking about now is creating pages where document.URL != document.baseURI but where document.getElementsByTagName('base').length == 0.

That is something that
* Could never happen before
* Still can't be done by a HTTP server

I agree it might not be important. But it's something that we should only do intentionally after having examined the pros and cons. And we should acknowledge that it means that SWs don't just describe HTTP semantics, but also invent new ones.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Ok, but are you objecting to this or suggesting a change?  Everyone in the room here is on board with this change and Jake has done a web developer survey to try to get their preferences included.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would rather that we changed the URL in the URL-bar to be honest. A few reasons are:

* It'll make implementation significantly easier since we'd only have to implement changes inside of
  necko. Adding new semantics means adding additional API to necko, wiring this additional information
  through docshell, document-loader-factory and the DOM.
* It'll add less confusion for developers. When a page does <img src="pic.jpg"> and the URL bar says
  https://website.com/coolstuff/page.html, why does it try to load an image from
  https://website.com/whatever/pic.jpg?
* It'll save developers from some of the weird cases around having a baseURI. Such as that
  <a href="#foo"> won't scroll within the current page, but rather navigate to a new page. Even worse is
  things like <svg:circle fill="url(#bar)"> which I believe will behave different in different browsers.
  Note that using <base href> is not particularly common, so most developers are not used to these edge
  cases.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

That would break a specific use case for service workers.  Specifically the ability to show an offline shell without redirecting.  AFAIK this has been a goal from the beginning.  We have also been shipping the "don't change location bar" for a long time and it's been a featured thing in almost every demo/article.

We need to change the baseURI in the stylesheet case in order for relative images and fonts to work.  We believe it's less confusing to keep baseURI behavior and document consistent.

Jake has actually talked to a lot of devs about this.  They have a wide variety of understanding and expectations.  No one approach came out as most clear for devs.  With that in mind we feel keeping relative resources functional and baseURI consistent is the best we can do.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

To back up my claim that not-changing-the-address-bar is an advertised feature people use for offline shells, see its use on The Guardian:

  https://www.theguardian.com/info/developer-blog/2015/nov/04/building-an-offline-page-for-theguardiancom

And other sites.

For more details on the results of Jake's research with web devs, see:

  https://github.com/slightlyoff/ServiceWorker/issues/787#issuecomment-184165823

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I definitely agree that we don't want to change the URL when serving a offline-shell. I'd even agree that doing that is more important than not introducing semantics that can't be expressed in HTTP.

What I'm not convinced of is that the proposed solution is the only solution to that problem.

One big concern is that having this behavior by default for navigations means that the complexities in comment 14 are exposed to developers even for the case when they are not serving an offline shell, but rather using an alternative resource for other reasons (i'm not sure what reasons those would be, but given that we handle using different responses for different URLs for resources, I assume that such reasons exist).


One potential solution is:
* Require that pages that are used as offline-shells include a <base> tag. Or that they use absolute URLs
  for stylesheets and images. This is what well-written websites do for 404 pages today.
* Enable SWs that are responding with a differet-url Response to a navigation Request to choose if
  it should be treated as a redirect or not (we can choose whatever default we want)

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #17)
> One big concern is that having this behavior by default for navigations
> means that the complexities in comment 14 are exposed to developers even for
> the case when they are not serving an offline shell, but rather using an
> alternative resource for other reasons (i'm not sure what reasons those
> would be, but given that we handle using different responses for different
> URLs for resources, I assume that such reasons exist).

Have you spoken to any developers about this?  Or is it just your belief?

> One potential solution is:
> * Require that pages that are used as offline-shells include a <base> tag.
> Or that they use absolute URLs
>   for stylesheets and images. This is what well-written websites do for 404
> pages today.

This is our current state today.  After talking with devs (see above link) we have decided we should fix the baseURIs so relative resources continue to work.

> * Enable SWs that are responding with a differet-url Response to a
> navigation Request to choose if
>   it should be treated as a redirect or not (we can choose whatever default
> we want)

This is already possible.  `evt.respondWith(response)` does not change location bar, while `evt.respondWith(Response.redirect(url))` does change location bar.

I'm sorry if you disagree, but this decision has been made deliberately with data from the community and devs.  If you have additional data showing its a problem, we would love to see that.  Simply stating a belief that its more confusing than the other options does not match the data we have.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Another potential solution is to add the ability for http responses to explicitly set a base-uri, thereby at least keeping http semantics and SW semantics in sync. This seems like it could be useful for said 404 pages.

But should I interpret your answer as that these other solutions have been considered and rejected? And that everyone was aware of the problems in comment 14?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> This is already possible.  `evt.respondWith(response)` does not change location bar, while
> `evt.respondWith(Response.redirect(url))` does change location bar.

That still means that for resources we have the ability to do a combined redirect-and-return-resource using `evt.respondWith(responseWithDifferentURL)`, but we don't have that ability for navigations. So sounds like developers asked for different capabilities for navigations and resources?

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #20)
> That still means that for resources we have the ability to do a combined
> redirect-and-return-resource using
> `evt.respondWith(responseWithDifferentURL)`, but we don't have that ability
> for navigations. So sounds like developers asked for different capabilities
> for navigations and resources?

I don't understand what this means.

And also I think resources already behave in this manner.  If an img element gets redirected its .src attribute is not updated to the final URL.  That is conceptually the same as leaving the location bar unchanged.  I would look up more spec cases, but I'm on tethering at the moment.  The last time I spoke with Boris about this, though, I remember him thinking this was reasonable given how we don't update sub-resource element attributes for final redirect URL.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For a fetch to URL X, there's a big difference between
A) Redirecting a fetch to a new URL, Y, and returning a resource there, and
B) Returning a resource but fudging its baseURL to be Y.

A involves a redirect and returns a resource with URL Y. B does not involve a redirect and returns a resource with URL X.

I agree that you can't tell the difference through the <img>.src attribute. In fact, there's relatively few places where you can tell a difference during resource loads, but it's not zero places.

I would imagine that fetch() would treat the two differently. I would expect behavior A to result in a Response object with a .url = Y, and B to result in a Response object with .url = X.

I would also expect that if fetch() used { redirect: "error" }, that behavior A would result in an error while behavior B would not.

Another example is SVG files loaded into an <img> or used as a CSS image, if such an SVG file contain a <circle fill="url(#foo)">. Using behavior A I would expect the circle to be painted using the pattern found at element with id=foo. Using behavior B I would expect another network request to be made to URL Y before a pattern could be extracted. In gecko I would also expect this network request to be blocked since we don't allow SVG-in-image to load external resources.


I would really encourage you to not think about this in terms of "how will img.src and the location bar behave", since the web platform contains a lot of APIs and you'll have to define behavior for all of them.

Instead I would encourage you to think about this in terms of what how necko behaves towards the gecko callsite. Which is equivalent to thinking about this in terms of the fetch specification. I know the latter isn't always easy since not all specifications has been updated and expressed in terms of the fetch specification. However in practice in implementations all web platform features do use the same network stack and are affected by how that network stack behaves.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Another potential solution is to add the ability for http responses to explicitly set a base-uri

This does not seem totally unreasonable, via headers.

Comment 24

[Jake Archibald]

Jonas: I'm struggling to get a lock on where your objection is. I know it's a pain, but could you answer the questions at https://jakearchibald.com/2016/service-workers-and-base-uris/ (either there or here).

I was unsure of this change initially, but making baseURL the response URL feels like the most consistent rule.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Jonas, have you had a chance to provide the feedback Jake asked for?  Other browsers are starting to implement this so if you have spec feedback, now's the time.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

I made a glitch to test this, and it appears chrome has implemented it:

https://sw-passthrough-redirect.glitch.me/

We should implement it as well.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Edge 16 with service workers enabled in about:flags also shows the same behavior as chrome on the glitch in comment 26.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

I don't have time to work this immediately, but I think we should do it soonish.  Tom, do you have any bandwidth to take this?  Maybe for early 59 timeframe?

I think what we need to do is:

1. Change the respondWith() code that does the "fake" internal redirect for opaque responses and make it do the redirect whenever the Response.url is non-empty and does not match the original Request.url.
2. If the Response.redirected value is false, do an internal redirect like today.
3. If the Response.redirected value is true, do a real redirect instead.
4. Probably fix a bunch of tests.

Comment 29

[Tom Tung [:tt, :ttung]]

Sure, I'll work on this.

Comment 30

[Tom Tung [:tt, :ttung]]

Attachment: bug1222008.patch

Hi Ben,

I'm not pretty sure if I understand what should do in this bug clearly or if I misunderstand anything, so would you mind giving me some feedback for this patch?

[The current code path]
IIUC, when |FetchEvent::respondWith()| resolves a promise, we pass responseURLSpec[1] to the InterceptChannel via class StartResponse if the response type is opauqe[2]. Then, StartSynthesizedResponse decides to do the internal redirect[3] only when the response URL/responseURI is different from the request/original URL. (Now, only opaque response may have different URI since [2])

[Solution]
Set the responseURLSpec[1] when the response's URL is different from request's. Also, pass the response.redirected to BeginNonIPCRedirect(). 

Decide whether do the internal or the real redirect by response.redirected.

For how to do the real redirect, I guess changing the redirect flag for SetupRedirect() in BeginNonIPCRedirect[4] to REDIRECT_TEMPORARY might be enough since it will make the FetchDriver record the redirecting URL into the URL list on the response.

[ToDo]
If this patch is correct so far, I need to deal with the non-e10s case (I guess I need to modify the code in InterceptHttpChannel) and fix the broken tests.

[1] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#346
[2] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#664-670
[3] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedChannel.cpp#304-305
[4] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1738

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8924107 [details] [diff] [review]
bug1222008.patch

Review of attachment 8924107 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Thanks!

Do you have any idea what tests break with this yet?  I'm just curious how widespread it is and if there are WPT tests that chrome is currently passing somehow.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +670,5 @@
> +  } else if (!ir->GetUnfilteredURL().IsEmpty() &&
> +             !mRequestURL.EqualsASCII(ir->GetUnfilteredURL().get())) {
> +    // Do the (internal/real) redirect to the response URL if it's different
> +    // from the request URL.
> +    responseURL = ir->GetUnfilteredURL();

I think you can remove all the conditions for opaque and equality here.  You should be able to pass `ir->GetUnfilteredURL()` directly instead of computing `responseURL` here.  The later necko code will do the string comparison for equality.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +432,5 @@
>  
>    // Perform a redirection without communicating with the parent process at all.
>    void BeginNonIPCRedirect(nsIURI* responseURI,
> +                           const nsHttpResponseHead* responseHead,
> +                           const bool internalRedirect);

nit: Maybe flip this argument to `aRealRedirect` instead of `internalRedirect` to match the logic in nsINetworkInterceptController interface.

Comment 32

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #31)

Thanks for the quick feedback! I'll address your suggestions, make sure it works fine on non-e10s(InterceptHttpChannel), check out how many tests fail and how do chrome deal with them.

> Comment on attachment 8924107 [details] [diff] [review]
> bug1222008.patch
> 
> Review of attachment 8924107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.  Thanks!
> 
> Do you have any idea what tests break with this yet?  I'm just curious how
> widespread it is and if there are WPT tests that chrome is currently passing
> somehow.

I have no idea about it now. I'll check it out today.

> ::: dom/workers/ServiceWorkerEvents.cpp
> @@ +670,5 @@
> > +  } else if (!ir->GetUnfilteredURL().IsEmpty() &&
> > +             !mRequestURL.EqualsASCII(ir->GetUnfilteredURL().get())) {
> > +    // Do the (internal/real) redirect to the response URL if it's different
> > +    // from the request URL.
> > +    responseURL = ir->GetUnfilteredURL();
> 
> I think you can remove all the conditions for opaque and equality here.  You
> should be able to pass `ir->GetUnfilteredURL()` directly instead of
> computing `responseURL` here.  The later necko code will do the string
> comparison for equality.

Sure

> ::: netwerk/protocol/http/HttpChannelChild.h
> @@ +432,5 @@
> >  
> >    // Perform a redirection without communicating with the parent process at all.
> >    void BeginNonIPCRedirect(nsIURI* responseURI,
> > +                           const nsHttpResponseHead* responseHead,
> > +                           const bool internalRedirect);
> 
> nit: Maybe flip this argument to `aRealRedirect` instead of
> `internalRedirect` to match the logic in nsINetworkInterceptController
> interface.

Sure

Comment 33

[Tom Tung [:tt, :ttung]]

Attachment: WIP

Address the comment and ensure it does the same thing in non-e10s mode. I'll try to catch out the broken tests on my machine. Meanwhile, I'll push it to try in case I overlook any broken test.

Comment 34

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=935066eae4cddf36331561c2f25179594732ad5f

Comment 35

[Tom Tung [:tt, :ttung]]

I put the failed tests below:

XpcShell
[Fixed] netwerk/test/unit/test_synthesized_response.js
[Fixed] netwerk/test/unit_ipc/test_synthesized_response_wrap.js 
- Solution: Fill up the last argument (adding in the patch) to startSynthesizedResponse() in the test 

Mochitest
[Fixed] dom/tests/mochitest/fetch/test_fetch_basic_http_sw_reroute.html
- Solution: Correct the redirected URL [1] if the request is hijacked by the service worker
[1] http://searchfox.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic_http.js#53-54

[Fixing] dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html 
- Error: synthesize CORS response should result in outer CORS response
- Solution: Since we do the internal redirect while a service worker responds with a different URL (without redirected), we should get opaque type of response.

- Error: INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html | Load CORS cross origin XML Document should be allowed
- Still finding the reason...


dom/workers/test/serviceworkers/test_fetch_event.html 

dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html

Wpt

service-workers/service-worker/fetch-event-redirect.https.html

service-workers/service-worker/fetch-frame-resource.https.htm

service-workers/service-worker/fetch-response-taint.https.html

service-workers/service-worker/redirected-response.https.html

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #35)
> I put the failed tests below:

Thanks!

> [Fixing]
> dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html 
> - Error: synthesize CORS response should result in outer CORS response
> - Solution: Since we do the internal redirect while a service worker
> responds with a different URL (without redirected), we should get opaque
> type of response.

Wait, the type of the response should still be CORS.  I think probably the outer channel is no-cors and is hitting this path in its security code:

https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#712

The service worker spec has this annoying thing where we have to override this behavior in some synthetic cases.  We do it already here:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#338

The problem, though, is that call is made before the additional redirect.  We either need to move it to happen after the redirect (not sure where to put that).  Or maybe make the LoadInfo set a flag when we have called SynthesizeServiceWorkerTainting() so that we don't change it further in MaybeIncreaseTainting().  I think freezing the tainting value after synthesis is probably the easiest approach.

Christoph, do you have any opinions here?  The issue is we're making synthesized channels perform an internal redirect to their synthetic Response.url.  The problem is that when a channel with SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is synthesized with a CORS response from the SW the SW spec wants the resulting outer channel to be LoadTainting::CORS.  I'm basically suggesting we follow the same solution used in bug 1369862.

Comment 37

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #36)
Got it! Thanks for telling me that!

I'll try the approach you suggested if Christoph doesn't have other suggestion.

Comment 38

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #35)
> - Error: INFO TEST-UNEXPECTED-FAIL |
> dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html |
> Load CORS cross origin XML Document should be allowed
> - Still finding the reason...

Hi Ben,

In this test, "index.html" tries to open an XML document under the same origin (http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch/load_cross_origin_xml_document_cors.xml). Then, the request is hijacked by a service worker and respond with another response [2]. The response is generated by fetching a request with a cross-origin URL (http://example.com/tests/dom/security/test/cors/file_CrossSiteXHR_server.sjs?status=200&allowOrigin=*) with CORS mode.

After applying my patch, since the requesting URL is different from responding URL and the |response.redirected = false|, it should do an internal redirect.

However, I got few warning messages and fail in this test case. I put the warning messages below:

GECKO(65301) | [Child 65303, Main Thread] WARNING: NS_ENSURE_TRUE(!(NS_HasBeenCrossOrigin(aChannel, true))) failed: file /Mozilla/Work/m-c/dom/security/nsContentSecurityManager.cpp, line 196
GECKO(65301) | [Child 65303, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file /Mozilla/Work/m-c/dom/security/nsContentSecurityManager.cpp, line 706
GECKO(65301) | [Child 65303, Main Thread] WARNING: 'NS_FAILED(aRv)', file /Mozilla/Work/m-c/dom/workers/ServiceWorkerEvents.cpp, line 245

It seems the security mode in the load flag is set to be "nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED", but the URL of the checking channel is redirected to response's "http://example.com/tests/dom/security/test/cors/file_CrossSiteXHR_server.sjs?status=200&allowOrigin=*". Thus, the test fails while checking the DOSOPChecks().

I'm not sure about the correct behavior and the real problem here. Does the security mode wrong not being redirected correctly? Or the reponse's URL should be "http://mochi.test:8888/tests/dom/workers/test/serviceworkers/fetch/load_cross_origin_xml_document_cors.xml" after the internal redirect. Or there is another issue?

Would you mind giving me some suggestions or hints? Thanks!
 
[1] http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/fetch/index.html#140
[2] http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/fetch_event_worker.js#343-351

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Anne, what do you think about comment 38?  If a same-origin outer request (xml document in this case) is intercepted with a cross-origin CORS request by the SW, should we still use the final Response.url?  In this case the feature (xml doc) does not expect to see a cross-origin URL at all.  Do we really want to start exposing a cross-origin documentURI here?

I feel like it might be safer to expect this sort of use-case to reject.  You can't synthesize a cross-origin Response onto a Request in same-origin mode, etc.

If we do want to support this we probably need to change nsContentSecurityManager and nsCORSListenerProxy to better understand synthesized channels.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #36)
> Christoph, do you have any opinions here?  The issue is we're making
> synthesized channels perform an internal redirect to their synthetic
> Response.url.  The problem is that when a channel with
> SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is synthesized with a CORS response
> from the SW the SW spec wants the resulting outer channel to be
> LoadTainting::CORS.  I'm basically suggesting we follow the same solution
> used in bug 1369862.

Yeah that makes sense. Adding a new flag to the loadinfo, similar as in Bug 1369862 sounds like the right path forward to me.

Comment 41

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #35)
> dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html

The test does the steps as follows:
1. The controlled document send out a request with "fake.html"
2. The service worker hijacks the request and respond a response with "hello.html"
3. Waiting for the OnStopRequest() which sending the "http-on-stop-request" topic to the observer and then checking the timing information.
4. Timeout

Currently, we do an internal redirect for this case because the response "hello.html" hasn't redirected. However, this implies the OnStopRequest will stop the redirected request ("hello.html") [1]. Meanwhile, we record the FinishSynthesizedResponseEnd on the original channel [2] after redirecting. It means we track this marker on the original channel without having a chance to copy it to the redirected channel.

Thus, in this test, we cannot get the original channel (with all the timing markers) by listening to the "http-on-stop-request" topic, although we can get the redirect channel ("hello.html") without all the timing markers (missing the FinishSynthesizedResponseEnd) by listening to the "http-on-stop-request".

Both situations cannot meet the testing requirement (get all the timing makrers).

I guess we could either manually sending OnStopRequest for the original request or record the FinishSynthesizedResponseEnd on the redirect channel. Not sure which ways are better or if there is a better solution to fix this problem.


[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedHttpChannel.cpp#1015
[2] https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#216

Comment 42

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #35) 
> Wpt
> 
> service-workers/service-worker/fetch-event-redirect.https.html

It looks like this change makes Firefox pass the rest test cases which are expected fail. Remove the ".ini" file fix the issue.

> service-workers/service-worker/fetch-frame-resource.https.htm

This one looks problematic; my Nightly crashed while running it.

> service-workers/service-worker/fetch-response-taint.https.html

We fail on 6 test cases, but I got 16 failures on Chrome Canary. I list our failure test cases below, I'll dig a bit more tomorrow:

fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"omit" should succeed.
  with error message:
    promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

  Chrome Canary pass this test.

fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"same-origin" should succeed.
  with error message:
    promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

  Chrome Canary pass this test.

fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"include" should succeed.
  with error message:
    promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

  Chrome Canary pass this test.

fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8443%26ACACredentials%3Dtrue&mode=cors&credentials=include&" mode:"same-origin" credentials:"omit" should succeed.
  with error message:
    promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

  Chrome Canary: fail as well with error message: 
    promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch" 

fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8443%26ACACredentials%3Dtrue&mode=cors&credentials=include&" mode:"same-origin" credentials:"same-origin" should succeed.
  with error message:
    promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

  Chrome Canary: fail as well with error message: 
    promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch" 

fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3Dhttps%3A%2F%2Fweb-platform.test%3A8443%26ACACredentials%3Dtrue&mode=cors&credentials=include&" mode:"same-origin" credentials:"include" should succeed.
  with error message:
    promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

  Chrome Canary: fail as well with error message: 
    promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch" 

> service-workers/service-worker/redirected-response.https.html

I pass this test after addressing comment #36 (adding a flag)

Comment 43

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #42)
> > service-workers/service-worker/fetch-response-taint.https.html
> 
> We fail on 6 test cases, but I got 16 failures on Chrome Canary. I list our
> failure test cases below, I'll dig a bit more tomorrow:

They are all the same problem as comment #38. It fails on same origin check in nsContentSecurityManager if we are passing an outer request in the same origin with same-origin mode and it is intercepted with a cross-origin CORS request by the SW.

Note: It's interesting that Chrome have the different behavior in these test cases. They fails on the first three test cases but pass the other three test cases. I'll investigate this after investigating "service-workers/service-worker/fetch-frame-resource.https.html" and fixing "dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html".

Comment 44

[Anne (:annevk)]

> If a same-origin outer request (xml document in this case) is intercepted with a cross-origin CORS request by the SW, should we still use the final Response.url?  In this case the feature (xml doc) does not expect to see a cross-origin URL at all.  Do we really want to start exposing a cross-origin documentURI here?

I don't really see the problem. Security checks don't rely on the URL of a document. I think we knew going on that this was a mismatch with browser architecture, but we thought it was a better model to put the authority with the response, rather than the request. We can revisit that of course, especially with it still not being implemented, but I don't think anything has changed meanwhile.

Comment 45

[Tom Tung [:tt, :ttung]]

re-push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bc9cad98d8ddf01217f5c402481581cbd39cc9&selectedJob=142647303

I find out I overlook three broken tests:

1. dom/workers/test/serviceworkers/test_devtools_track_serviceworker_time.html
  This test left a service worker registered without cleaning it up

Seems that it's affected by the failure of "test_devtools_serviceworker_interception.htm". I can pass the test in my local machine.

2. /service-workers/service-worker/redirected-response.https.html
  Unexpected-Pass

3. /service-workers/service-worker/worker-interception.https.html 
  Verify worker script intercepted by cors response succeeds - promise_test: Unhandled rejection with value: undefined

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Tom, I think we need to add some "channel is synthesized, disable URL checks" load flag.  The security manager would then need to look for this to bypass its checking.  We'll need to have some scary comments indicating this should not be used outside of service worker interception, etc.

Comment 47

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #46)
> Tom, I think we need to add some "channel is synthesized, disable URL
> checks" load flag.  The security manager would then need to look for this to
> bypass its checking.  We'll need to have some scary comments indicating this
> should not be used outside of service worker interception, etc.

Thanks for the suggestion! I'll try to do that!

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #47)
> (In reply to Ben Kelly [:bkelly] from comment #46)
> > Tom, I think we need to add some "channel is synthesized, disable URL
> > checks" load flag.  The security manager would then need to look for this to
> > bypass its checking.  We'll need to have some scary comments indicating this
> > should not be used outside of service worker interception, etc.
> 
> Thanks for the suggestion! I'll try to do that!

So, we spoke about this issue at the service worker face-to-face today.  We actually came around to thinking that perhaps we should be blocking CORS cross-origin Response from being synthesized for a same-origin Request.  This is different from what the fetch spec has said for a long time, though, and different from Anne's read in comment 44.

I'm going to file a fetch spec issue about this, but its unclear to me which way its going to go.

Do you want to continue investigating here or switch to something else while we sort the desired behavior out?  Sorry for the confusion.

Comment 49

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #48)
> So, we spoke about this issue at the service worker face-to-face today.  We
> actually came around to thinking that perhaps we should be blocking CORS
> cross-origin Response from being synthesized for a same-origin Request. 
> This is different from what the fetch spec has said for a long time, though,
> and different from Anne's read in comment 44.
> 
> I'm going to file a fetch spec issue about this, but its unclear to me which
> way its going to go.
> 
> Do you want to continue investigating here or switch to something else while
> we sort the desired behavior out?  Sorry for the confusion.

I'll focus on the other issues (other broken test) found in this bug first. Thanks for the information!

Comment 50

[Tom Tung [:tt, :ttung]]

Attachment: Propagate the URLs from respondWith() if the response's URL is different from the request's and the response has been redirected

Sending the patches finished so far for review.

Hi Ben,

This patch is the main changes for this bug. Basically, when the response's URL is diffenet from the request's URL, it makes the interceptedChannel do the internal redirect if response.redirected is false and do the real redirect if response.redirected is true.

For the internal redirect, I follow the current logic which use the REDIRECT_INTERNAL flag to open a channel. For the real redirect, I choose the REDIRECT_TEMPORARY flag [1] to open a channel, since the redirect is caused by a service worker. The upcoming request should re-validate if the service worker is somehow terminated.

Would you mind helping me to review this patch? Thanks!

Note: feel free to drop the request if the final decision for synthesizing a CORS cross-origin request to a same-origin request may change the decision for doing this bug. 

[1] https://searchfox.org/mozilla-central/source/netwerk/base/nsIChannelEventSink.idl#29

Comment 51

[Tom Tung [:tt, :ttung]]

Attachment: Fix the test for verifying synthisized response in netwerk

This patch is for fixing the broken netwerk xpcshell test due to having addtional argument after applying P1

Comment 52

[Tom Tung [:tt, :ttung]]

Attachment: Fix the test_fetch_basic_http_sw_reoute.htnl

This patch is to fix the test which checks the response.url won't change even the service worker redirects its request to another. The behavior changes since P1, so fix the test.

Comment 53

[Tom Tung [:tt, :ttung]]

Attachment: Freeze the tinting if a service worker responds with a synthesize repsonse

This patch is to add a flag on LoadInfo, so that we can decide not to change the tainting if the response is a synthesized by a service worker.

Note: I'm not sure about if I need to pass this flag through IPC. I do it for now. For current architecture, I _think_ we won't. However, I'm not sure whether we still check the channel/update the tainting on content process (hijack on chrome process).

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Tom, I'll probably wait to review here until after we get some clarification on the spec.  I'll leave the flags for now, though.

Also, here is the spec issue I filed:

https://github.com/whatwg/fetch/issues/629

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Tom, sorry I haven't had a chance to do reviews here yet.  I just never had enough time to sit and think while at TPAC and while traveling.  I'll take a look tomorrow.

I think we do have some general idea of where we want to go from a spec perspective, though.  Let me describe that since I think some of it may be actionable before I review this here.

To refresh the core question is what to do if a `evt.respondWith()` gets a `Response.type === 'cors'` while `evt.request.mode === 'same-origin'`.  Our initial approach was to propagate the `Response.url` which ended up triggering failures in nsSecurityManager due to the same-origin policy.

We discussed this a bit at the TPAC face-to-face and in this issue:

https://github.com/whatwg/fetch/issues/629

I believe what we all want to do now is to actually fail the network request in this case.  So this mostly matches what our nsSecurityManager is doing already.

The question is, can we safely make a CORS response passed for a same-origin mode request fail without breaking existing sites?

To that end, I was wondering if you could file another bug to track telemetry or a use counter for this case?

Ideally we would land this soon and maybe even uplift to beta.  The use counter would be ideal since it measured per document loaded (I think), but it also needs to an nsIDocument to reference.  So you would have to get that out of the LoadInfo or nsIChannel.  I'm not sure if a dedicated Worker script has a document reference like this.  I really want to know if worker scripts are running into this since its one of the more common causes of a same-origin mode Request.

If we can't do the use counters reasonably, maybe we just fall back to telemetry that gives us a count of CORS Response with same-origin mode Request vs total count of synthesized responses.  We can then at least get a percentage of the total network requests even if its not normalized to the number of pages loaded.

While that is collecting data, we could then proceed with implementing the Response.url propagation and the CORS rejection code.  Until we get a verdict from the data we could add a temporary quirk in the code controlled by a pref.  If the pref has the quirk enabled then we would do the equivalent of this when we hit the CORS response with same-origin mode Request:

  var quirkResponse = new Response(corsResponse.body, corsResponse);

Basically synthesize a new Response object just like the service worker script could do.  This will result in an empty string Response.url and may lose some non-standard headers.  This Response would then pass the Response validation code as its no longer CORS and it no longer has a URL to be propagated.

Hopefully, once we have data we can then disable the pref to turn off the quirk.  We could also do this if the other browsers agree to do it at the same time, even if it is breaking.

Also, I realized that there is another case where we do not want to propagate the Response.url.  The service worker and html spec are actually written in this weird way that causes the Request.url to always be used for `Request.mode === 'navigation'`.  If the mode indicates its a navigation then we probably do not want to do the fake redirect to propagate the URL.  We don't need to worry about the Response.redirected flag here because these requests will also be `Request.redirect === 'manual'` which should fail on a redirected Response.

What do you think?  Sorry this issue kind of turned into a mess.

Comment 56

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #55)
Keep the ni flag because I need a bit more time to digest this...

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8926220 [details] [diff] [review]
Propagate the URLs from respondWith() if the response's URL is different from the request's and the response has been redirected

Review of attachment 8926220 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks really good.  I think it just needs to be updated to handle the navigation case I forgot to mention originally.

Also, I think we need to add the quirk/rejection code for the "CORS response and same-origin mode request" case here:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#609

That one could reasonably be done as a follow-up, though.

::: dom/workers/ServiceWorkerEvents.cpp
@@ -278,5 @@
>    StartResponse(nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
>                  InternalResponse* aInternalResponse,
>                  const ChannelInfo& aWorkerChannelInfo,
>                  const nsACString& aScriptSpec,
> -                const nsACString& aResponseURLSpec,

Sorry, I think I changed requirements on you such that we need to keep passing the URL here.

In RespondWithHandler::ResolvedCallback() we need to check for a navigation request and pass empty string in that case.  Something like:

  nsCString url;
  if (mRequestMode != RequestMode::Navigate) {
    responseURL = ir->GetUnfilteredURL();
  }

And then pass responseURL to StartResponse() constructor.

This is to handle the case where someone synthesizes a Response with a different URL for a document navigation request.  In that case we want to make sure the resulting window location is the original Request.url and does not use the Response.url.

@@ +661,5 @@
>    // When an opaque response is encountered, we need the original channel's principal
>    // to reflect the final URL. Non-opaque responses are either same-origin or CORS-enabled
>    // cross-origin responses, which are treated as same-origin by consumers.
> +  if (response->Type() == ResponseType::Opaque &&
> +      NS_WARN_IF(ir->GetUnfilteredURL().IsEmpty())) {

This is basically an extra safety check to make sure our invariant that opaque responses always have a URL does not break.  I think we should probably check this for CORS responses as well.  It would also be useful to add an assertion here.

Maybe something like this:

if (NS_WARN_IF((response->Type() == ResponseType::Opaque ||
                response->Type() == ResponseType::CORS) &&
               ir->GetUnfilteredURL().IsEmpty()))) {
  MOZ_DIAGNOSTIC_ASSERT(false, "CORS or opaque Response without a URL");
  return;
}

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1734,5 @@
>    LOG(("HttpChannelChild::BeginNonIPCRedirect [this=%p]\n", this));
>  
> +  const uint32_t redirectFlag =
> +    aRealRedirect ? nsIChannelEventSink::REDIRECT_TEMPORARY
> +                  : nsIChannelEventSink::REDIRECT_INTERNAL;

Maybe add a comment that we are using REDIRECT_TEMPORARY here, but the exact type does not matter as long as its not REDIRECT_INTERNAL.

::: netwerk/protocol/http/InterceptedHttpChannel.cpp
@@ +236,1 @@
>    // the given cross-origin response URL.  The resulting channel will then

This is not always a cross-origin URL any more.  Probably need to clarify this whole comment to indicate it can be called for many different URLs now.  It allows content to see the final URL where appropriate and also helps us enforce cross-origin restrictions.

Also, could we name this RedirectForResponseURL() or something?  I just want to somehow clarify its different from a 30x redirect.

::: netwerk/protocol/http/InterceptedHttpChannel.h
@@ +114,5 @@
>    FollowSyntheticRedirect();
>  
> +  // If the response's URL is different from the request's, do a real redirect
> +  // while the response has not been redirected yet. Otherwise, do an internal
> +  // redirect.

nit:  Maybe clarify this a bit.  Maybe something like:

  // If the response's URL is different from the request's then do a
  // redirect. If Response.redirected is false we do an internal redirect.
  // Otherwise, if Response.redirect is true to a non-internal redirect
  // so end consumers detect the redirected state.

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8926229 [details] [diff] [review]
Freeze the tinting if a service worker responds with a synthesize repsonse

Review of attachment 8926229 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should need this one any more now that we are planning to reject/quirk the "CORS response on same-origin mode request" case.  Do you agree?

Comment 59

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #55)
Thanks for providing information, the feeback and the reviews!

I'll try to implement this after adding the telemetry/use counter. I feel like we should test the behaviors under different combinations of request.mode, request.redirect and response.type.

Comment 60

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6891fdce23a270d809945c9ca81e5da0b50931d

Comment 61

[Tom Tung [:tt, :ttung]]

Attachment: Propagate the URLs from respondWith() if the response's URL is different from the request's and the response has been redirected

Address the comment.

Comment 62

[Tom Tung [:tt, :ttung]]

Comment on attachment 8926229 [details] [diff] [review]
Freeze the tinting if a service worker responds with a synthesize repsonse

Remove the patch per comment #58

Comment 63

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #57)
> Also, I think we need to add the quirk/rejection code for the "CORS response
> and same-origin mode request" case here:
> 
> https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.
> cpp#609
> 
> That one could reasonably be done as a follow-up, though.

I'll file a follow-up bug to track it.

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #63)
> I'll file a follow-up bug to track it.

I actually was hoping for a follow-up patch in this bug.  I don't think we can wait for this since we need it to avoid having to change the security manager.

Comment 65

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #58)
> Comment on attachment 8926229 [details] [diff] [review]
> Freeze the tinting if a service worker responds with a synthesize repsonse
> 
> Review of attachment 8926229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think we should need this one any more now that we are planning to
> reject/quirk the "CORS response on same-origin mode request" case.  Do you
> agree?

Sorry Ben, I feel like I still not understand about this. So, the original reason for doing this patch is:
- SW intercepts an cross origin URL with "no-cors" mode and responds by fetching another URL with "cors" mode and its headers

Which response.type is the content excepted to have? It's "opaque" or "cors"? 

Then, we decide to have this patch (freeze the tatinting so that it won't be increase after the redirect) as your suggestion in comment #36. So that we could get the response with its type is "cors" because it won't be increased by nsContentSecurityManager.

So, I don't understand why we don't need this patch?

BTW, just to make sure I understand correctly, we are going to have a perf and it's enabled by default. If there is an CORS response on same-origin mode request, we should make a quirkResponse for it, right?

Also, I have one more question about the perf.

I find out we need to get the value perf on the main thread. However, while we are in the code for respondWith(), we are on the worker thread. Currently, I have two naive thoughts to solve this problem.

One is to read the perf while the service worker is registering/updating, but we cannot be aware of the change of the pref immediately. The other one is to jump into the main thread synchronously while we are in the condition for the CORS response on same-origin mode request. However, I'm a a bit afraid this might slow down the service worker.

Do you have any suggestion about that? Thanks!

Comment 66

[Tom Tung [:tt, :ttung]]

Attachment: Fix for the browser_devtools_serviceworker_interception

Comment 67

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P1: Propagate the URLs from respondWith() if the response's URL is different from the request's. r?bkelly

Ben, I address the comment. Also, I change the arguemnt "aRealRedirect"'s name to "aResponseRedirected" for being more clearly. Could you help me to review it? Thanks!

Comment 68

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P2: Fix the netwerk xpcshell test since adding an argument to startSynthesizedResponse(). r=bkelly

Comment 69

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P3: Fix the test_fetch_basic_http_sw_reroute.html since we decide to propagate the response's URLs to the content. r=bkelly

Comment 70

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P4: Fix the interception test since we decide to propagate the response's URLs to the content. r?bkelly

This patch is to modify the test browser_devtools_serviceworker_interception.js. 

This test used to catch the channel by the request's URL for the most of the cases. However, since we decide to propagate the response's URL to the content, we need to change this test to catch the channel by the expected response's URL. Besides, this makes the final URI of the outer and inner channel be the same, so we need to catch the second channel which fires the notification.

Comment 71

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P4: Fix the interception test since we decide to propagate the response's URLs to the content. r?bkelly

Sorry, there is a typo in the patch.

Comment 72

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=41bc6d185740b2b952fba875122832aefa3fca25

Comment 73

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #57)
> Also, I think we need to add the quirk/rejection code for the "CORS response
> and same-origin mode request" case here:
> 
> https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.
> cpp#609

Only add the quirk code here is not enough. It seems that we also need to modify code on StartSynthesizedResponse(), because it makes the responseURI be the originalURI(channel URI) when the responseURI is an empty string [1, 2].

We need to either passing the request mode to here (not redirect for the navigate mode) or move the logic to here

[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedHttpChannel.cpp#779-781
[2] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/InterceptedChannel.cpp#297-299

Comment 74

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #73)
Sorry, I was wrong. It avoids the security check by passing the original channel URI.

Comment 75

[Tom Tung [:tt, :ttung]]

I get an another issue for synthsizing response while it's a same-origin request and a cors-mode response on fetch-response-taint.https.html. Note there is six failure but all of them fails on expected mode is not cors mode. 

I put one of the failures as follows:

FAIL | /service-workers/service-worker/fetch-response-taint.https.html | fetching url:"https://web-platform.test:8443/?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FACAOrigin%3D*&mode=cors&credentials=omit&" mode:"same-origin" credentials:"omit" should succeed. 

Basically, if we create a quirk response just like the following script:

var quirkResponse = new Response(corsResponse.body, corsResponse);

It will make the type of the response be "default". However, this makes the final response become a response to a basic type [1].

If the description in LoadTainting is still correct [2], I think we should make its type be the "cors".

[1] https://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#295-296
[2] https://searchfox.org/mozilla-central/source/netwerk/base/LoadTainting.h#20

Comment 76

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d733c015f56cc952c6d9d985ca3b1f6b9fb2278

Comment 77

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P5: Remove wpt ini file since passing the test. r?bkelly

Ben, this patch removes the wpt ini file, after the service worker propagates the response URLs to the content when intercepting a request.

Comment 78

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #76)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7d733c015f56cc952c6d9d985ca3b1f6b9fb2278

This push-to-try applies the attached patches in this bug, the patch for freezing tainting (Attachment #8926229 [details] [diff]), and the patch for the simple implementation of the quirk response.

Comment 79

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #65)
> Also, I have one more question about the perf.

I find out that I can expose the pref to workers by adding one more item here [1].

[1] https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h

Comment 80

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P6: Implement the quirk repsonse and add a pref for it. r?beklly

Ben, this patch is mainly to add a pref and implement the quirk response.

For the implementation, I'm a little afraid of whether the patch does the same as the scrip |var response = new Response(corRes.body, corRes)| does. (copy the body & header just like clone, but only copy the mStatus & mStatusText for InternalResponse metadata) Besides, I also copy the type of internalResponse to make type of quirkResponse be cors. 

For the pref, I enable it by default for all the platform. Furthermore, I expose it to worker so that the service worker can access it while it's in worker thread.

Comment 81

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae287b0b3d97699f816062be0f0ae8a393df1200

Comment 82

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #81)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ae287b0b3d97699f816062be0f0ae8a393df1200

This try push is applied the P1 ~ P6 and the Attachment #8926229 [details] [diff]. I expect it to pass all the service worker related tests.

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

Tom, I'm afraid I won't have time to do these reviews today.  I spent all day fixing some crashes I introduced before the holiday and catching up on other mail/reviews.  I'll do these first thing tomorrow.  Sorry for the delay.

Comment 84

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #65)
> (In reply to Ben Kelly [:bkelly] from comment #58)
> > I don't think we should need this one any more now that we are planning to
> > reject/quirk the "CORS response on same-origin mode request" case.  Do you
> > agree?
> 
> Sorry Ben, I feel like I still not understand about this. So, the original
> reason for doing this patch is:
> - SW intercepts an cross origin URL with "no-cors" mode and responds by
> fetching another URL with "cors" mode and its headers
> 
> Which response.type is the content excepted to have? It's "opaque" or
> "cors"? 
> 
> Then, we decide to have this patch (freeze the tatinting so that it won't be
> increase after the redirect) as your suggestion in comment #36. So that we
> could get the response with its type is "cors" because it won't be increased
> by nsContentSecurityManager.
> 
> So, I don't understand why we don't need this patch?

You're correct.  I got confused.  I thought this was for overriding the same-origin restriction in the security manager.

Can you re-flag?

> One is to read the perf while the service worker is registering/updating,
> but we cannot be aware of the change of the pref immediately. The other one
> is to jump into the main thread synchronously while we are in the condition
> for the CORS response on same-origin mode request. However, I'm a a bit
> afraid this might slow down the service worker.
> 
> Do you have any suggestion about that? Thanks!

Since we only need a boolean, I think you can use the WorkerPrefs.h stuff:

https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h#27
https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1566

Does that help?

Comment 85

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #84)
> Since we only need a boolean, I think you can use the WorkerPrefs.h stuff:
> 
> https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h#27
> https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1566
> 
> Does that help?

Note that in Bug 1419771 which should land once :baku addresses weird Windows build failures, WorkerPrefs will no longer do things for boolean prefs, and instead DOMPreferences will come into existence and use the cool new Preferences::Add*Cache methods.  So if you can wait a day or two, you should have those.

Comment 86

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #84)
Thanks for the hints and feedback!

> You're correct.  I got confused.  I thought this was for overriding the
> same-origin restriction in the security manager.
> 
> Can you re-flag?

Sure
 
> Since we only need a boolean, I think you can use the WorkerPrefs.h stuff:
> 
> https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrefs.h#27
> https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp#1566
> 
> Does that help?

Thanks! I used it to expose the pref to the worker in P6 (attachment 8932052 [details] [diff] [review]) after investigating. That verifies I found a right method to use :P

(In reply to Andrew Sutherland [:asuth] from comment #85)
> Note that in Bug 1419771 which should land once :baku addresses weird
> Windows build failures, WorkerPrefs will no longer do things for boolean
> prefs, and instead DOMPreferences will come into existence and use the cool
> new Preferences::Add*Cache methods.  So if you can wait a day or two, you
> should have those.

Thanks for the information! I'll try to use DOMPreferences.

Comment 87

[Tom Tung [:tt, :ttung]]

Comment on attachment 8926229 [details] [diff] [review]
Freeze the tinting if a service worker responds with a synthesize repsonse

Re-flag the patch. The original comment for this patch is in comment #53

Comment 88

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P6a: Implement the quirk response

Split the original P6 patch into two pieces. This one is for the implementation of the quirk response while the other is for the add a pref for it.

I'll send the the other one to review after bug 1419771.

Note: the original comment is in comment #80.

Comment 89

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8931554 [details] [diff] [review]
Bug 1222008 - P1: Propagate the URLs from respondWith() if the response's URL is different from the request's. r?bkelly

Review of attachment 8931554 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/InterceptedHttpChannel.h
@@ +114,5 @@
>    FollowSyntheticRedirect();
>  
> +  // If the response's URL is different from the request's then do a service
> +  // worker redirect. If Response.redirected is false we do an internal
> +  // redirect. Otherwise, if Response.redirect is true to a non-internal

nit: s/to a/do a/

@@ +120,2 @@
>    nsresult
> +  RedirectForResponseURL(nsIURI* aResponseURI, const bool aResponseRedirected);

nit: I don't think the const is necessary on the bool here.

Comment 90

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8932304 [details] [diff] [review]
Bug 1222008 - P6a: Implement the quirk response

Review of attachment 8932304 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/Response.cpp
@@ +344,5 @@
> +    mInternalResponse->Clone(body ? InternalResponse::eDontCloneInputStream
> +                                  : InternalResponse::eCloneInputStream,
> +                             true /* quirkResponse */);
> +
> +  RefPtr<Response> response = new Response(mOwner, ir, mSignal);

I guess I was trying to suggest we do exactly what script can do here.  So call `Response::Constructor(body, origResponse)`.  I think this should just do the right thing.  I guess we might have to handle a ReadableStream body vs a native body slightly differently.  I don't think we need to clone the body, though.

The Response::Constructor() should set the url and response type appropriately.  It should look like a synthetic response created by the script.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +673,5 @@
>    if (mRequestMode == RequestMode::Same_origin &&
>        response->Type() == ResponseType::Cors) {
>      Telemetry::ScalarAdd(Telemetry::ScalarID::SW_CORS_RES_FOR_SO_REQ_COUNT, 1);
>  
> +    response = response->QuirkResponse(aCx, rv);

Will the patch that adds the pref make us reject here if the quirk is disabled?  I think thats what we want to do.

Comment 91

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #90)
Thanks for the reveiw and the feedback! But, I still have a question about expected repsonse type of quirk response, and I put it in below.

> ::: dom/fetch/Response.cpp
> @@ +344,5 @@
> > +    mInternalResponse->Clone(body ? InternalResponse::eDontCloneInputStream
> > +                                  : InternalResponse::eCloneInputStream,
> > +                             true /* quirkResponse */);
> > +
> > +  RefPtr<Response> response = new Response(mOwner, ir, mSignal);
> 
> I guess I was trying to suggest we do exactly what script can do here.  So
> call `Response::Constructor(body, origResponse)`.  I think this should just
> do the right thing.  I guess we might have to handle a ReadableStream body
> vs a native body slightly differently.  I don't think we need to clone the
> body, though.
> 
> The Response::Constructor() should set the url and response type
> appropriately.  It should look like a synthetic response created by the
> script.

I'll try to call Response::Constructor(body, origResponse) on ServiceWorkerEvents to make the quirk response.

However, if we only call Response::Constructor(body, origResponse)[1] to make the quirk response, then it might bring us to another issue.

I checked the Response::Constructor()[1] and it doesn't set the response type from aInit (ResponseInit). It just create a new InternalResponse with aInit.mStatus and aInit.mStatusText. Thus, the response type of the synthesized response will be ResponseType::Default[2]. So that the final response type which the content script will get is ResponseType::Basic and that makes us not be able to pass the test fetch-response-taint.https.html (as I mentioned in comment #75). 

In short, which response type should the content script expect to get for the quirk response? Is it ResponseType::Cors or ResponseType::Basic? If it's ResponseType::Cors, then we need to set the response type except calling Response::Constructor(). If it's ResponseType::Basic, then we need to modify the test.

[1] https://searchfox.org/mozilla-central/source/dom/fetch/Response.cpp#163
[2] https://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#30

> ::: dom/workers/ServiceWorkerEvents.cpp
> @@ +673,5 @@
> >    if (mRequestMode == RequestMode::Same_origin &&
> >        response->Type() == ResponseType::Cors) {
> >      Telemetry::ScalarAdd(Telemetry::ScalarID::SW_CORS_RES_FOR_SO_REQ_COUNT, 1);
> >  
> > +    response = response->QuirkResponse(aCx, rv);
> 
> Will the patch that adds the pref make us reject here if the quirk is
> disabled?  I think thats what we want to do.

Yes, I'll have a check before this line like:

+ if (!DOMPreferences::QuirkResponseEnabled()) {
+   return;
+ }

However, right now, I expose the pref to the worker it via WorkerProf which is about to be removed in bug 1419771. Thus, I'll send the P6b (the patch for add the pref, and expose to worker) once the bug 1419771 is fixed.

Comment 92

[Ben Kelly [:bkelly, not reviewing]]

> I'll try to call Response::Constructor(body, origResponse) on
> ServiceWorkerEvents to make the quirk response.
> 
> However, if we only call Response::Constructor(body, origResponse)[1] to
> make the quirk response, then it might bring us to another issue.
> 
> I checked the Response::Constructor()[1] and it doesn't set the response
> type from aInit (ResponseInit). It just create a new InternalResponse with
> aInit.mStatus and aInit.mStatusText. Thus, the response type of the
> synthesized response will be ResponseType::Default[2]. So that the final
> response type which the content script will get is ResponseType::Basic and
> that makes us not be able to pass the test fetch-response-taint.https.html
> (as I mentioned in comment #75). 

Well, I think the test is going to have to change.  Ideally we will reject in this case.

Can we just mark it expected failure for now?  Is the test flexible enough to just mark this one case expected failure?

> In short, which response type should the content script expect to get for
> the quirk response? Is it ResponseType::Cors or ResponseType::Basic? If it's
> ResponseType::Cors, then we need to set the response type except calling
> Response::Constructor(). If it's ResponseType::Basic, then we need to modify
> the test.

I think we should either reject or make it Basic with the quirk.  Neither one completely matches the spec or the test right now.  Hopefully we end up at rejecting and change the test.

FWIW, I checked telemetry since you landed the probe.  Currently we have ~2 million intercepted responses here:

https://mzl.la/2Bvqkdj

But not a single CORS-response-for-same-origin-request yet.  The telemetry probe doesn't have any values in the site yet AFAICT.

Obviously we need to give it more time, though.  Getting it uplifted to beta would help.

> However, right now, I expose the pref to the worker it via WorkerProf which
> is about to be removed in bug 1419771. Thus, I'll send the P6b (the patch
> for add the pref, and expose to worker) once the bug 1419771 is fixed.

Ok.  As long as we get the pref and rejection in before 59 merges to beta I think its ok.

Comment 93

[Ben Kelly [:bkelly, not reviewing]]

I also asked the blink folks if they have any data yet:

https://github.com/whatwg/fetch/issues/629#issuecomment-347741117

Maybe we should just go with rejection by default for now and punt the quirk/pref to a follow-on bug.

Comment 94

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #92)
Thanks for the quick reply!

> Can we just mark it expected failure for now?  Is the test flexible enough
> to just mark this one case expected failure?

Yes, I think we can. I'll mark it expected failure.

> I think we should either reject or make it Basic with the quirk.  Neither
> one completely matches the spec or the test right now.  Hopefully we end up
> at rejecting and change the test.

I see.

(In reply to Ben Kelly [:bkelly] from comment #93)
> I also asked the blink folks if they have any data yet:
> 
> https://github.com/whatwg/fetch/issues/629#issuecomment-347741117
> 
> Maybe we should just go with rejection by default for now and punt the
> quirk/pref to a follow-on bug.

I could do that.

Comment 95

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2515b612744c255c9a42f7d4dbaacccc75e87e4a

Comment 96

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P1: Propagate the URLs from respondWith() if the response's URL is different from the request's. r=bkelly

Update the patch

Comment 97

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P4: Fix the interception test since we decide to propagate the response's URLs to the content. r=bkelly

Comment 98

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P5: Remove wpt ini file since passing the test. r=bkelly

Comment 99

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly

Ben, this patch becomes pretty simple after deciding to reject the promise by default. Could you help me to review this? Thanks!

Comment 100

[Tom Tung [:tt, :ttung]]

Comment on attachment 8926229 [details] [diff] [review]
Freeze the tinting if a service worker responds with a synthesize repsonse

I find out that I forgot to set this patch not to be obsolete...

Comment 101

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P8: Expected failure for wpt tests since we mean to reject the cors response for same-origin request. r?bkelly

This patch is for expecting wpt tests failure since we decide to reject the promise

Comment 102

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P9: Expected failure for loading a same origin XML Document but being responded with a cors response. r?bkelly

This patch is for expecting failure of loading a same-origin XML Doc but the sw synthesize a cors response.

Comment 103

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6ab1152d0fb5d28ab479ea2aaf6657273f7a8f0

Comment 104

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8932786 [details] [diff] [review]
Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly

Review of attachment 8932786 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.  Thanks!

::: dom/workers/ServiceWorkerEvents.cpp
@@ +674,5 @@
>        response->Type() == ResponseType::Cors) {
>      Telemetry::ScalarAdd(Telemetry::ScalarID::SW_CORS_RES_FOR_SO_REQ_COUNT, 1);
>  
> +    // XXXtt: Will have a pref to enable the quirk response in bug 1419684.
> +    return;

Please include a cancellation message so developers understand why its fail.  Something like this:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#632

Comment 105

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8926229 [details] [diff] [review]
Freeze the tinting if a service worker responds with a synthesize repsonse

Review of attachment 8926229 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and I understand it better now.  Thanks!

::: netwerk/base/LoadInfo.h
@@ +183,5 @@
>    bool                             mMixedContentWouldBlock : 1;
>    bool                             mIsHSTSPriming: 1;
>    bool                             mIsHSTSPrimingUpgrade: 1;
> +
> +  bool                             mIsSynthesizeServiceWorkerTainting;

Can you move this up above the bitfields and next to the other full `bool` methods?  Right after mLoadTriggeredFromExternal.  I think putting it here will create some struct packing wastage.

Comment 106

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8932789 [details] [diff] [review]
Bug 1222008 - P8: Expected failure for wpt tests since we mean to reject the cors response for same-origin request. r?bkelly

Review of attachment 8932789 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/meta/service-workers/service-worker/worker-interception.https.html.ini
@@ +1,3 @@
> +[worker-interception.https.html]
> +  [Verify worker script intercepted by cors response succeeds]
> +    expected: FAIL

Can you add an annotations like this to the failing test cases here?

  bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1419684

Or another bug to come back and fix the test once we finalize the spec?  I guess maybe the spec issue link here would be good.

Comment 107

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8932790 [details] [diff] [review]
Bug 1222008 - P9: Expected failure for loading a same origin XML Document but being responded with a cors response. r?bkelly

Review of attachment 8932790 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/fetch/index.html
@@ +139,5 @@
>    var xmlDoc = document.implementation.createDocument(null, null, null);
>    xmlDoc.load('load_cross_origin_xml_document_cors.xml');
>    xmlDoc.onload = function(evt) {
>      var content = new XMLSerializer().serializeToString(evt.target);
> +    // XXXtt: should be allowed by opening the pref after bug 1419684.

Maybe just replace this comment with a link to the fetch spec issue.

Comment 108

[Tom Tung [:tt, :ttung]]

Attachment: diff for P6

Ben, could you help me to check the cancellation message for rejecting service workers to synthesize a cors response for a same-origin request? It's the inter-diff patch for P6 (attachment 8932786 [details] [diff] [review]). Thanks!

Comment 109

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8933148 [details] [diff] [review]
diff for P6

Review of attachment 8933148 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +195,5 @@
>  RTCPeerConnectionGetStreamsWarning=RTCPeerConnection.getLocalStreams/getRemoteStreams are deprecated. Use RTCPeerConnection.getSenders/getReceivers instead.
>  # LOCALIZATION NOTE: Do not translate "ServiceWorker". %S is a URL.
>  InterceptionFailedWithURL=Failed to load ‘%S’. A ServiceWorker intercepted the request and encountered an unexpected error.
> +# LOCALIZATION NOTE: Do not translate "ServiceWorker", "cors", "Response", "same-origin" or "Request".
> +CorsResponseForSameOriginRequest=ServcieWorker is not allowed to synthesize a cors Response for a same-origin Request.

s/Servcie/Service

I think we should probably log the request.url here.  See what BadOpaqueInterceptionRequestModeWithURL does below.

Comment 110

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #109)
> I think we should probably log the request.url here.  See what
> BadOpaqueInterceptionRequestModeWithURL does below.

Should we log the response.url as well? Or just the request.url?

Comment 111

[Ben Kelly [:bkelly, not reviewing]]

Both URLs sounds good.  Thanks.

Comment 112

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1008383a7f0aefaab058804227f966f499c5569f

Comment 113

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly

Address the comment

Comment 114

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P7: Freeze the tainting if a service worker responds with a synthesize response. r=bkelly

Comment 115

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P8: Expected failure for wpt tests since we mean to reject the cors response for same-origin request. r=bkelly

Comment 116

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P9: Expected failure for loading a same origin XML Document but being responded with a cors response. r=bkelly

Comment 117

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #112)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1008383a7f0aefaab058804227f966f499c5569f

If the result of this try goes green, then I will push them to inbound

(In reply to Tom Tung [:tt] from comment #114)
> Created attachment 8933213 [details] [diff] [review]
> Bug 1222008 - P7: Freeze the tainting if a service worker responds with a
> synthesize response. r=bkelly

Note that I also change the variable name from "IsSynthesizeServiceWorkerTainting" to "ServiceWorkerTaintingSynthesized"

Comment 118

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly

Sorry, I updated the wrong patch

Comment 119

[Tom Tung [:tt, :ttung]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5aff04d76e0818e9b1f27ec5a76ba9fd6793607
Bug 1222008 - P1: Propagate the URLs from respondWith() if the response's URL is different from the request's. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc17f19f55986bad0c1dd70939b949152c0de9b7
Bug 1222008 - P2: Fix the netwerk xpcshell test since adding an argument to startSynthesizedResponse(). r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/1beda261a70875f1155d7ea0f3bcb7d329de1789
Bug 1222008 - P3: Fix the test_fetch_basic_http_sw_reroute.html since we decide to propagate the response's URLs to the content. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/efa0d71107dfcc5b30fac1e5dd72f10e7226e6a8
Bug 1222008 - P4: Fix the interception test since we decide to propagate the response's URLs to the content. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/f112fa183600e6b8e14485147a2b20cef5a0565d
Bug 1222008 - P5: Remove wpt ini file since passing the test. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/c29e6b8b00b618f67b83c20527b449b98834e1eb
Bug 1222008 - P6: Reject the cors synthesized response for same-origin request. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e66dd4b9241a14d7cb5be77404011569d498dcb
Bug 1222008 - P7: Freeze the tainting if a service worker responds with a synthesize response. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7ed01915d23df60989b6b38b2916889fb79914
Bug 1222008 - P8: Expected failure for wpt tests since we mean to reject the cors response for same-origin request. r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/22450268a82f78f84c1db8776f0059c1a1fe7d97
Bug 1222008 - P9: Expected failure for loading a same origin XML Document but being responded with a cors response. r=bkelly

Comment 120

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e5aff04d76e0
https://hg.mozilla.org/mozilla-central/rev/cc17f19f5598
https://hg.mozilla.org/mozilla-central/rev/1beda261a708
https://hg.mozilla.org/mozilla-central/rev/efa0d71107df
https://hg.mozilla.org/mozilla-central/rev/f112fa183600
https://hg.mozilla.org/mozilla-central/rev/c29e6b8b00b6
https://hg.mozilla.org/mozilla-central/rev/8e66dd4b9241
https://hg.mozilla.org/mozilla-central/rev/4a7ed01915d2
https://hg.mozilla.org/mozilla-central/rev/22450268a82f

Comment 121

[Ben Kelly [:bkelly, not reviewing]]

Liz, this is another one that should be in the 59 developer notes.  Not sure the best way to get it in there now.  Can you help me?

It can probably be listed as two different items:

* Service worker interception now propagates the FetchEvent.respondWith()'s Response.url to the outer network request. (bug 1222008)
* FetchEvent.respondWith() triggers a NetworkError if the FetchEvent.request.mode is "same-origin" and the Response.type is "cors". (bug 1222008)

Comment 122

[Ben Kelly [:bkelly, not reviewing]]

I'm going to manually add this to the developer release notes here:

https://developer.mozilla.org/en-US/Firefox/Releases/59

Leaving dev-doc-needed because the documentation should still be updated on MDN I think.

Comment 123

[Ben Kelly [:bkelly, not reviewing]]

I wrote this in the release notes:

* When a service worker provides a Response to FetchEvent.respondWith(), the Response.url value will not be propagated to the intercepted network request as the final resolved URL.  In the past the FetchEvent.request.url was used for this instead.  This means, for example, if a service worker intercepts a stylesheet or worker script, then the provided Response.url will be used to resolve any relative @import() or importScripts() subresource loads. (bug 1222008)

* FetchEvent.respondWith() will now trigger a network error if the FetchEvent.request.mode is "same-origin" and the provided Response.type is "cors". (bug 1222008)

Comment 124

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #123)
> I wrote this in the release notes:
> 
> * When a service worker provides a Response to FetchEvent.respondWith(), the
> Response.url value will not be propagated to the intercepted network request
> as the final resolved URL.  In the past the FetchEvent.request.url was used
> for this instead.  This means, for example, if a service worker intercepts a
> stylesheet or worker script, then the provided Response.url will be used to
> resolve any relative @import() or importScripts() subresource loads. (bug
> 1222008)
> 
> * FetchEvent.respondWith() will now trigger a network error if the
> FetchEvent.request.mode is "same-origin" and the provided Response.type is
> "cors". (bug 1222008)

Hi Ben,

Thanks for doing this. I've updated the release notes to include links to all the relevant ref pages. In terms of documenting this on the ref pages, I've added some information to:

https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith

(see the "Exceptions" and "Browser compatibility" sections - let me know if this reads OK).

But I'm not really sure what to write about the first bullet you added. What page should be updated, and what should the text be?

Comment 125

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #124)
> (In reply to Ben Kelly [:bkelly] from comment #123)
> > I wrote this in the release notes:
> > 
> > * When a service worker provides a Response to FetchEvent.respondWith(), the
> > Response.url value will not be propagated to the intercepted network request
> > as the final resolved URL.  In the past the FetchEvent.request.url was used
> > for this instead.  This means, for example, if a service worker intercepts a
> > stylesheet or worker script, then the provided Response.url will be used to
> > resolve any relative @import() or importScripts() subresource loads. (bug
> > 1222008)
> > 
> > * FetchEvent.respondWith() will now trigger a network error if the
> > FetchEvent.request.mode is "same-origin" and the provided Response.type is
> > "cors". (bug 1222008)
> 
> Hi Ben,
> 
> Thanks for doing this. I've updated the release notes to include links to
> all the relevant ref pages. In terms of documenting this on the ref pages,
> I've added some information to:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith
> 
> (see the "Exceptions" and "Browser compatibility" sections - let me know if
> this reads OK).

That's not quite correct.  There were existing combinations of Request.mode and Response.type that would trigger a network error.  We just added an additional one specific to `request.mode === 'same-origin' && response.type === 'cors'`.  The relevant spec issue is here if it helps:

https://github.com/whatwg/fetch/issues/629

> But I'm not really sure what to write about the first bullet you added. What
> page should be updated, and what should the text be?

Yea, its hard to describe.  Its really about how service worker interception interacts with the network request being intercepted.  The impact depends on what is being intercepted.  For most types of network requests this change has no impact because you can't observe the final URL.  There are a few, though, where it does matter:

1. If fetch() is intercepted, then you can observe the final URL on the result Response.url.
2. If a worker script is intercepted, then the final URL is used to set self.location and used as the base URL for relative URLs in the worker script.
3. If a stylesheet is intercepted, then the final URL is used as the base URL for resolving relative @import loads.

This was changed in the spec years ago:

https://github.com/whatwg/fetch/pull/146#issuecomment-341710548

But we are only now implementing it.  In other browsers its only partially implemented.  For example, chrome will surface the URL in an intercepted fetch(), but does not use the final URL for worker scripts.  (It doesn't use final URL for worker scripts doing a normal redirect either, so maybe its just an unrelated bug.)

Note, navigation requests don't surface the final URL because of oddities of the navigation spec language.  It ends up using the request URL there.

Maybe this can go on FetchEvent.resolveWith() page as a special note?

Comment 126

[Ben Kelly [:bkelly, not reviewing]]

Chris, chrome is also working on this under this feature:

https://www.chromestatus.com/feature/5694278818856960

Comment 127

[Chris Mills [:cmills]]

Hi Ben,

I've had another go at this, having added some information on both points to the respondWith page:

https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith

Does this stuff sound OK? I've constructed it from your text. Feel free to edit as appropriate.

Comment 128

[Ben Kelly [:bkelly, not reviewing]]

Thanks Chris.  It looked mostly good, but I did make a few edits.  I also added a paragraph about window navigation requests.  Do you think you could help me linkify that paragraph?

Comment 130

[Chris Mills [:cmills]]

(In reply to Ben Kelly [:bkelly] from comment #128)
> Thanks Chris.  It looked mostly good, but I did make a few edits.  I also
> added a paragraph about window navigation requests.  Do you think you could
> help me linkify that paragraph?

This looks great; I've linkified it, and made a couple of additional edits. Thanks a lot for your help on this.

Comment 131

[Karl Tomlinson (:karlt)]

(In reply to Ben Kelly [:bkelly, not reviewing] from comment #36)

(In reply to Tom Tung [:tt] from comment #35)

dom/workers/test/serviceworkers/test_fetch_event_with_thirdpartypref.html

• Error: synthesize CORS response should result in outer CORS response
• Solution: Since we do the internal redirect while a service worker
  responds with a different URL (without redirected), we should get opaque
  type of response.

Wait, the type of the response should still be CORS. I think probably the
outer channel is no-cors and is hitting this path in its security code:

That was https://hg.mozilla.org/mozilla-central/annotate/8e2976d8a5e1c776e94774b4cc09087940bd8166/dom/security/nsContentSecurityManager.cpp#l712