Closed Bug 1206124 Opened 5 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 + fixed
firefox44 + fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(2 files, 7 obsolete files)

7.23 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
5.56 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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.
Summary: Implement "omit" RequestCredentials in Fetch with RequestMode "cors" → Fetch does not handle "same-origin" credentials in CORS mode properly
I need this to implement my tests in bug 1201160.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
I will add some tests in P2.
Attachment #8662997 - Flags: review?(nsm.nikhil)
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
Attachment #8662997 - Flags: review?(nsm.nikhil)
Attachment #8662997 - Attachment is obsolete: true
Attachment #8663141 - Flags: review?(nsm.nikhil)
Comment on attachment 8663141 [details] [diff] [review]
P1 Fix "same-origin" and "cors" credentials in FetchDriver. r=nsm

Forgot something.
Attachment #8663141 - Attachment is obsolete: true
Attachment #8663141 - Flags: review?(nsm.nikhil)
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.
Attachment #8663149 - Attachment is obsolete: true
Attachment #8666953 - Flags: review?(ehsan)
Update to remove a debugging statement I left in by accident.
Attachment #8666953 - Attachment is obsolete: true
Attachment #8666953 - Flags: review?(ehsan)
Attachment #8666956 - Flags: review?(ehsan)
Computers are hard.
Attachment #8666956 - Attachment is obsolete: true
Attachment #8666956 - Flags: review?(ehsan)
Attachment #8666958 - Flags: review?(ehsan)
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.
Attachment #8666958 - Flags: review?(ehsan) → review+
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.
Depends on: 1210413
Attachment #8668121 - Attachment is obsolete: true
Attachment #8668488 - Flags: review?(ehsan)
Attachment #8668488 - Flags: review?(ehsan) → review+
Add some test cases that Jonas requested in bug 1210413 for XHR.  We should do them for fetch() as well.
Attachment #8668488 - Attachment is obsolete: true
Attachment #8669689 - Flags: review+
Jonas, just FYI about my patches here in case they relate/interact with your current WIP patches.
Flags: needinfo?(jonas)
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
Attachment #8666958 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/75ac148466c6
https://hg.mozilla.org/mozilla-central/rev/8be7799b8529
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Attachment #8666958 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking this for 43 and 44 since it's part of a new feature.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.