The fetch spec has three values for RequestCredentials: 1) "include" which corresponds to .crossOrigin = "use-credentials" 2) "same-origin" which corresponds to .crossOrigin = "anonymous" 3) "omit" which never sends credentials regardless of origin Today have a boolean flag in nsCORSListenerProxy called mWithCredentials. If true, then we get "include" behavior where credentials are sent to all origins. If false, then we get "same-origin" behavior where credentials are only sent to same-origin when we have never redirected through a cross-origin. We need to properly implement "same-origin" in Fetch, so it is currently passing (or will very shortly) false for mWithCredentials to nsCORSListenerProxy for both "same-origin" and "omit". We need to add a third configuration setting inside the CORS proxy in order to properly implement "omit" RequestCredentials, though.

Actually, I realize I can fix this without touching nsCORSListenerProxy. I will use this bug to fix both the current "same-origin" credentials issue in FetchDriver and the "omit" setting.

I need this to implement my tests in bug 1201160.

I will add some tests in P2.

Comment on attachment 8662997 [details] [diff] [review] P1 Fix "same-origin" and "cors" credentials in FetchDriver. r=nsm Needs some changes we discussed on IRC: [13:39:02] <nsm|away> bkelly: FetchDriver.cpp line 429 already sets the anonymous flag [13:39:05] <nsm|away> is that not enough? [13:39:07] <bholley> bkelly: ok I'll look at that, thanks [13:39:25] <nsm|away> bholley: GetResolvedScriptURI() will only work after the script has been loaded [13:39:35] <bkelly> nsm: no... thats not propagated on redirects AFAIK [13:39:40] <bholley> nsm: that should be fine, no? [13:39:58] <nsm|away> yes, just making sure you aren't trying to get the uri before the load [13:40:03] <bholley> nsm: because the SW is clearly loaded by the time we do RespondWith [13:40:12] <bholley> nsm: this is about doing CSP checks in respondWith in the case of synthetic responses [13:40:47] <bkelly> bholley: RespondWithHandler has a ServiceWorker handle... and then ServiceWorker::GetWorkerPrivate() [13:41:17] <nsm|away> bkelly: shouldn't the channel flags be propagated with all the other flags? [13:41:27] <bkelly> you would have to propagate the ServiceWorker handle to the RespondWithClosure and pass to FinishResponse, though [13:42:01] <-- davidb (davidb@moz-efp51a.dsl.bell.ca) has quit (Ping timeout: 121 seconds) [13:43:02] <bkelly> hmm [13:43:57] <bkelly> nsm: I guess it does... let me check if nsCORSListenerProxy every clears it [13:44:00] <bkelly> https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2303 [13:44:37] <bkelly> nsm: it seems cors proxy only adds the flag, but never clears it: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp?case=true&from=nsCORSListenerProxy.cpp#1046 [13:44:46] <bkelly> so I guess "omit" was already working [13:45:17] <bkelly> we just need to pass true to cors proxy for "same-origin" and "include" regardless of current cors flags setting [13:45:37] <bkelly> nsm: so I think I can lose my changes to the redirect handler in FetchDriver [13:46:29] <nsm|away> bkelly: i think line 422 sets the flag correctly for passing to nscorslistenerproxy [13:46:36] <nsm|away> do you mean drop the !aCORSFlag check? [13:47:06] <bkelly> nsm: just a sec [13:48:31] <bkelly> nsm: we can't there... because if the first channel load is same-origin (aCorsFlag false), then we don't want it to be LOAD_ANONYMOUS [13:48:49] <bkelly> nsm: but we must pass true always to cors proxy even in that case so it applies LOAD_ANONYMOUS on redirect [13:49:06] <-- gavin_ (gavin@moz-hbsjla.cable.rogers.com) has quit (A TLS packet with unexpected length was received.) [13:49:09] --> gavin (gavin@moz-hbsjla.cable.rogers.com) has joined #content [13:49:10] <bkelly> must pass true to cors proxy when same-origin credentials so it applies LOAD_ANONYMOUS on redirect [13:49:30] <bkelly> nsm: so cors proxy does this: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp?case=true&from=nsCORSListenerProxy.cpp#1041 [13:49:43] <-- ehsan (ehsan@moz-i5m.05u.207.66.IP) has quit (Connection closed) [13:50:00] <bkelly> nsm: it doesn't set LOAD_ANONYMOUS for same origin sites, though, because it short circuits here: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp?case=true&from=nsCORSListenerProxy.cpp#986 [13:50:21] <bkelly> nsm: so true aWithCredentials to cors proxy is equivalent to "same-origin" RequestCredentials [13:50:30] --> ehsan (ehsan@moz-smaemc.ckpj.s0pt.0450.2001.IP) has joined #content [13:51:12] <bkelly> does that make sense? [13:51:55] --> ferjm (textual@moz-3sc64t.staticIP.rima-tde.net) has joined #content [13:52:15] <bkelly> nsm: I'll upload a new patch [13:55:20] <-- ferjm (textual@moz-3sc64t.staticIP.rima-tde.net) has quit (Quit: Textual IRC Client: www.textualapp.com) [13:57:13] --> jesup (chatzilla@moz-l7d6u7.fios.verizon.net) has joined #content [13:57:22] <-- jesup (chatzilla@moz-l7d6u7.fios.verizon.net) has left #content ("") [14:04:59] <nsm|away> bkelly: you are right

Comment on attachment 8663141 [details] [diff] [review] P1 Fix "same-origin" and "cors" credentials in FetchDriver. r=nsm Forgot something.

Current version. Let me test before flagging for review.

I'm sorry, I'll have to finish this after I get back from PTO. The current version is hitting one of my LOAD_ANONYMOUS redirect asserts when I run my tests in bug 1201160.

BTW, I think we should be able to test these cases in test_fetch_cors.js relatively easy.

Still finalizing the tests, but this passes my initial local testing and I believe is correct. There are a few related fixes here: 1) Make sure we also pass false for credentials when creating the nsCORSListenerProxy when RequestCredentials is "same-origin". This is necessary to allow the proxy to set the LOAD_ANONYMOUS flag correctly for cross-origin redirects, etc. 2) Set the LOAD_ANONYMOUS flag manually during redirects when in "no-cors" mode since the CORS proxy isn't there to do it for us. 3) Assert the LOAD_ANONYMOUS flag is set correctly during redirects.

Update to remove a debugging statement I left in by accident.

Computers are hard.

Sorry, I'll have to remove that printf_stderr later. My computer is having some kind of cache coherence issue reading from my SMB share.

These tests fail before the P1 and pass after the P1. On non-e10s. On e10s, though, we still fail them after the P1. I need to investigate.

Add some test cases that Jonas requested in bug 1210413 for XHR. We should do them for fetch() as well.

Jonas, just FYI about my patches here in case they relate/interact with your current WIP patches.

Comment on attachment 8666958 [details] [diff] [review] P1 Fix "same-origin" CORS credentials in FetchDriver. r=ehsan Approval request for P1 and P2. Approval Request Comment [Feature/regressing bug #]: fetch and service workers [User impact if declined]: I would like to uplift this to maintain compatibility with further service worker patches. It fixes a case where we are over conservative in blocking credentials to cross-origin redirects in fetch(). Some of the service worker tests depend on this fix. [Describe test coverage new/current, TreeHerder]: mochitests included in this bug and web-platform-tests included in later bugs [Risks and why]: risk limited to fetch() [String/UUID change made/needed]: none

https://hg.mozilla.org/mozilla-central/rev/75ac148466c6 https://hg.mozilla.org/mozilla-central/rev/8be7799b8529

https://hg.mozilla.org/mozilla-central/rev/75ac148466c6 https://hg.mozilla.org/mozilla-central/rev/8be7799b8529

Comment on attachment 8666958 [details] [diff] [review] P1 Fix "same-origin" CORS credentials in FetchDriver. r=ehsan Includes tests, important for service worker feature. OK to uplift.

Tracking this for 43 and 44 since it's part of a new feature.

https://hg.mozilla.org/releases/mozilla-aurora/rev/59ab27502812 https://hg.mozilla.org/releases/mozilla-aurora/rev/60f2b7ff359d