1171215 - [e10s] Websockets upgrade request does not include cookies with e10s

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[silverwind]

I have an app that connects to wss:// on the same origin, and it seems that with e10s enabled, no cookie headers are sent over the websocket. Once I disable e10s, cookie headers are there as expected.

Comment 1

[Patrick McManus [:mcmanus]]

interesting. michal do you want to dive into it?

Comment 2

[Michal Novotny [:michal]]

I'll have a look at it.

Comment 3

[silverwind]

I can also confirm that regular unencrypted websockets don't seem to send any cookie headers under e10s.

Comment 4

[Michal Novotny [:michal]]

Is there any testcase available showing this problem?

Comment 5

[silverwind]

I've set up a simple server to echo headers received by http and websocket here:

  http://wstest.silverwind.io/

The page sets a cookie 'some=value', you need to refresh the page once to see it in the headers. If you don't see the cookie appearing on the websocket headers (which are obtained from the upgrade request), you have reproduced the bug.

Comment 6

[Michal Novotny [:michal]]

I can't reproduce the bug. I tested it with latest m-c source as well as with firefox developer edition (on linux and windows). I always see an expected behavior, i.e. during the first load the cookie is present only in websocket headers, during any subsequent load the cookie is present in both headers.

Do I understand correctly that you see the cookie in HTTP headers but not in websocket headers when reloading the page? Or is it missing in HTTP headers too? What addons do you have installed?

Could you please provide a NSPR log of the load and reload? Use NSPR_LOG_MODULE=nsHttp:5,cookie:5 and don't forget to set GECKO_SEPARATE_NSPR_LOGS=1.

Comment 7

[silverwind]

You're right. It looks to be addon or profile related as I'm not seeing it in safe mode. I'll try to track it down further.

Comment 8

[silverwind]

Or rather, scrap that. I was having an weird issue with page rendering in safe mode (OS X) so I disabled Hardware acceleration, unknowingly disabling e10s in that test too.

So, it's still happening for me. The issue is that the websocket upgrade request never includes any cookie. not on first load or any subsequent loads.

I'll try your logging suggestion and/or a fresh profile.

Comment 9

[silverwind]

I think I found the cause. From the log:

2018988032[102025400]: ===== COOKIE NOT SENT =====
2018988032[102025400]: request URL: http://wstest.silverwind.io/
2018988032[102025400]: current time: Mon Jul 13 22:25:18 2015 GMT
2018988032[102025400]: rejected because context is third party

You should be able to reproduce with third-party cookies disabled and e10s enabled. For some reason the Upgrade request is wrongly considered third-party it seems. If you need more details from the log, I could mail it to you on request.

Comment 10

[Michal Novotny [:michal]]

silverwind, thanks for the test case. I can reproduce it when third-party cookies are disabled. Websocket's http channel is considered as third party channel in e10s but not in single process. There is no associated window in the parent process so we fail here at http://hg.mozilla.org/mozilla-central/annotate/e786406bc683/dom/base/ThirdPartyUtil.cpp#l275. I don't know this code. Blake, could you please have a look at it?

Comment 11

[Jason Duell]

Steve, would anyone on your team be a logical person to figure out why Websocket bootstrap channels are being treated as 3rd party (only in e10s)?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

This is because we're sticking a ton of stuff on nsIHttpChannelInternal. For bug 1049299, I added two flags, "THIRD_PARTY_IS_{THIRD,SAME}_PARTY" that are computed in the child and passed to the parent so that the parent doesn't have to do any computation on DOM windows that we can't pass to it. However, because that information only lives on nsIHttpChannelInternal[1], the websocket channel ends up trying to do a computation that it doesn't have enough data for.

The easiest way to fix this bug would probably be to pull the third-party stuff out of nsIHttpChannelInternal and into some other interface (nsIThirdPartyChannel?) and pass the same information, hopefully refactored, with websocket channels in order to use it in ThirdPartyUtil.cpp.

[1] http://hg.mozilla.org/mozilla-central/annotate/e786406bc683/dom/base/ThirdPartyUtil.cpp#l194

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Looks like Blake had the answer here. Clearing needinfo.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Todo: Check what happens for shared workers and dedicated workers (and dedicated workers in shared workers) for the cases where the workers are created by iframes that are both same- and cross-origin to their parents.

For shared workers, we should consider the request to be third party and block cookies if the pref requests it and for dedicated workers ideally we should figure out if we can reach a document and use that to check thirdparty-ness.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I still need to write tests for this (and to test the answers to comment 14: shared workers' cookies are always considered to be third-party and dedicated workers' cookies are correctly attributed to the page that created them) but this patch seems to work. I have a try run at:

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

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8675978 [details] [diff] [review]
Patch v1

This is rather orange on try.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I need to think about this more. The loadinfo doesn't have enough information in its constructor (oddly enough) to do this calculation.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

We can probably do this in nsIOService::NewChannel...Internal.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

This patch seems more correct. I still have to make sure that redirects work correctly and I need to step through e10s's codepath since I think it currently tries to calculate third-partyness in the parent as well.

Jonas, what do you think?

Comment 20

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

Comment on attachment 8678393 [details] [diff] [review]
Patch v2

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

Looks like a good approach.

::: dom/base/ThirdPartyUtil.cpp
@@ +216,5 @@
> +    // the child and send the result to the parent via the loadinfo.
> +    // Note: All channels *should* have a loadinfo, but it's possible for
> +    // extensions to create one that doesn't. In that case, we'll try to check
> +    // the parent chain using the notification callbacks but note that this
> +    // won't work in e10s.

Is it really worth doing complex fallback here? It should be easy for addons to create channels with loadinfo these days, and if they don't, it doesn't seem like a big deal that we give them a static default (i.e. always 3rd party, or never 3rd party)

::: netwerk/base/nsILoadInfo.idl
@@ +230,5 @@
> +   * We don't have enough information in the constructor to know if this is a
> +   * third party load request, so we are called back when we do have that
> +   * information and can compute the answer.
> +   */
> +  [noscript] void notifyLoadingURI(in nsIURI aURI);

Is there a reason to not make this scriptable for JS-implemented channels?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v3

This addresses Jonas' comments and appears to work in my testing. I still have to write an automated test for the websocket case.

The changes for redirects were tricky. I'm attaching them in a separate patch. Jonas tells me that there's a patch underway to have a LoadInfo per redirect and that'll make everything *so much better*, but until this lands, I had to do some crazy things to sync the state of "third-partyness" across the parent and the child as well as do the right thing if we failed the redirect (I haven't figured out how to test that case yet).

I'll request review once I have a green try run.

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

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make redirects do the right thing

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Oh, and the changes to the control flow in ThirdPartyUtils::IsThirdPartyChannel could use a thorough double-checking. The interaction between forcing, whether a URI was passed and whether the window is in a third-party context makes for some exciting reading.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixing tests

I had to fix a few more callers of WebSocket.initLoadInfo.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Changes for redirects

I had a couple of bugs in the last patch dealing with the failure case (in particular, if we failed, the child should only reset the third-partyness of its loadinfo and do nothing more; also, the parent should unconditionally set mOldIsThirdPartyRequest).

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8679676 [details] [diff] [review]
Changes for redirects

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

Bug 1194052 just landed, which should make your life and this patch a whole lot easier, right? No need to reset the thirdpartyness and such.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Hi Christoph,

Reading through the patch that landed, there's one thing that I don't understand: why do we pass the current loadinfo to create the redirect channel and clone it after we create the channel? The way things are structured, that means that my patch here will overwrite the result of "mIsThirdPartyRequest" on both the original loadinfo and its clone. It's hard to work around this since there isn't really a better place to call NotifyLoadingURI (I really want to compute this on all new channels). Wouldn't it make sense to clone the loadinfo before creating the new channel?

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rebased first patch

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated redirect patch

This implements what I suggest in comment 28. I left the nsBaseChannel as it was and just made the HTTP channels pre-clone the loadinfo.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

The try push in comment 32 was green but I don't understand why. I think the one from comment 33 is the right one!

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Base patch

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix redirects

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a test for cookies in frames with redirection

I'm a little surprised that there wasn't something like this already in the tree.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8683995 [details] [diff] [review]
Base patch

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

::: dom/base/ThirdPartyUtil.cpp
@@ +211,4 @@
>  
> +  if (!doForce) {
> +    nsCOMPtr<nsILoadInfo> loadInfo(aChannel->GetLoadInfo());
> +    if (MOZ_LIKELY(loadInfo)) {

nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
not sure if MOZ_LIKELY is really needed :-)

@@ +219,4 @@
>      }
> +  } else {
> +    parentIsThird = false;
> +  }

nit, you could initialize parentIsThird with false then you could safe that else branch, right?

::: dom/base/test/unit/test_thirdpartyutil.js
@@ +70,5 @@
>    // heirarchy. We leave that to mochitests.
>  
>    // Test isThirdPartyChannel. As above, we can't test the bits that require
> +  // a load context or window heirarchy. Because of that, the code assumes
> +  // that these are all third-party loads.

nit: s/heirarchy/hierarchy

::: netwerk/base/LoadInfo.cpp
@@ +256,5 @@
> +  }
> +
> +  // This is a subdocument load or a resource load.
> +  uint64_t windowID = 0;
> +  if (mInternalContentPolicyType == nsIContentPolicy::TYPE_SUBDOCUMENT) {

I think it doesn't really make a difference at this point because TYPE_DOCUMENT and TYPE_SUBDOCUMENT do not have any different internal types, but maybe they have in the furture. Potentially it's safer to call
nsContentUtils::InternalContentPolicyTypeToExternal(mInternalContentPolicyType)
before doing the comparison - not completely sure though - just a thought.

::: netwerk/base/nsIOService.cpp
@@ +411,5 @@
>      // are in a captive portal, so we trigger a recheck.
>      RecheckCaptivePortalIfLocalRedirect(newChan);
>  
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    newChan->GetLoadInfo(getter_AddRefs(loadInfo));

nit: simplify to:
nsCOMPtr<nsILoadInfo> loadInfo = newChan->GetLoadInfo();

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8683997 [details] [diff] [review]
Fix redirects

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

::: netwerk/base/LoadInfo.cpp
@@ +271,5 @@
> +    // We can get here for redirects, but we don't have enough information to do
> +    // anything about it. Wait for the child to let us know.
> +
> +    return NS_OK;
> +  }

As discussed over IRC, it's slightly unfortunate for this bug that we clone the loadInfo in SetupReplacementChannel which happens after we need to update those flags. However, SetupReplacementChannel is the only central point where all redirects go through. Alternatively we would have to update all the callsites of SetupReplacementChannel and make sure the loadInfo is already cloned. Unfrotunately, we also can't clone it in the ioservice, because we potentially also have to deal with inner channels. 

To sum it up, I suppose this tri-state seems like the best option to me as well.

@@ +282,5 @@
>  
>    // This is a subdocument load or a resource load.
>    uint64_t windowID = 0;
> +  if (mInternalContentPolicyType == nsIContentPolicy::TYPE_INTERNAL_FRAME ||
> +      mInternalContentPolicyType == nsIContentPolicy::TYPE_INTERNAL_IFRAME) {

Ok, I suppose that answers my question form the previous review.

::: netwerk/base/LoadInfo.h
@@ +109,5 @@
>    bool                             mInitialSecurityCheckDone;
> +  enum {
> +    SAME_ORIGIN,
> +    THIRD_PARTY,
> +    UNKNOWN

nit: can we rename that to UNKNOWN_THIRD_PARTY_STATE or something. UNKNOWN by itself seems rather confusing to me.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +731,5 @@
>         this, result));
> +
> +  // Update our new channel's loadinfo's mIsThirdParty with the new value.
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  mRedirectChannel->GetLoadInfo(getter_AddRefs(loadInfo));

nsCOMPtr<nsILoadInfo> loadInfo = mRedirectChannel->GetLoadInfo();

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8683998 [details] [diff] [review]
Add a test for cookies in frames with redirection

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

::: dom/tests/mochitest/bugs/test_bug1171215.html
@@ +17,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  /** Test for Bug 1022869 **/
> +  function startTest() {
> +    Math.sin(Math.random());

Why is that needed?

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #41)
> > +    Math.sin(Math.random());
> 
> Why is that needed?

That was leftover from debugging. I've removed it locally (and added public domain notices to all of the tests).

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8683998 [details] [diff] [review]
Add a test for cookies in frames with redirection

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

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #42)
> That was leftover from debugging. I've removed it locally (and added public
> domain notices to all of the tests).

Great, thanks!

Comment 44

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

Comment on attachment 8683995 [details] [diff] [review]
Base patch

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

What code handles redirects of websockets?

r- for now based on the comment in ThirdPartyUtil.cpp

::: dom/base/ThirdPartyUtil.cpp
@@ +230,5 @@
>    }
>  
>    // Obtain the URI from the channel, and its base domain.
>    nsCOMPtr<nsIURI> channelURI;
>    aChannel->GetURI(getter_AddRefs(channelURI));

I think we want to call NS_GetFinalChannelURI here. But see below.

@@ +239,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
> +  // Determine whether aURI is foreign with respect to channelURI.
> +  return IsThirdPartyInternal(channelDomain, aURI, aResult);

Does this work at all?

It's really hard to trace the cookie code calling  ThirdPartyUtil::IsThirdPartyChannel in mxr, there's just too much indirection. But I *think* that we originally come from here:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1922

Which means that the passed in URI is the URI of the channel. I.e. this call will return false any time we're loading a resource (say an image).

I think what you want to compare, is aURI and the URI of the loadingPrincipal.

If I'm right though, then I'm surprised that no tests would catch this?

::: dom/simplepush/PushService.jsm
@@ +829,5 @@
>                              Services.scriptSecurityManager.getSystemPrincipal(),
>                              null, // aTriggeringPrincipal
>                              Ci.nsILoadInfo.SEC_NORMAL,
> +                            Ci.nsIContentPolicy.TYPE_WEBSOCKET,
> +                            uri);

It's really unfortunate to have to do this. Can't the websocket code call loadinfo.notifyLoadingURI when the socket is opened?

::: netwerk/base/LoadInfo.cpp
@@ +256,5 @@
> +  }
> +
> +  // This is a subdocument load or a resource load.
> +  uint64_t windowID = 0;
> +  if (mInternalContentPolicyType == nsIContentPolicy::TYPE_SUBDOCUMENT) {

Actually, I think TYPE_SUBDOCUMENT is not used for internal types. We use TYPE_INTERNAL_FRAME and TYPE_INTERNAL_IFRAME.

I think calling nsContentUtils::InternalContentPolicyTypeToExternal is the safer thing to do here. That should generate more stable values in the face of future changes.

Both here and for the TYPE_DOCUMENT test above.

::: netwerk/base/nsIOService.cpp
@@ +419,5 @@
> +        nsCOMPtr<nsIURI> uri;
> +        newChan->GetURI(getter_AddRefs(uri));
> +
> +        loadInfo->NotifyLoadingURI(uri);
> +    }

Why is this needed? Won't the call in nsIOService::NewChannelFromURIWithProxyFlagsInternal take care of this?

Comment 45

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

Comment on attachment 8683997 [details] [diff] [review]
Fix redirects

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

::: netwerk/base/LoadInfo.cpp
@@ +257,5 @@
> +  // problems:
> +  //   * We call this function in the parent, where we can't access the window
> +  //     chain. In that case, the child either computes this and we're created
> +  //     with the right value or, for redirects, updates our state via
> +  //     ResetIsThirdPartyRequest.

I'm not sure it needs to be this complex.

If mIsThirdPartyRequest indicates that it's not third party when we first get into the parent, then it won't be thirdparty until we get a mismatch between the loadingPrincipal and the requested URI.

I.e. the window chain only matters for the initial value of mIsThirdPartyRequest. After that, any checks against the window chain will yield the same result.

No?

Comment 46

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

1. Get rid of the current redirectChain concept
2. Clone the loadinfo before calling NS_NewChannel
3. Keep your loadInfo.notifyUri function
4. Only call it in IOService.newChannel
5. Make loadInfo.notifyUri do something like

if (type == TYPE_DOCUMENT) {
  mIsThirdParty = false;
  return;
}
if (!mLoadingNode)
  mIsThirdParty = true;
  return;
}
mIsThirdParty = mIsThirdParty ||
                ThirdPartyUtils.CompareUrls(mLoadingPrincipal, uri) ||
                (mUriChain.IsEmpty() && mLoadingNode &&
                 ThirdPartyUtils.WindowHasThirdPartyAncestor(loadingNode.document.window)
mUriChain.AppendElement(uri);


6. In places where we look at the redirectChain, instead look at mUriChain.

mUriChain is effectively the same as the redirectChain. Only difference is that the urichain also include the current channels uri.


One tricky aspect is how we track internal redirects vs. non-internal ones. But that's solvable.

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

This is the entire patch rolled up. Jonas, Christoph, and I met up a couple of days ago to discuss the end result. The solution we came up with in order to make things simpler was to avoid doing the final "is the load third party to its loading context" in the child and keeping that updated and only store the "is the load in a context that's already third party" on the loadinfo (since that doesn't ever change, even with redirects).

Comment 49

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

Comment on attachment 8689859 [details] [diff] [review]
Patch v2

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

r=me with the below fix.

::: dom/base/ThirdPartyUtil.cpp
@@ +212,3 @@
>    // Obtain the URI from the channel, and its base domain.
>    nsCOMPtr<nsIURI> channelURI;
>    aChannel->GetURI(getter_AddRefs(channelURI));

I think you should change this to NS_GetFinalChannelURI. It shouldn't affect behavior of http requests, but it's the right thing to do.

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8689859 [details] [diff] [review]
Patch v2

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

Sorry that review took so long. Looks great!

::: dom/base/ThirdPartyUtil.cpp
@@ +186,5 @@
>    NS_ASSERTION(aResult, "null outparam pointer");
>  
>    nsresult rv;
>    bool doForce = false;
> +

Nit: is there a trailing whitespace left behind?

@@ +216,5 @@
>  
>    nsCString channelDomain;
>    rv = GetBaseDomain(channelURI, channelDomain);
>    if (NS_FAILED(rv))
>      return rv;

Just asking: would it make sense to assign a default-return-value to *aResult at the beginning of the function? Here we return an error but we do not assign a value to *aResult.

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #50)
> Just asking: would it make sense to assign a default-return-value to
> *aResult at the beginning of the function? Here we return an error but we do
> not assign a value to *aResult.

XPCOM rules say that if a function returns error, the value of any outparams can't be trusted (and might not be set). I'm going to leave this the way it is.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc44c9e55ef0e042c5a6c88bd241c08282b27f55
Bug 1171215 - Compute third-partyness in the loadinfo instead of nsIHttpChannelInternal so that other protocols correctly respect the third-party cookie pref. r=sicking/ckerschb

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc44c9e55ef0