WebSocketExtensionParser: reject top-bit-set characters

WebSocketExtensionParser: reject top-bit-set characters

net::WebSocketExtensionParser would accept top-bit-set characters in
tokens. Change it to use net::HttpUtil::IsTokenChar() so it accepts the same
characters as everything else.

Add some tests for top-bit-set characters and a regression test for issue
647156.

Also various minor changes to make the code more Chromey.

BUG=647156
Committed: https://crrev.com/66318666019b16c7315c5e1747101bc7e8e9bab7
Cr-Commit-Position: refs/heads/master@{#419125}

[Adam Rice, 2016-09-15 10:21:50]

The CQ bit was checked by ricea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 10:22:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 11:11:39]

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

[commit-bot: I haz the power, 2016-09-15 11:11:39]

Dry run: This issue passed the CQ dry run.

[Adam Rice, 2016-09-16 00:58:38]

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

[tyoshino (SeeGerritForStatus), 2016-09-16 04:44:22]

tyoshino@chromium.org changed reviewers:
+ tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-09-16 04:44:23]

lgtm

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
net/websockets/websocket_extension_parser.cc:112: while (current_ < end_ &&
!IsControl(*current_)) {
we can remove this IsControl() call?

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
net/websockets/websocket_extension_parser.cc:113: if (*current_ == '\\') {
good catch

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
net/websockets/websocket_extension_parser.cc:121: break;
we can return with false here too?

[Adam Rice, 2016-09-16 05:57:56]

+tyoshino I reorganised ConsumeQuotedToken() a bit more so if you could have a
quick look at it I would appreciate it.

+yhirano If you still want to review this, please say so.

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
net/websockets/websocket_extension_parser.cc:112: while (current_ < end_ &&
!IsControl(*current_)) {
On 2016/09/16 04:44:23, tyoshino wrote:
> we can remove this IsControl() call?

Yes! I didn't notice that during refactoring it had become
unnecessary.

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
net/websockets/websocket_extension_parser.cc:121: break;
On 2016/09/16 04:44:23, tyoshino wrote:
> we can return with false here too?

I tried it but it doesn't work. If the character is a '"' we need to fall
through.

This code was quite subtle so I have changed it to be more obvious. PTAL.

[tyoshino (SeeGerritForStatus), 2016-09-16 06:44:24]

lgtm

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
File net/websockets/websocket_extension_parser.cc (right):

https://codereview.chromium.org/2344873002/diff/1/net/websockets/websocket_ex...
net/websockets/websocket_extension_parser.cc:121: break;
On 2016/09/16 05:57:56, Adam Rice wrote:
> On 2016/09/16 04:44:23, tyoshino wrote:
> > we can return with false here too?
> 
> I tried it but it doesn't work. If the character is a '"' we need to fall
> through.
> 
> This code was quite subtle so I have changed it to be more obvious. PTAL.

Got it!

[Adam Rice, 2016-09-16 06:45:24]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2016-09-16 06:45:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 07:47:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-16 07:48:40]

Description was changed from

==========
WebSocketExtensionParser: reject top-bit-set characters

net::WebSocketExtensionParser would accept top-bit-set characters in
tokens. Change it to use net::HttpUtil::IsTokenChar() so it accepts the same
characters as everything else.

Add some tests for top-bit-set characters and a regression test for issue
647156.

Also various minor changes to make the code more Chromey.

BUG=647156
==========

to

==========
WebSocketExtensionParser: reject top-bit-set characters

net::WebSocketExtensionParser would accept top-bit-set characters in
tokens. Change it to use net::HttpUtil::IsTokenChar() so it accepts the same
characters as everything else.

Add some tests for top-bit-set characters and a regression test for issue
647156.

Also various minor changes to make the code more Chromey.

BUG=647156

Committed: https://crrev.com/66318666019b16c7315c5e1747101bc7e8e9bab7
Cr-Commit-Position: refs/heads/master@{#419125}
==========

[commit-bot: I haz the power, 2016-09-16 07:48:41]

Patchset 4 (id:??) landed as
https://crrev.com/66318666019b16c7315c5e1747101bc7e8e9bab7
Cr-Commit-Position: refs/heads/master@{#419125}