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

VERIFIED FIXED

Status

()

Core
Networking: WebSockets
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Wellington Fernando de Macedo, 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)

(Reporter)

Description

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

Updated

7 years ago
Blocks: 472529
(Reporter)

Comment 1

7 years ago
Created attachment 454211 [details] [diff] [review]
v1

Biesi, could you review these changes, please?
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
Attachment #454211 - Flags: review?(cbiesinger)

Updated

7 years ago
blocking2.0: --- → ?
(Reporter)

Comment 2

7 years ago
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.
Attachment #454211 - Attachment is obsolete: true
Attachment #457301 - Flags: review?(Olli.Pettay)
Attachment #454211 - Flags: review?(cbiesinger)
(Reporter)

Comment 3

7 years ago
Comment on attachment 457301 [details] [diff] [review]
v2

sorry... wrong bug.
Attachment #457301 - Attachment is obsolete: true
Attachment #457301 - Flags: review?(Olli.Pettay)
(Reporter)

Updated

7 years ago
Attachment #454211 - Attachment is obsolete: false
Attachment #454211 - Flags: review?(cbiesinger)

Updated

7 years ago
blocking2.0: ? → betaN+
(Reporter)

Comment 4

7 years ago
Biesi, can you review this patch, please?

Comment 5

7 years ago
Wellington, any chance to for a testcase here?
(Reporter)

Comment 6

7 years ago
Yes, sure. I will write one as soon as possible.
(Reporter)

Comment 7

7 years ago
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.
(Reporter)

Comment 8

7 years ago
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?
(Reporter)

Comment 9

7 years ago
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).
(Reporter)

Updated

7 years ago
Attachment #480465 - Flags: feedback?(Olli.Pettay)
(Reporter)

Comment 10

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

Updated

7 years ago
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....
(Reporter)

Comment 12

7 years ago
> 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.
(Reporter)

Comment 13

7 years ago
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?

Comment 15

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

Comment 16

7 years ago
> 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
(Reporter)

Comment 17

7 years ago
Created attachment 488752 [details] [diff] [review]
v2

Using mOwner->mURI is working now.

Who can review this patch?
Attachment #454211 - Attachment is obsolete: true
Attachment #454211 - Flags: review?(cbiesinger)
(Reporter)

Comment 18

7 years ago
Created attachment 488755 [details] [diff] [review]
v2
Attachment #488752 - Attachment is obsolete: true

Comment 19

7 years ago
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'?
(Reporter)

Comment 20

7 years ago
> 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.
(Reporter)

Updated

7 years ago
Attachment #488755 - Flags: review?(dwitte)
Attachment #488755 - Flags: review?(bzbarsky)

Comment 21

7 years ago
(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?
(Reporter)

Comment 22

7 years ago
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 :)
(Reporter)

Comment 23

7 years ago
> 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

7 years ago
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+
(Reporter)

Comment 26

7 years ago
> 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

7 years ago
(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

7 years ago
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.
(Reporter)

Comment 29

7 years ago
> 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

7 years ago
(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);
(Reporter)

Comment 31

7 years ago
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

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

Comment 33

7 years ago
> 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...
(Reporter)

Comment 34

7 years ago
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.
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 36

7 years ago
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.
(Reporter)

Comment 37

7 years ago
Created attachment 492549 [details] [diff] [review]
tests-v2

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)

Updated

7 years ago
Blocks: 537787
Assignee: wfernandom2004 → nobody
Component: DOM → Networking: WebSockets
QA Contact: general → networking.websockets

Updated

7 years ago
Assignee: nobody → wfernandom2004

Comment 38

7 years ago
Comment on attachment 492002 [details] [diff] [review]
v3

Nice. r=dwitte
Attachment #492002 - Flags: review?(dwitte) → review+

Comment 39

7 years ago
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+ → -
Created attachment 497554 [details] [diff] [review]
tests fixed
blocking2.0: - → .x
Depends on: 640003

Updated

6 years ago
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.
status-firefox6: --- → affected
tracking-firefox6: --- → ?
(Reporter)

Updated

6 years ago
Assignee: wfernandom2004 → nobody
tracking-firefox6: ? → +
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 48

6 years ago
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.
status-firefox6: affected → fixed
You need to log in before you can comment on or make changes to this bug.