Fix WebSocketServer extension parser.

Fix WebSocketServer extension parser.

This CL makes the WebSocket server in net/server use the net/websockets
parser for parsing Sec-WebSocket-Extensions in the extension negotiation.

The new implementation validates the extension negotiation offer more
strictly than before. Specifically,

- Malformed Sec-WebSocket-Extensions header value causes connection failure.
 - Previously it was just ignored.
- Malformed permessage-deflate parameters are declined.
 - Previously part of such params were accepted partially.

BUG=523228
Committed: https://crrev.com/a10dd4ef5e42a54bea6ce71ef3f3d9f974dbb37e
Cr-Commit-Position: refs/heads/master@{#351040}

[yhirano, 2015-09-11 10:40:12]

Patchset #1 (id:1) has been deleted

[yhirano, 2015-09-11 10:40:17]

Patchset #1 (id:20001) has been deleted

[yhirano, 2015-09-11 11:09:59]

Patchset #3 (id:80001) has been deleted

[yhirano, 2015-09-11 11:10:02]

Patchset #2 (id:60001) has been deleted

[yhirano, 2015-09-11 11:10:05]

Patchset #1 (id:40001) has been deleted

[yhirano, 2015-09-11 11:57:16]

yhirano@chromium.org changed reviewers:
+ dgozman@chromium.org, tyoshino@chromium.org

[yhirano, 2015-09-11 11:57:16]

This CL depends on https://codereview.chromium.org/1324113002/ and
https://codereview.chromium.org/1334113003/.

[tyoshino (SeeGerritForStatus), 2015-09-11 12:18:40]

https://codereview.chromium.org/1340523002/diff/20002/net/server/web_socket.cc
File net/server/web_socket.cc (right):

https://codereview.chromium.org/1340523002/diff/20002/net/server/web_socket.c...
net/server/web_socket.cc:26: const char kWebSocketGuid[] =
"258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
common to net/websockets/websocket_handshake_constants.cc. let's consider
merging later.

https://codereview.chromium.org/1340523002/diff/20002/net/websockets/websocke...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/20002/net/websockets/websocke...
net/websockets/websocket_extension.cc:57: if (param.HasValue()) {
if (!param.HasValue())
  continue;

[dgozman, 2015-09-11 17:49:36]

Could you please elaborate on what does this fix? Changes in
web_socket{_encoder} do not look like changing behavior, but I probably miss
something.

https://codereview.chromium.org/1340523002/diff/100001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/100001/net/server/web_socket_...
net/server/web_socket_encoder.cc:276: has_error_ =
!deflater_->Initialize(params.PermissiveServerMaxWindowBits()) ||
Why don't you just pass |is_server|, |inflater| and |deflater| in constructor
and remove |has_error| entirely. This way, factory method will be responsible
for error handling, and you don't need AsServer and AsClient.

[yhirano, 2015-09-14 04:28:51]

Patchset #4 (id:140001) has been deleted

[yhirano, 2015-09-14 09:05:08]

Patchset #4 (id:160001) has been deleted

[yhirano, 2015-09-14 09:10:35]

Patchset #4 (id:180001) has been deleted

[yhirano, 2015-09-14 09:10:40]

Patchset #4 (id:180001) has been deleted

[yhirano, 2015-09-15 06:14:06]

Patchset #4 (id:200001) has been deleted

[yhirano, 2015-09-15 06:14:13]

Patchset #3 (id:120001) has been deleted

[yhirano, 2015-09-15 06:25:57]

PS3 declines invalid permessage-deflate extensions and rejects malformed
extensions headers (PS1 and PS2 rejected both).

> Could you please elaborate on what does this fix? Changes in
> web_socket{_encoder} do not look like changing behavior, but I probably miss
> something.

Done.

https://codereview.chromium.org/1340523002/diff/20002/net/server/web_socket.cc
File net/server/web_socket.cc (right):

https://codereview.chromium.org/1340523002/diff/20002/net/server/web_socket.c...
net/server/web_socket.cc:26: const char kWebSocketGuid[] =
"258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
On 2015/09/11 12:18:40, tyoshino wrote:
> common to net/websockets/websocket_handshake_constants.cc. let's consider
> merging later.

Done.

https://codereview.chromium.org/1340523002/diff/20002/net/websockets/websocke...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/20002/net/websockets/websocke...
net/websockets/websocket_extension.cc:57: if (param.HasValue()) {
On 2015/09/11 12:18:40, tyoshino wrote:
> if (!param.HasValue())
>   continue;

Done.

https://codereview.chromium.org/1340523002/diff/100001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/100001/net/server/web_socket_...
net/server/web_socket_encoder.cc:276: has_error_ =
!deflater_->Initialize(params.PermissiveServerMaxWindowBits()) ||
On 2015/09/11 17:49:36, dgozman wrote:
> Why don't you just pass |is_server|, |inflater| and |deflater| in constructor
> and remove |has_error| entirely. This way, factory method will be responsible
> for error handling, and you don't need AsServer and AsClient.

Done. I generally don't like |true /* is_server */| pattern and introduced Type
enum.

[dgozman, 2015-09-15 17:26:19]

net/server lgtm

https://codereview.chromium.org/1340523002/diff/220001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/220001/net/server/web_socket_...
net/server/web_socket_encoder.cc:278: : is_server_(type == FOR_SERVER),
Let's go ahead and change |is_server_| to |type_|.

[yhirano, 2015-09-16 05:28:17]

https://codereview.chromium.org/1340523002/diff/220001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/220001/net/server/web_socket_...
net/server/web_socket_encoder.cc:278: : is_server_(type == FOR_SERVER),
On 2015/09/15 17:26:19, dgozman wrote:
> Let's go ahead and change |is_server_| to |type_|.

Done.

[yhirano, 2015-09-16 07:48:13]

Patchset #5 (id:260001) has been deleted

[tyoshino (SeeGerritForStatus), 2015-09-16 09:33:10]

lgtm

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:202: WebSocketDeflateParameters offered,
response;
offered -> accepted_offered_params or accepted_params or accepted_offer, etc.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:202: WebSocketDeflateParameters offered,
response;
response can be declared at L219

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:223: response.SetClientMaxWindowBits(8);
15? We had offline discussion.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:244: // TODO(yhirano): Add a way to return an
error.
please also mark lines where we should error the encoder by adding TODOs

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:254: }
handle no extension case?

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
File net/server/web_socket_encoder.h (right):

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.h:33: const std::string& extensions,
request_extensions, or change to extensions in .cc file too

https://codereview.chromium.org/1340523002/diff/270001/net/websockets/websock...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/270001/net/websockets/websock...
net/websockets/websocket_extension.cc:65: result += '"';
per HTTP rfc, we need to scan all non-token characters.

In WebSocket extension header, any non token character is disallowed even in the
quoted-string. So, this is unnecessary and should be DCHECK or something. It's
ok to address this later. Leave TODO?

[yhirano, 2015-09-16 10:46:51]

Patchset #6 (id:290001) has been deleted

[yhirano, 2015-09-16 10:47:47]

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:202: WebSocketDeflateParameters offered,
response;
On 2015/09/16 09:33:10, tyoshino wrote:
> response can be declared at L219

Done.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:202: WebSocketDeflateParameters offered,
response;
On 2015/09/16 09:33:10, tyoshino wrote:
> offered -> accepted_offered_params or accepted_params or accepted_offer, etc.

I removed this variable and renamed |params| to |offer|.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:223: response.SetClientMaxWindowBits(8);
On 2015/09/16 09:33:10, tyoshino wrote:
> 15? We had offline discussion.

Done.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:244: // TODO(yhirano): Add a way to return an
error.
On 2015/09/16 09:33:10, tyoshino wrote:
> please also mark lines where we should error the encoder by adding TODOs

Done.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:254: }
On 2015/09/16 09:33:10, tyoshino wrote:
> handle no extension case?

It is handled in L246-L249. I added comments.

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
File net/server/web_socket_encoder.h (right):

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.h:33: const std::string& extensions,
On 2015/09/16 09:33:10, tyoshino wrote:
> request_extensions, or change to extensions in .cc file too

Done.

https://codereview.chromium.org/1340523002/diff/270001/net/websockets/websock...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/270001/net/websockets/websock...
net/websockets/websocket_extension.cc:65: result += '"';
On 2015/09/16 09:33:10, tyoshino wrote:
> per HTTP rfc, we need to scan all non-token characters.
> 
> In WebSocket extension header, any non token character is disallowed even in
the
> quoted-string. So, this is unnecessary and should be DCHECK or something. It's
> ok to address this later. Leave TODO?

Done.

[tyoshino (SeeGerritForStatus), 2015-09-17 02:24:37]

lgtm

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
File net/server/web_socket_encoder.cc (right):

https://codereview.chromium.org/1340523002/diff/270001/net/server/web_socket_...
net/server/web_socket_encoder.cc:254: }
On 2015/09/16 10:47:46, yhirano wrote:
> On 2015/09/16 09:33:10, tyoshino wrote:
> > handle no extension case?
> 
> It is handled in L246-L249. I added comments.

Ah, got it!

https://codereview.chromium.org/1340523002/diff/310001/net/websockets/websock...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/310001/net/websockets/websock...
net/websockets/websocket_extension.cc:62:
DCHECK(HttpUtil::IsToken(param.value()));
the parser is ok. it's checking if the unquoted data consists of token.

we should also add DCHECK to the Parameter constructor?

[yhirano, 2015-09-17 03:32:15]

https://codereview.chromium.org/1340523002/diff/310001/net/websockets/websock...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/310001/net/websockets/websock...
net/websockets/websocket_extension.cc:62:
DCHECK(HttpUtil::IsToken(param.value()));
On 2015/09/17 02:24:37, tyoshino wrote:
> the parser is ok. it's checking if the unquoted data consists of token.
> 
> we should also add DCHECK to the Parameter constructor?

Done.

[yhirano, 2015-09-17 04:00:42]

yhirano@chromium.org changed reviewers:
+ davidben@chromium.org

[yhirano, 2015-09-17 04:00:42]

+davidben for OWNER review.

[davidben, 2015-09-22 19:44:02]

Sorry about the delay? lgtm with some nits.

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket.cc
File net/server/web_socket.cc (right):

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket....
net/server/web_socket.cc:130: server_->Send500(connection_->id(), message);
Why do some codepaths call Close and others not?

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket_...
File net/server/web_socket_encoder_unittest.cc (right):

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket_...
net/server/web_socket_encoder_unittest.cc:15: auto server =
WebSocketEncoder::CreateServer("", &params);
Using auto with smart pointers seems a bad idea, since you lose ownership
information.

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
File net/websockets/websocket_deflate_parameters.h (right):

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
net/websockets/websocket_deflate_parameters.h:114: int PermissiveBits() const {
return is_specified && has_value ? bits : 15; }
Nit: I'd put parens around the conditional. Not obvious which way that
parenthesizes.

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
net/websockets/websocket_extension.cc:54: }
Nit: no curlies

[davidben, 2015-09-22 20:13:57]

On 2015/09/22 19:44:02, David Benjamin wrote:
> Sorry about the delay? lgtm with some nits.

(Not sure where that question mark came from. I think I meant it to be a '!')

[yhirano, 2015-09-28 03:17:06]

Thanks!

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket.cc
File net/server/web_socket.cc (right):

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket....
net/server/web_socket.cc:130: server_->Send500(connection_->id(), message);
On 2015/09/22 19:44:01, David Benjamin wrote:
> Why do some codepaths call Close and others not?

(extensions negotiation)
https://tools.ietf.org/html/rfc6455#section-9.1
"
 If a value is received by either the client or
 the server during negotiation that does not conform to the ABNF
 below, the recipient of such malformed data MUST immediately _Fail
 the WebSocket Connection_.
"

https://tools.ietf.org/html/rfc6455#section-4.2.1
"
 If the server, while reading the handshake, finds that the client did
 not send a handshake that matches the description below (note that as
 per [RFC2616], the order of the header fields is not important),
 including but not limited to any violations of the ABNF grammar
 specified for the components of the handshake, the server MUST stop
 processing the client's handshake and return an HTTP response with an
 appropriate error code (such as 400 Bad Request).
"

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket_...
File net/server/web_socket_encoder_unittest.cc (right):

https://codereview.chromium.org/1340523002/diff/410001/net/server/web_socket_...
net/server/web_socket_encoder_unittest.cc:15: auto server =
WebSocketEncoder::CreateServer("", &params);
On 2015/09/22 19:44:01, David Benjamin wrote:
> Using auto with smart pointers seems a bad idea, since you lose ownership
> information.
> 
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0

Done.

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
File net/websockets/websocket_deflate_parameters.h (right):

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
net/websockets/websocket_deflate_parameters.h:114: int PermissiveBits() const {
return is_specified && has_value ? bits : 15; }
On 2015/09/22 19:44:01, David Benjamin wrote:
> Nit: I'd put parens around the conditional. Not obvious which way that
> parenthesizes.

Done.

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
File net/websockets/websocket_extension.cc (right):

https://codereview.chromium.org/1340523002/diff/410001/net/websockets/websock...
net/websockets/websocket_extension.cc:54: }
On 2015/09/22 19:44:02, David Benjamin wrote:
> Nit: no curlies

Done.

[yhirano, 2015-09-28 03:17:11]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-09-28 03:17:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, tyoshino@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1340523002/#ps430001
(title: " ")

[commit-bot: I haz the power, 2015-09-28 03:17:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340523002/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340523002/430001

[commit-bot: I haz the power, 2015-09-28 07:27:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-28 07:27:44]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[yhirano, 2015-09-28 07:30:45]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-09-28 07:31:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340523002/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340523002/430001

[commit-bot: I haz the power, 2015-09-28 09:06:42]

Committed patchset #12 (id:430001)

[commit-bot: I haz the power, 2015-09-28 09:07:52]

Patchset 12 (id:??) landed as
https://crrev.com/a10dd4ef5e42a54bea6ce71ef3f3d9f974dbb37e
Cr-Commit-Position: refs/heads/master@{#351040}