Last Comment Bug 574897 - Fix wrong cookie handling in WebSockets (need to confirm this is a bug with the new websockets code)
: Fix wrong cookie handling in WebSockets (need to confirm this is a bug with t...
Status: VERIFIED FIXED
[verification needed or WebSockets ge...
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 640003
Blocks: websocket
  Show dependency treegraph
 
Reported: 2010-06-25 19:32 PDT by Wellington Fernando de Macedo
Modified: 2013-12-27 14:21 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
.x+


Attachments
v1 (8.87 KB, patch)
2010-06-25 19:35 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
v2 (18.55 KB, patch)
2010-07-14 07:26 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
test (10.89 KB, patch)
2010-10-02 18:43 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
v2 (8.10 KB, patch)
2010-11-07 08:23 PST, Wellington Fernando de Macedo
no flags Details | Diff | Review
v2 (8.32 KB, patch)
2010-11-07 08:34 PST, Wellington Fernando de Macedo
bzbarsky: review+
Details | Diff | Review
v3 (7.45 KB, patch)
2010-11-19 16:54 PST, Wellington Fernando de Macedo
dwitte: review+
Details | Diff | Review
tests-v2 (18.38 KB, patch)
2010-11-22 18:48 PST, Wellington Fernando de Macedo
dwitte: review+
Details | Diff | Review
tests fixed (19.07 KB, patch)
2010-12-14 11:24 PST, Olli Pettay [:smaug]
no flags Details | Diff | Review

Description Wellington Fernando de Macedo 2010-06-25 19:32:35 PDT
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 2010-06-25 19:35:32 PDT
Created attachment 454211 [details] [diff] [review]
v1

Biesi, could you review these changes, please?
Comment 2 Wellington Fernando de Macedo 2010-07-14 07:26:31 PDT
Created attachment 457301 [details] [diff] [review]
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 2010-07-14 07:29:06 PDT
Comment on attachment 457301 [details] [diff] [review]
v2

sorry... wrong bug.
Comment 4 Wellington Fernando de Macedo 2010-08-17 17:37:08 PDT
Biesi, can you review this patch, please?
Comment 5 Olli Pettay [:smaug] 2010-09-26 08:06:18 PDT
Wellington, any chance to for a testcase here?
Comment 6 Wellington Fernando de Macedo 2010-09-26 18:46:10 PDT
Yes, sure. I will write one as soon as possible.
Comment 7 Wellington Fernando de Macedo 2010-10-02 18:43:15 PDT
Created attachment 480465 [details] [diff] [review]
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 2010-10-02 19:04:25 PDT
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 2010-10-03 10:00:41 PDT
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 2010-11-01 16:00:50 PDT
Guys, this bug needs some attention. I really wouldn't like to see FF4 be released with this bug.
Comment 11 Boris Zbarsky [:bz] 2010-11-01 23:07:23 PDT
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 2010-11-02 11:54:55 PDT
> 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 2010-11-02 12:03:20 PDT
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 [:bz] 2010-11-02 12:17:26 PDT
> 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 dwitte@gmail.com 2010-11-02 12:43:32 PDT
Can you point me to a description (spec, perhaps) of what the 'ws' scheme is?
Comment 16 Wellington Fernando de Macedo 2010-11-02 13:06:10 PDT
> 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 2010-11-07 08:23:03 PST
Created attachment 488752 [details] [diff] [review]
v2

Using mOwner->mURI is working now.

Who can review this patch?
Comment 18 Wellington Fernando de Macedo 2010-11-07 08:34:58 PST
Created attachment 488755 [details] [diff] [review]
v2
Comment 19 dwitte@gmail.com 2010-11-08 15:10:34 PST
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 2010-11-08 15:21:13 PST
> 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 dwitte@gmail.com 2010-11-08 15:23:21 PST
(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 2010-11-08 15:29:37 PST
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 2010-11-08 15:35:46 PST
> 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 dwitte@gmail.com 2010-11-08 15:39:28 PST
Ah, OK. The string I suggested in comment 19 should do that. ;)
Comment 25 Boris Zbarsky [:bz] 2010-11-18 09:25:00 PST
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 2010-11-19 11:56:45 PST
> 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 dwitte@gmail.com 2010-11-19 12:19:50 PST
(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 dwitte@gmail.com 2010-11-19 12:20:38 PST
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 2010-11-19 12:37:14 PST
> 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 dwitte@gmail.com 2010-11-19 12:49:36 PST
(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 2010-11-19 15:18:29 PST
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 dwitte@gmail.com 2010-11-19 15:36:06 PST
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 2010-11-19 15:57:45 PST
> 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 2010-11-19 16:54:31 PST
Created attachment 492002 [details] [diff] [review]
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 [:bz] 2010-11-19 19:21:04 PST
The mOwner->mScriptContext thing should be ok, I think.  It uses the loadgroup of the window the websocket was created from.
Comment 36 dwitte@gmail.com 2010-11-22 13:59:52 PST
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 2010-11-22 18:48:13 PST
Created attachment 492549 [details] [diff] [review]
tests-v2

dwitte, this patch has tests using both cookieBehavior equal to 0 and 1.
Comment 38 dwitte@gmail.com 2010-12-10 10:56:36 PST
Comment on attachment 492002 [details] [diff] [review]
v3

Nice. r=dwitte
Comment 39 dwitte@gmail.com 2010-12-10 11:01:43 PST
Comment on attachment 492549 [details] [diff] [review]
tests-v2

r=dwitte
Comment 40 Olli Pettay [:smaug] 2010-12-13 17:10:40 PST
I assume this just need to be checked in, right?
Comment 41 Olli Pettay [:smaug] 2010-12-13 18:05:54 PST
Or, should the tests be updated so that we enable websockets when needed.
See other websocket tests.
Comment 42 Olli Pettay [:smaug] 2010-12-14 09:07:53 PST
I'll fix the test.
Comment 43 Jonas Sicking (:sicking) 2010-12-14 10:09:20 PST
No longer a blocker since we turned off websockets
Comment 44 Olli Pettay [:smaug] 2010-12-14 11:24:58 PST
Created attachment 497554 [details] [diff] [review]
tests fixed
Comment 45 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-06-14 16:29:23 PDT
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) 2011-06-15 11:39:33 PDT
Over to pat to confirm/deny.
Comment 47 Patrick McManus [:mcmanus] 2011-06-15 12:34:13 PDT
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] 2011-07-17 22:02:20 PDT
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] 2011-07-18 05:32:54 PDT
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?) 2011-07-18 09:46:04 PDT
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) 2011-08-01 14:52:41 PDT
According to pat the code here is the same for 6,7,8 so should be fixed in all places.

Note You need to log in before you can comment on or make changes to this bug.