Closed Bug 574897 Opened 13 years ago Closed 12 years ago
Fix wrong cookie handling in Web
Sockets (need to confirm this is a bug with the new websockets code)
WebSockets are setting/getting cookies from their owner's documents instead of their uris. This is very bad when used x-domains.
Biesi, could you review these changes, please?
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
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 on attachment 457301 [details] [diff] [review] v2 sorry... wrong bug.
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.
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).
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
Using mOwner->mURI is working now. Who can review this patch?
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.
(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...
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.
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.
dwitte, this patch has tests using both cookieBehavior equal to 0 and 1.
Assignee: wfernandom2004 → nobody
Component: DOM → Networking: WebSockets
QA Contact: general → networking.websockets
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?
Or, should the tests be updated so that we enable websockets when needed. See other websocket tests.
I'll fix the test.
No longer a blocker since we turned off websockets
blocking2.0: betaN+ → -
blocking2.0: - → .x
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.
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 -  Open a URL on domain A to get some HTML and JS. The JS creates a websocket to ws://domain-A. HTTP transaction  and  contain the cookie for domain A, as expected. Scenario 2 -  Open a URL on domain A to get some HTML and JS. The JS creates a websocket to ws://domain-B. HTTP transaction  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: 12 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.
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.