574897 - Fix wrong cookie handling in WebSockets (need to confirm this is a bug with the new websockets code)

Status: VERIFIED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Wellington Fernando de Macedo]

WebSockets are setting/getting cookies from their owner's documents instead of their uris. This is very bad when used x-domains.

Comment 1

[Wellington Fernando de Macedo]

Attachment: v1

Biesi, could you review these changes, please?

Comment 2

[Wellington Fernando de Macedo]

Attachment: v2

I got this strange error on Windows in the Try-Server:

30281 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_ws_basic_tests.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - shouldCloseCleanly is not defined at http://mochi.test:8888/tests/content/base/test/test_websocket.html:553

I actually don't know why shouldCloseCleanly was undefined there (in test 21, inside the onclose handler, and only on Windows). Well, I found out that the previous patch made tests 21 and 22 being called twice. So I fixed that.

Anyway, I've pushed again this new patch into the Try-Server.

Comment 3

[Wellington Fernando de Macedo]

Comment on attachment 457301 [details] [diff] [review]
v2

sorry... wrong bug.

Comment 4

[Wellington Fernando de Macedo]

Biesi, can you review this patch, please?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Wellington, any chance to for a testcase here?

Comment 6

[Wellington Fernando de Macedo]

Yes, sure. I will write one as soon as possible.

Comment 7

[Wellington Fernando de Macedo]

Attachment: test

Olli, here it is the test you asked me for. This test requires multiple "Set-Cookie". Because of this it doesn't run with the trunk. So, you will need to import the patch available in this bug.

Comment 8

[Wellington Fernando de Macedo]

Well, I'll let you know some think I've noticed when writing this test.

The reason it requires multiple "Set-Cookie" is, from my first attempt, for instance:

Set-Cookie: cookie-1=v1; cookie-2; HttpOnly

I think it was supposed to set two cookies. The first one would be "cookie-1=v1" (available to the client page using document.cookie) and the second one "cookie-2" (not available to the client).

However, after handling that (by calling cookieService->SetCookieStringFromHttp) those both cookies aren't added to document.cookie. Also, I noted that some cookies that were in the document.cookie before vanish... It seems strange, doesn't it?

Comment 9

[Wellington Fernando de Macedo]

A small correction in my last comment:

Set-Cookie: cookie-1=v1; cookie-2=v2; HttpOnly
(When I writed the last comment I missed the "=v2" part. Anyway, that was there when I tested. I just missed to write here).

Comment 10

[Wellington Fernando de Macedo]

Guys, this bug needs some attention. I really wouldn't like to see FF4 be released with this bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So how does mOwner relate to the various documents floating around?

And is there a reason mOwner doesn't just have a URI we can use?   Hand-building URIs like that is very odd....

Comment 12

[Wellington Fernando de Macedo]

> So how does mOwner relate to the various documents floating around?
nsWebSocket::mOwner relates just to the document of the inner window that created the WebSocket object.

> And is there a reason mOwner doesn't just have a URI we can use?  
> Hand-building URIs like that is very odd....
Unfortunately the cookie service doesn't accept URIs with the "ws" scheme. But, yes, it could have that URI, too. However that uri would be used only twice (for getting and setting the cookies), when establishing the connection. I think it would be a waste unnecessary of resources.

Comment 13

[Wellington Fernando de Macedo]

I've just taken a look at the patch. I'm not sure if the document used to get the nsILoadContext in the nsWebSocketEstablishedConnection::GetInterface method is the right one. Please, whoever is going to review the patch take a look at there.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Unfortunately the cookie service doesn't accept URIs with the "ws" scheme. 

Accept in what sense?  And why shouldn't we fix that?

Comment 15

[[e-mail]]

Can you point me to a description (spec, perhaps) of what the 'ws' scheme is?

Comment 16

[Wellington Fernando de Macedo]

> Accept in what sense?  And why shouldn't we fix that?
In the sense that cookieService->(Set)GetCookieStringFromHttp(nsIURI("ws://..."), ...) fails. Well, I don't know if it has been fixed, but it was failing when I wrote the patch. I think fixing it a good idea.

> Can you point me to a description (spec, perhaps) of what the 'ws' scheme is?
http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol-76#page-45 and
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.h#161

Comment 17

[Wellington Fernando de Macedo]

Attachment: v2

Using mOwner->mURI is working now.

Who can review this patch?

Comment 18

[Wellington Fernando de Macedo]

Attachment: v2

Comment 19

[[e-mail]]

Can you ask bz to look at the usage of mOwner->mURI and the GetInterface changes at the end of the patch? I have no idea whether those are correct. I can review the rest (which arguably isn't much!).

The reason your 'Set-Cookie: cookie-1=v1; cookie-2; HttpOnly' string didn't work is that you need to comma-delimit separate cookies. And you should supply a name and value for each cookie. Perhaps you meant 'Set-Cookie: cookie-1=v1, cookie-2=v2; HttpOnly'?

Comment 20

[Wellington Fernando de Macedo]

> Perhaps you meant 'Set-Cookie: cookie-1=v1,cookie-2=v2; HttpOnly'?
I meant Set-Cookie: cookie-1=v1;cookie-2=v2; HttpOnly
Well, I don't know. Anyway, it is related to the CookieService, not to this patch itself.

> Can you ask bz to look at the usage of mOwner->mURI and the GetInterface
> changes at the end of the patch?
Yes, of course.

Comment 21

[[e-mail]]

(In reply to comment #20)
> > Perhaps you meant 'Set-Cookie: cookie-1=v1,cookie-2=v2; HttpOnly'?
> I meant Set-Cookie: cookie-1=v1;cookie-2=v2; HttpOnly

Put another way -- can you explain what you're expecting that to do?

Comment 22

[Wellington Fernando de Macedo]

When writing the test, I was trying to set the cookies using just one "Set-Cookie" response header, because the current version of WebSockets doesn't accept repeated headers.

> you need to comma-delimit separate cookies
Actually, I thought that cookies were semicolon-delimited. So, that was my problem when trying to set all cookies using just one header... Thanks :)

Comment 23

[Wellington Fernando de Macedo]

> Put another way -- can you explain what you're expecting that to do?
In other words, I tried to replace these two response headers:
Set-Cookie: cookie-1=v1
Set-Cookie: cookie-2=v2; HttpOnly

by this one:
Set-Cookie: cookie-1=v1;cookie-2=v2; HttpOnly

Comment 24

[[e-mail]]

Ah, OK. The string I suggested in comment 19 should do that. ;)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488755 [details] [diff] [review]
v2

>+        docChannel->
>+          GetNotificationCallbacks(getter_AddRefs(notificationCallbacks));

Wouldn't it be better to use NS_QueryNotificationCallbacks here?

Also, double-check with dwitte about null aFirstURI?

And I'd just pass in a null prompt instead of worrying about getting the right thing there....

Comment 26

[Wellington Fernando de Macedo]

> Also, double-check with dwitte about null aFirstURI?
Although there is that parameter the implementation of the nsICookieService interface doesn't use it. The Http implementation pass null too. See:
http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1426
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1258

> And I'd just pass in a null prompt instead of worrying about getting the right
> thing there....
Http implementation doesn't worry about. I think you are right. I will change it and let r=dwitte the new patch that I'll send in few hours.

Comment 27

[[e-mail]]

(In reply to comment #26)
> Although there is that parameter the implementation of the nsICookieService
> interface doesn't use it. The Http implementation pass null too.

Yeah, you can just pass null.

> Http implementation doesn't worry about. I think you are right. I will change
> it and let r=dwitte the new patch that I'll send in few hours.

Same here. You do need a channel, but you already have that. :)

One important question: who creates the websocket channel? Can you point me to the place (or places) where that happens? It's important that we have an owner that points to the docshell responsible for the load.

Comment 28

[[e-mail]]

A simple way you can test this, actually, is to disable third party cookies (Preferences/Privacy/Use custom settings/uncheck "accept third party cookies") and see if websocket cookies work.

Comment 29

[Wellington Fernando de Macedo]

> Same here. You do need a channel, but you already have that. :)
One question. When passing null will the nsWebSocketEstablishedConnection::GetInterface 's nsILoadContext stuff be called? Is that stuff still necessary?

> One important question: who creates the websocket channel? Can you point me to
> the place (or places) where that happens? It's important that we have an owner
> that points to the docshell responsible for the load.
nsWebSocketEstablishedConnection is the class that implements the nsIChannel interface. It is created here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#2986

As you can see, many nsIChannel's methods aren't implemented:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#2396

Comment 30

[[e-mail]]

(In reply to comment #29)
> One question. When passing null will the
> nsWebSocketEstablishedConnection::GetInterface 's nsILoadContext stuff be
> called? Is that stuff still necessary?

Yes. See http://mxr.mozilla.org/mozilla-central/source/content/base/src/ThirdPartyUtil.cpp#274.

(It will be called regardless of whether you pass a null aFirstURI. That parameter is totally unused within the cookie service.)

> As you can see, many nsIChannel's methods aren't implemented:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsWebSocket.cpp#2396

As long as you have the nsILoadContext GetInterface() stuff and nsIChannel::GetURI is implemented, you should be OK.

It looks like you're using mOwner->mScriptContext to get the loadgroup. As long as bz thinks that's correct, and you've tested per comment 28, then we're fine.

+    nsCOMPtr<nsIDocument> doc =
+      nsContentUtils::GetDocumentFromScriptContext(mOwner->mScriptContext);

Comment 31

[Wellington Fernando de Macedo]

I have a last question. When thirdparty cookies are disabled websockets will fail to establish the connection because SetCookieStringFromHttp fails. However, I think that the connection could be established, even if the cookies were not set. What do you think about?

Comment 32

[[e-mail]]

SetCookieStringFromHttp will always return NS_OK; the cookies just won't get set if it's third party. Is that what you're asking?

Comment 33

[Wellington Fernando de Macedo]

> SetCookieStringFromHttp will always return NS_OK;
Hmm, actually.

In my test I have websocket from a foreign domain url that set cookies. This websocket is establishing the connection, but suddenly it is closing it. I thought it was because SetCookieStringFromHttp. But actually it is not. It is strange, I will try to realize what is happening...

Comment 34

[Wellington Fernando de Macedo]

Attachment: v3

This patch addresses the previous comments. 

ps: dwitte, the error I was getting was because a bug in my testcase, not in the websocket implementation.

Comment 35

[Boris Zbarsky [:bzbarsky]]

The mOwner->mScriptContext thing should be ok, I think.  It uses the loadgroup of the window the websocket was created from.

Comment 36

[[e-mail]]

Comment on attachment 492002 [details] [diff] [review]
v3

Are there tests for this? If there exist any tests for websockets, you should be able to just put a Set-Cookie header in one of them (using file^headers^, see e.g. extensions/cookie/test) and check that it succeeded. You should set 'network.cookie.cookieBehavior' to 1 (reject third-party) before doing so.

Comment 37

[Wellington Fernando de Macedo]

Attachment: tests-v2

dwitte, this patch has tests using both cookieBehavior equal to 0 and 1.

Comment 38

[[e-mail]]

Comment on attachment 492002 [details] [diff] [review]
v3

Nice. r=dwitte

Comment 39

[[e-mail]]

Comment on attachment 492549 [details] [diff] [review]
tests-v2

r=dwitte

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

I assume this just need to be checked in, right?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Or, should the tests be updated so that we enable websockets when needed.
See other websocket tests.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

I'll fix the test.

Comment 43

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

No longer a blocker since we turned off websockets

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Attachment: tests fixed

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This must be fixed in Firefox 6 Aurora or WebSockets must be disabled. Patrick says that this should already be fixed and this just needs to be retested.

Comment 46

[Christopher Blizzard (:blizzard)]

Over to pat to confirm/deny.

Comment 47

[Patrick McManus [:mcmanus]]

This is what I tested, feel free to let me know if it doesn't satisfy the bug.

There is a cookie set for domain A, but not for domain B.

Scenario 1 - 

[1] Open a URL on domain A to get some HTML and JS. The JS creates a websocket[2] to ws://domain-A.

HTTP transaction [1] and [2] contain the cookie for domain A, as expected.

Scenario 2 - 

[1] Open a URL on domain A to get some HTML and JS. The JS creates a websocket[2] to ws://domain-B.

HTTP transaction [1] contains the domain A cookie, HTTP transaction 2 does not contain a cookie at all (as none is set for domain B).

Comment 48

[Asa Dotzler [:asa]]

Brian, Patrick, we're getting deep into Firefox 6 Beta and it'd be nice to know if we're happy here.

Comment 49

[Patrick McManus [:mcmanus]]

I marked this as fixed based on comment 47 as requested by Chris in Comment 46. I have no concerns. Does Brian need to confirm the criteria I laid out in 47 satisfies the base concern of the bug?

Comment 50

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The behavior that Patrick described in comment 47 is the correct behavior, and Patrick already confirmed that is the current behavior, so this bug is resolved.

Comment 51

[Christopher Blizzard (:blizzard)]

According to pat the code here is the same for 6,7,8 so should be fixed in all places.