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

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: wfernandom2004, Assigned: mcmanus)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6+ fixed, blocking2.0 .x+)

Details

(Whiteboard: [verification needed or WebSockets gets the kibosh])

Attachments

(3 attachments, 5 obsolete attachments)

WebSockets are setting/getting cookies from their owner's documents instead of their uris. This is very bad when used x-domains.
Blocks: websocket
Posted patch v1 (obsolete) — Splinter Review
Biesi, could you review these changes, please?
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
Attachment #454211 - Flags: review?(cbiesinger)
blocking2.0: --- → ?
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #454211 - Attachment is obsolete: true
Attachment #457301 - Flags: review?(Olli.Pettay)
Attachment #454211 - Flags: review?(cbiesinger)
Comment on attachment 457301 [details] [diff] [review]
v2

sorry... wrong bug.
Attachment #457301 - Attachment is obsolete: true
Attachment #457301 - Flags: review?(Olli.Pettay)
Attachment #454211 - Attachment is obsolete: false
Attachment #454211 - Flags: review?(cbiesinger)
blocking2.0: ? → betaN+
Biesi, can you review this patch, please?
Wellington, any chance to for a testcase here?
Yes, sure. I will write one as soon as possible.
Posted patch test (obsolete) — Splinter Review
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.
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?
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).
Attachment #480465 - Flags: feedback?(Olli.Pettay)
Guys, this bug needs some attention. I really wouldn't like to see FF4 be released with this bug.
Summary: Fix wrong cookie handling in WS → Fix wrong cookie handling in WebSockets
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....
> 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.
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.
> Unfortunately the cookie service doesn't accept URIs with the "ws" scheme. 

Accept in what sense?  And why shouldn't we fix that?
Can you point me to a description (spec, perhaps) of what the 'ws' scheme is?
> 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
Posted patch v2 (obsolete) — Splinter Review
Using mOwner->mURI is working now.

Who can review this patch?
Attachment #454211 - Attachment is obsolete: true
Attachment #454211 - Flags: review?(cbiesinger)
Posted patch v2 (obsolete) — Splinter Review
Attachment #488752 - Attachment is obsolete: true
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'?
> 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.
Attachment #488755 - Flags: review?(dwitte)
Attachment #488755 - Flags: review?(bzbarsky)
(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?
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 :)
> 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
Ah, OK. The string I suggested in comment 19 should do that. ;)
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....
Attachment #488755 - Flags: review?(bzbarsky) → review+
> 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.
(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.
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.
> 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
(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);
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?
SetCookieStringFromHttp will always return NS_OK; the cookies just won't get set if it's third party. Is that what you're asking?
> 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...
Posted patch v3Splinter Review
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.
Attachment #488755 - Attachment is obsolete: true
Attachment #492002 - Flags: review?(dwitte)
Attachment #488755 - Flags: review?(dwitte)
The mOwner->mScriptContext thing should be ok, I think.  It uses the loadgroup of the window the websocket was created from.
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.
Posted patch tests-v2Splinter Review
dwitte, this patch has tests using both cookieBehavior equal to 0 and 1.
Attachment #480465 - Attachment is obsolete: true
Attachment #492549 - Flags: review?(dwitte)
Attachment #480465 - Flags: feedback?(Olli.Pettay)
Blocks: 537787
Assignee: wfernandom2004 → nobody
Component: DOM → Networking: WebSockets
QA Contact: general → networking.websockets
Assignee: nobody → wfernandom2004
Comment on attachment 492002 [details] [diff] [review]
v3

Nice. r=dwitte
Attachment #492002 - Flags: review?(dwitte) → review+
Comment on attachment 492549 [details] [diff] [review]
tests-v2

r=dwitte
Attachment #492549 - Flags: review?(dwitte) → review+
I assume this just need to be checked in, right?
Keywords: checkin-needed
Or, should the tests be updated so that we enable websockets when needed.
See other websocket tests.
Keywords: checkin-needed
I'll fix the test.
No longer a blocker since we turned off websockets
blocking2.0: betaN+ → -
Depends on: 640003
No longer blocks: 537787
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.
Assignee: wfernandom2004 → nobody
Summary: Fix wrong cookie handling in WebSockets → Fix wrong cookie handling in WebSockets (need to confirm this is a bug with the new websockets code)
Over to pat to confirm/deny.
Assignee: nobody → 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).
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Brian, Patrick, we're getting deep into Firefox 6 Beta and it'd be nice to know if we're happy here.
Whiteboard: [verification needed or WebSockets gets the kibosh]
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?
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.
Status: RESOLVED → VERIFIED
According to pat the code here is the same for 6,7,8 so should be fixed in all places.
You need to log in before you can comment on or make changes to this bug.