Support HttpOnly cookie on Web Socket

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

[Paweł Hajdan Jr., 2010-02-16 09:49:09]

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 = 0; i <
lines.size() - 2; i++) {
nit: Please add SCOPED_TRACE so that the error messages carry more info.

http://codereview.chromium.org/601077/diff/1/14#newcode231
net/websockets/websocket_job_unittest.cc:231: for (size_t i = 0; i <
lines.size() - 2; i++) {
nit: Please add SCOPED_TRACE so that the error messages carry more info.

http://codereview.chromium.org/601077/diff/1/14#newcode251
net/websockets/websocket_job_unittest.cc:251: for (size_t i = 0; i <
lines.size() - 2; i++) {
nit: Please add SCOPED_TRACE so that the error messages carry more info.

[ukai, 2010-02-16 10:10:32]

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 <
lines.size() - 2; i++) {
On 2010/02/16 09:49:09, Paweł Hajdan Jr. wrote:
> nit: Please add SCOPED_TRACE so that the error messages carry more info.

Done.

http://codereview.chromium.org/601077/diff/1/14#newcode231
net/websockets/websocket_job_unittest.cc:231: for (size_t i = 0; i <
lines.size() - 2; i++) {
On 2010/02/16 09:49:09, Paweł Hajdan Jr. wrote:
> nit: Please add SCOPED_TRACE so that the error messages carry more info.

Done.

http://codereview.chromium.org/601077/diff/1/14#newcode251
net/websockets/websocket_job_unittest.cc:251: for (size_t i = 0; i <
lines.size() - 2; i++) {
On 2010/02/16 09:49:09, Paweł Hajdan Jr. wrote:
> nit: Please add SCOPED_TRACE so that the error messages carry more info.

Done.

[ukai, 2010-02-19 10:21:10]

ping

[Paweł Hajdan Jr., 2010-02-19 13:27:46]

The bits I commented about in the drive-by LGTM.

Darin: please note that you're the main reviewer for this change. I just added
minor comments about one test.

[darin (slow to review), 2010-02-19 18:10:55]

Sorry for the slow reply.  This was lost in my inbox :(  I'm working on a better
email filter!!

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: 
please add some comments here explaining how this class should be used.

http://codereview.chromium.org/601077/diff/2002/5008#newcode41
net/socket_stream/socket_stream_job.h:41: virtual URLRequestContext* context()
const {
you should not use the "unix_hacker" naming convention for virtual methods. 
that naming convention is reserved for functions that are cheap to call.

These should be GetContext and SetContext, or they should be made non-virtual if
they do not need to be virtual.

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

http://codereview.chromium.org/601077/diff/2002/5011#newcode55
net/websockets/websocket_job.cc:55: 
nit: delete this extra new line

http://codereview.chromium.org/601077/diff/2002/5011#newcode183
net/websockets/websocket_job.cc:183: if (cookie_store) {
unfortunately, this still needs to respect the CookiePolicy.  you need to call
CanGetCookies, and that may complete asynchronously.  you have to do this before
you call GetCookiesWithOptions.  this is important because some cookies that
were set by assignment to document.cookie may not be stored yet (pending user
prompting).  so we need to wait for those prompts to clear before we read the
cookie store.  this 'synchronization' is all done for you if you use
CookiePolicy::CanGetCookies.

i'm afraid having to support an asynchronous completion here is probably going
to make this code more complicated :(

http://codereview.chromium.org/601077/diff/2002/5011#newcode238
net/websockets/websocket_job.cc:238: if (LowerCaseEqualsASCII(iter.name_begin(),
iter.name_end(),
maybe this can loop over kSetCookieHeaders?

http://codereview.chromium.org/601077/diff/2002/5011#newcode242
net/websockets/websocket_job.cc:242: cookie_store->SetCookieWithOptions(
same deal as above.  you need to use CookiePolicy::CanSetCookie before calling
SetCookieWithOptions.

[ukai, 2010-02-22 07:48:55]

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:
> please add some comments here explaining how this class should be used.

Done.

http://codereview.chromium.org/601077/diff/2002/5008#newcode41
net/socket_stream/socket_stream_job.h:41: virtual URLRequestContext* context()
const {
On 2010/02/19 18:10:55, darin wrote:
> you should not use the "unix_hacker" naming convention for virtual methods. 
> that naming convention is reserved for functions that are cheap to call.
> 
> These should be GetContext and SetContext, or they should be made non-virtual
if
> they do not need to be virtual.

they don't need to be virtual.

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

http://codereview.chromium.org/601077/diff/2002/5011#newcode55
net/websockets/websocket_job.cc:55: 
On 2010/02/19 18:10:55, darin wrote:
> nit: delete this extra new line

Done.

http://codereview.chromium.org/601077/diff/2002/5011#newcode183
net/websockets/websocket_job.cc:183: if (cookie_store) {
On 2010/02/19 18:10:55, darin wrote:
> unfortunately, this still needs to respect the CookiePolicy.  you need to call
> CanGetCookies, and that may complete asynchronously.  you have to do this
before
> you call GetCookiesWithOptions.  this is important because some cookies that
> were set by assignment to document.cookie may not be stored yet (pending user
> prompting).  so we need to wait for those prompts to clear before we read the
> cookie store.  this 'synchronization' is all done for you if you use
> CookiePolicy::CanGetCookies.
> 
> i'm afraid having to support an asynchronous completion here is probably going
> to make this code more complicated :(

Done.

http://codereview.chromium.org/601077/diff/2002/5011#newcode238
net/websockets/websocket_job.cc:238: if (LowerCaseEqualsASCII(iter.name_begin(),
iter.name_end(),
On 2010/02/19 18:10:55, darin wrote:
> maybe this can loop over kSetCookieHeaders?

Done.

http://codereview.chromium.org/601077/diff/2002/5011#newcode242
net/websockets/websocket_job.cc:242: cookie_store->SetCookieWithOptions(
On 2010/02/19 18:10:55, darin wrote:
> same deal as above.  you need to use CookiePolicy::CanSetCookie before calling
> SetCookieWithOptions.

Done.

[darin (slow to review), 2010-02-24 18:51:58]

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
protocl) needs to inspect/modify data
nit: protocl -> protocol

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);
nit: how about using the .append method instead, which avoids the extra
std::string temporary object?

[ukai, 2010-03-01 02:36:24]

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.