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

Issue 601077: Support HttpOnly cookie on Web Socket (Closed)

Created:
10 years, 10 months ago by ukai
Modified:
9 years, 7 months ago
CC:
chromium-reviews, brettw+cc_chromium.org, darin+cc_chromium.org, dpranke+watch_chromium.org, ben+cc_chromium.org, pam+watch_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org, jam, jam+cc_chromium.org
Visibility:
Public.

Description

Support HttpOnly cookie on Web Socket Web Socket should send "HttpOnly" cookie when handshaking. In WebKit/WebCore, WebSocketHandshake uses cookieRequestHeaderFieldValue() to get cookies including HttpOnly cookie. However, Chrome doesn't trunk renderer process, so we're not allowed to access HttpOnly cookie in WebCore. Thus, we handle HttpOnly cookies in browser process. Add SocketStreamJob as interface for protocol specific handling on SocketStream. WebSocketJob implements Web Socket specific handling. For now, it handles cookies in Web Socket. It checks Web Socket handshake request message from renderer process, and replaces Cookie: header to include HttpOnly cookies. It also checks Web Socket handshake response message, sets cookies if any, and strips Set-Cookie: header, so that renderer process couldn't see Set-Cookie: header. BUG=35660 TEST=net_unittests and layout_tests passes Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=40250

Patch Set 1 #

Total comments: 6

Patch Set 2 : Use SCOPED_TRACE #

Total comments: 12

Patch Set 3 : Use CookiePolicy #

Total comments: 4

Patch Set 4 : fix darin's comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1217 lines, -22 lines) Patch
M chrome/browser/renderer_host/socket_stream_dispatcher_host.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/socket_stream_host.h View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/renderer_host/socket_stream_host.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M net/net.gyp View 1 2 3 3 chunks +7 lines, -0 lines 0 comments Download
M net/socket_stream/socket_stream.h View 6 chunks +12 lines, -9 lines 0 comments Download
M net/socket_stream/socket_stream.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
A net/socket_stream/socket_stream_job.h View 1 2 3 1 chunk +87 lines, -0 lines 0 comments Download
A net/socket_stream/socket_stream_job.cc View 1 chunk +27 lines, -0 lines 0 comments Download
A net/socket_stream/socket_stream_job_manager.h View 1 chunk +40 lines, -0 lines 0 comments Download
A net/socket_stream/socket_stream_job_manager.cc View 1 chunk +59 lines, -0 lines 0 comments Download
A net/websockets/websocket_job.h View 1 2 1 chunk +98 lines, -0 lines 0 comments Download
A net/websockets/websocket_job.cc View 1 2 3 1 chunk +378 lines, -0 lines 0 comments Download
net/websockets/websocket_job_unittest.cc View 1 2 1 chunk +495 lines, -0 lines 0 comments Download
M webkit/tools/layout_tests/test_expectations.txt View 1 2 3 1 chunk +0 lines, -4 lines 0 comments Download
M webkit/tools/layout_tests/webkitpy/layout_tests/layout_package/websocket_server.py View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M webkit/tools/test_shell/simple_socket_stream_bridge.cc View 4 chunks +5 lines, -3 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
ukai
10 years, 10 months ago (2010-02-16 09:41:18 UTC) #1
Paweł Hajdan Jr.
Drive-by with some test comments. http://codereview.chromium.org/601077/diff/1/14 File net/websockets/websocket_job_unittest.cc (right): http://codereview.chromium.org/601077/diff/1/14#newcode219 net/websockets/websocket_job_unittest.cc:219: for (size_t i = ...
10 years, 10 months ago (2010-02-16 09:49:09 UTC) #2
ukai
Thanks for review. http://codereview.chromium.org/601077/diff/1/14 File net/websockets/websocket_job_unittest.cc (right): http://codereview.chromium.org/601077/diff/1/14#newcode219 net/websockets/websocket_job_unittest.cc:219: for (size_t i = 0; i ...
10 years, 10 months ago (2010-02-16 10:10:32 UTC) #3
ukai
ping
10 years, 10 months ago (2010-02-19 10:21:10 UTC) #4
Paweł Hajdan Jr.
The bits I commented about in the drive-by LGTM. Darin: please note that you're the ...
10 years, 10 months ago (2010-02-19 13:27:46 UTC) #5
darin (slow to review)
Sorry for the slow reply. This was lost in my inbox :( I'm working on ...
10 years, 10 months ago (2010-02-19 18:10:55 UTC) #6
ukai
Thanks for review. http://codereview.chromium.org/601077/diff/2002/5008 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/601077/diff/2002/5008#newcode16 net/socket_stream/socket_stream_job.h:16: On 2010/02/19 18:10:55, darin wrote: > ...
10 years, 10 months ago (2010-02-22 07:48:55 UTC) #7
darin (slow to review)
nice work. LGTM http://codereview.chromium.org/601077/diff/7003/7011 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/601077/diff/7003/7011#newcode18 net/socket_stream/socket_stream_job.h:18: // If a protocol (e.g. WebSocket ...
10 years, 10 months ago (2010-02-24 18:51:58 UTC) #8
ukai
10 years, 9 months ago (2010-03-01 02:36:24 UTC) #9
http://codereview.chromium.org/601077/diff/7003/7011
File net/socket_stream/socket_stream_job.h (right):

http://codereview.chromium.org/601077/diff/7003/7011#newcode18
net/socket_stream/socket_stream_job.h:18: // If a protocol (e.g. WebSocket
protocl) needs to inspect/modify data
On 2010/02/24 18:51:59, darin wrote:
> nit: protocl -> protocol

Done.

http://codereview.chromium.org/601077/diff/7003/7014
File net/websockets/websocket_job.cc (right):

http://codereview.chromium.org/601077/diff/7003/7014#newcode188
net/websockets/websocket_job.cc:188: original_handshake_request_ +=
std::string(data, len);
On 2010/02/24 18:51:59, darin wrote:
> nit: how about using the .append method instead, which avoids the extra
> std::string temporary object?

Done.

Powered by Google App Engine
This is Rietveld 408576698