Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(360)

Issue 336263005: Map WebSocket URL schemes to HTTP URL schemes for auth purposes. (Closed)

Created:
6 years, 6 months ago by Adam Rice
Modified:
6 years, 4 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Map WebSocket URL schemes to HTTP URL schemes for auth purposes. This permits WebSocket connections to inherit credentials from HTTP pages, and matches the behaviour of other browsers. Design doc: https://docs.google.com/a/chromium.org/document/d/129rLtf5x3hvhP5rayLiSxnEjOXS8Z7EnLJgBL4CdwjI/edit Also consider any 401 or 407 results that reach the WebSocketStream URLRequest::Delegate to be unrecoverable errors. Also ensure that the response headers are reported back to the renderer when the developer tools are open and a 401 error happens. BUG=123862 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286108

Patch Set 1 #

Patch Set 2 : Test preflighting auth via HTTP, and also digest auth. #

Patch Set 3 : Remove unused file. #

Total comments: 2

Patch Set 4 : Factor out WebSocketDispatchOnFinishOpeningHandshake() into a separate function. #

Total comments: 7

Patch Set 5 : Style fixes. #

Total comments: 2

Patch Set 6 : Add missing DISALLOW_COPY_AND_ASSIGN(AutoLogin) #

Patch Set 7 : Rebase. #

Patch Set 8 : Replace accidentally removed #include <string> #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+517 lines, -64 lines) Patch
M chrome/browser/net/websocket_browsertest.cc View 1 2 3 4 5 6 7 3 chunks +186 lines, -23 lines 0 comments Download
M net/data/websocket/README View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
A + net/data/websocket/connect_to.html View 1 2 3 4 5 6 1 chunk +10 lines, -2 lines 0 comments Download
M net/http/http_network_transaction.cc View 1 2 3 4 5 6 2 chunks +12 lines, -0 lines 3 comments Download
M net/test/spawned_test_server/base_test_server.h View 1 2 3 2 chunks +9 lines, -0 lines 0 comments Download
M net/test/spawned_test_server/base_test_server.cc View 1 2 3 4 5 6 3 chunks +9 lines, -2 lines 0 comments Download
M net/tools/testserver/testserver.py View 1 2 3 4 5 6 3 chunks +6 lines, -0 lines 0 comments Download
M net/websockets/websocket_basic_handshake_stream.cc View 1 2 3 1 chunk +4 lines, -13 lines 0 comments Download
M net/websockets/websocket_stream.h View 1 2 3 2 chunks +12 lines, -0 lines 0 comments Download
M net/websockets/websocket_stream.cc View 1 2 3 4 5 6 4 chunks +38 lines, -1 line 0 comments Download
M net/websockets/websocket_stream_test.cc View 1 2 3 4 5 6 12 chunks +226 lines, -23 lines 0 comments Download

Messages

Total messages: 26 (0 generated)
Adam Rice
6 years, 5 months ago (2014-07-03 15:07:29 UTC) #1
tyoshino (SeeGerritForStatus)
lgtm https://codereview.chromium.org/336263005/diff/40001/net/websockets/websocket_stream.cc File net/websockets/websocket_stream.cc (right): https://codereview.chromium.org/336263005/diff/40001/net/websockets/websocket_stream.cc#newcode146 net/websockets/websocket_stream.cc:146: void OnFinishOpeningHandshake() { this looks similar to WebSocketBasicHandshakeStream::OnFinishOpeningHandshake
6 years, 5 months ago (2014-07-07 19:26:44 UTC) #2
Adam Rice
+jgraettinger for net OWNERS. https://codereview.chromium.org/336263005/diff/40001/net/websockets/websocket_stream.cc File net/websockets/websocket_stream.cc (right): https://codereview.chromium.org/336263005/diff/40001/net/websockets/websocket_stream.cc#newcode146 net/websockets/websocket_stream.cc:146: void OnFinishOpeningHandshake() { On 2014/07/07 ...
6 years, 5 months ago (2014-07-08 10:23:45 UTC) #3
Johnny
lgtm % comments https://codereview.chromium.org/336263005/diff/50001/net/tools/testserver/testserver.py File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/336263005/diff/50001/net/tools/testserver/testserver.py#newcode2210 net/tools/testserver/testserver.py:2210: # TODO(ricea): Generalise this to support ...
6 years, 5 months ago (2014-07-08 20:27:40 UTC) #4
Adam Rice
https://codereview.chromium.org/336263005/diff/50001/net/tools/testserver/testserver.py File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/336263005/diff/50001/net/tools/testserver/testserver.py#newcode2210 net/tools/testserver/testserver.py:2210: # TODO(ricea): Generalise this to support basic auth for ...
6 years, 5 months ago (2014-07-09 06:35:02 UTC) #5
Adam Rice
+mmenke for chrome/browser/net
6 years, 5 months ago (2014-07-09 09:26:31 UTC) #6
mmenke
LGTM rubberstamp, deferring to Johnny on this one. https://codereview.chromium.org/336263005/diff/70001/chrome/browser/net/websocket_browsertest.cc File chrome/browser/net/websocket_browsertest.cc (right): https://codereview.chromium.org/336263005/diff/70001/chrome/browser/net/websocket_browsertest.cc#newcode129 chrome/browser/net/websocket_browsertest.cc:129: content::NotificationRegistrar ...
6 years, 5 months ago (2014-07-09 14:19:00 UTC) #7
Johnny
PS5 lgtm % mmenke's nit. https://codereview.chromium.org/336263005/diff/50001/net/websockets/websocket_stream_test.cc File net/websockets/websocket_stream_test.cc (right): https://codereview.chromium.org/336263005/diff/50001/net/websockets/websocket_stream_test.cc#newcode1155 net/websockets/websocket_stream_test.cc:1155: socket_data.Pass()); On 2014/07/09 06:35:01, ...
6 years, 5 months ago (2014-07-09 16:16:17 UTC) #8
Adam Rice
https://codereview.chromium.org/336263005/diff/70001/chrome/browser/net/websocket_browsertest.cc File chrome/browser/net/websocket_browsertest.cc (right): https://codereview.chromium.org/336263005/diff/70001/chrome/browser/net/websocket_browsertest.cc#newcode129 chrome/browser/net/websocket_browsertest.cc:129: content::NotificationRegistrar registrar_; On 2014/07/09 14:19:00, mmenke wrote: > nit: ...
6 years, 5 months ago (2014-07-10 05:30:28 UTC) #9
Adam Rice
The CQ bit was checked by ricea@chromium.org
6 years, 5 months ago (2014-07-10 05:31:10 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/336263005/90001
6 years, 5 months ago (2014-07-10 05:32:15 UTC) #11
commit-bot: I haz the power
Change committed as 282307
6 years, 5 months ago (2014-07-10 11:14:07 UTC) #12
Adam Rice
Re-opening to land again now that the tests have been de-flaked.
6 years, 4 months ago (2014-07-28 04:57:17 UTC) #13
Adam Rice
The CQ bit was checked by ricea@chromium.org
6 years, 4 months ago (2014-07-28 04:58:08 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/336263005/90001
6 years, 4 months ago (2014-07-28 04:59:14 UTC) #15
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux ...
6 years, 4 months ago (2014-07-28 09:06:42 UTC) #16
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-07-28 09:20:51 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_clang_dbg/builds/306)
6 years, 4 months ago (2014-07-28 09:20:53 UTC) #18
Adam Rice
The CQ bit was checked by ricea@chromium.org
6 years, 4 months ago (2014-07-29 03:35:46 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/336263005/130001
6 years, 4 months ago (2014-07-29 03:36:44 UTC) #20
commit-bot: I haz the power
Change committed as 286108
6 years, 4 months ago (2014-07-29 07:42:33 UTC) #21
Ryan Sleevi
https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc#newcode1559 net/http/http_network_transaction.cc:1559: } This seems like a violation of SOP. ws://foo.bar ...
6 years, 4 months ago (2014-07-29 19:54:32 UTC) #22
tyoshino (SeeGerritForStatus)
https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc#newcode1559 net/http/http_network_transaction.cc:1559: } On 2014/07/29 19:54:32, Ryan Sleevi wrote: > This ...
6 years, 4 months ago (2014-07-30 03:39:54 UTC) #23
Adam Rice
https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc#newcode1559 net/http/http_network_transaction.cc:1559: } On 2014/07/29 19:54:32, Ryan Sleevi wrote: > This ...
6 years, 4 months ago (2014-07-30 03:43:34 UTC) #24
tyoshino (SeeGerritForStatus)
On 2014/07/30 03:39:54, tyoshino wrote: > https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc > File net/http/http_network_transaction.cc (right): > > https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_transaction.cc#newcode1559 > ...
6 years, 4 months ago (2014-07-30 03:44:30 UTC) #25
tyoshino (SeeGerritForStatus)
6 years, 4 months ago (2014-07-30 03:46:00 UTC) #26
Message was sent while issue was closed.
On 2014/07/30 03:43:34, Adam Rice wrote:
>
https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_t...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/336263005/diff/130001/net/http/http_network_t...
> net/http/http_network_transaction.cc:1559: }
> On 2014/07/29 19:54:32, Ryan Sleevi wrote:
> > This seems like a violation of SOP.
> > 
> > ws://foo.bar != SOP as http://foo.bar
> > 
> > Is sharing credentials really correct here? I can't help but feel like, no,
> it's
> > not and shouldn't be.
> 
> The issue is ws: both is and isn't the same protocol as http:. The same thing
> applies for cookies; quoting the WhatWG WebSocket API 'The headers to send
> appropriate cookies must be a Cookie header whose value is the cookie-string
> computed from the user's cookie store and the URL url; for these purposes this
> is not a "non-HTTP" API.'
> 
> The cookie behaviour has been long standardised and works without any
> WebSocket-specific hacks because Chrome's cookie store doesn't really know
about
> URL schemes.
> 

Fair enough.

> Authentication behaviour is not really standardised anywhere; RFC6455 mentions
> it in passing, and the WhatWG standard effectively disallows it by banning
> handling of status codes other than 101.
> 
> However, not implementing authentication leaves us incompatible with Firefox
and
> IE and means that a number of Intranet applications don't work with Chrome,
> blocking Chrome adoption. Since we never popup UI for a WebSocket connection,
> inheriting credentials from HTTP is the only reasonable implementation and is
> also what Firefox and IE do.
> 
> My way of looking at it is that ws://foo.bar is "mostly" SOP as
http://foo.bar.
> They share cookies, credentials, client certificates, SSL exceptions, etc.
> 
> Another view some people take is that ws: is not a "real" URL scheme at all.

Powered by Google App Engine
This is Rietveld 408576698