1206124 - Fetch does not handle "same-origin" credentials in CORS mode properly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I need this to implement my tests in bug 1201160.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix "same-origin" and "cors" credentials in FetchDriver. r=nsm

I will add some tests in P2.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 ([e-mail]) 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_ ([e-mail]) has quit (A TLS packet with unexpected length was received.)
[13:49:09] --> gavin ([e-mail]) 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 ([e-mail]) 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 ([e-mail]) has joined #content
[13:51:12] <bkelly> does that make sense?
[13:51:55] --> ferjm ([e-mail]) has joined #content
[13:52:15] <bkelly> nsm: I'll upload a new patch
[13:55:20] <-- ferjm ([e-mail]) has quit (Quit: Textual IRC Client: www.textualapp.com)
[13:57:13] --> jesup ([e-mail]) has joined #content
[13:57:22] <-- jesup ([e-mail]) has left #content ("")
[14:04:59] <nsm|away> bkelly: you are right

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix "same-origin" and "cors" credentials in FetchDriver. r=nsm

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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

Forgot something.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix "same-origin" and "cors" credentials in FetchDriver. r=nsm

Current version.  Let me test before flagging for review.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix "same-origin" CORS credentials in FetchDriver. r=ehsan

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.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix "same-origin" CORS credentials in FetchDriver. r=ehsan

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix "same-origin" CORS credentials in FetchDriver. r=ehsan

Computers are hard.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test fetch() with credentials and redirects. r=ehsan

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test fetch() with credentials and redirects. r=ehsan

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test fetch() with credentials and redirects. r=ehsan

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 20

[Pulsebot]

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

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 24

[Carsten Book [:Tomcat]]

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