Skip header lines with invalid header name

Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RFC 7230 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365
Committed: https://crrev.com/78f29ac0214e16bdcc6eaa17fa4a5a84882bb7a8
Cr-Commit-Position: refs/heads/master@{#402498}

[robwu, 2016-06-28 06:28:45]

rob@robwu.nl changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-06-28 14:51:20]

Thanks for doing this!  In your description, RGC -> RFC, and you should cite RFC
7230 instead of 2616 which is now obsolete.

[mmenke, 2016-06-28 14:54:01]

LGTM.  Even if FireFox is doing something similar, it could be just different
enough that we run into problems, but agree that this makes sense.

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:913: if (!IsToken(name_begin_, name_end_))
IsValidHeaderName seems clearer (It just calls IsToken, so it does the exact
same thing).

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util_unittest.cc
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util_unittest...
net/http/http_util_unittest.cc:199: }
Could you split this into two tests?  Not too concerned with what you call them
(HeadersIterator_MalformedName1 / HeadersIterator_MalformedName2, or
HeadersIterator_MalformedNameTwoHeaders for the second one, or whatever).

Just makes regressions marginally simpler to debug.

[robwu, 2016-06-28 15:24:29]

Description was changed from

==========
Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RGC 2616 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365
==========

to

==========
Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RFC 7230 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365
==========

[robwu, 2016-06-28 15:24:42]

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:913: if (!IsToken(name_begin_, name_end_))
On 2016/06/28 14:54:00, mmenke wrote:
> IsValidHeaderName seems clearer (It just calls IsToken, so it does the exact
> same thing).

IsValidHeaderName takes a std::string, while we only have two iterators.

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util_unittest.cc
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util_unittest...
net/http/http_util_unittest.cc:199: }
On 2016/06/28 14:54:01, mmenke wrote:
> Could you split this into two tests?  Not too concerned with what you call
them
> (HeadersIterator_MalformedName1 / HeadersIterator_MalformedName2, or
> HeadersIterator_MalformedNameTwoHeaders for the second one, or whatever).
> 
> Just makes regressions marginally simpler to debug.

Done.

[mmenke, 2016-06-28 15:26:17]

Still LGTM

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/2101963002/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:913: if (!IsToken(name_begin_, name_end_))
On 2016/06/28 15:24:42, robwu wrote:
> On 2016/06/28 14:54:00, mmenke wrote:
> > IsValidHeaderName seems clearer (It just calls IsToken, so it does the exact
> > same thing).
> 
> IsValidHeaderName takes a std::string, while we only have two iterators.

Oops, good point.

[robwu, 2016-06-28 15:53:21]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-06-28 15:53:52]

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-06-28 17:33:58]

Description was changed from

==========
Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RFC 7230 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365
==========

to

==========
Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RFC 7230 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365
==========

[commit-bot: I haz the power, 2016-06-28 17:34:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-28 17:35:34]

Description was changed from

==========
Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RFC 7230 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365
==========

to

==========
Skip header lines with invalid header name

The current header parsing algorithm is too lax and results in the
creation of a header data structure with invalid headers. This patch
makes the parser more strict, by rejecting header names that are not
complying with RFC 7230 (field-name must be a token).
Enforcing this standard should be web-compatible since Firefox (47)
also follows this behavior.

BUG=620365

Committed: https://crrev.com/78f29ac0214e16bdcc6eaa17fa4a5a84882bb7a8
Cr-Commit-Position: refs/heads/master@{#402498}
==========

[commit-bot: I haz the power, 2016-06-28 17:35:34]

Patchset 2 (id:??) landed as
https://crrev.com/78f29ac0214e16bdcc6eaa17fa4a5a84882bb7a8
Cr-Commit-Position: refs/heads/master@{#402498}