Normalize and update the header value checks to RFC 7230 for Fetch

Normalize and update the header value checks to RFC 7230
for Fetch. Normalize the Header value for append() and set() api.

To normalize a value, remove any leading and trailing HTTP whitespace bytes from it.
HTTP whitespace bytes are 0x09, 0x0A, 0x0D, and 0x20.

Intent to update header is:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K5jd8Y5l6pI

Spec link:
https://fetch.spec.whatwg.org/#concept-header-value-normalize

BUG=455099

[shiva.jm, 2015-09-08 14:18:15]

shiva.jm@samsung.com changed reviewers:
+ hiroshige@chromium.org, tyoshino@chromium.org, yhirano@chromium.org

[shiva.jm, 2015-09-08 14:18:16]

pls have a look.

[yhirano, 2015-09-09 04:41:50]

IIUC this CL also changes the way to check header values, right? If so, please
update the issue title and description.

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchUtils.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
Source/core/fetch/FetchUtils.cpp:17: static bool isHTTPWhitespace(UChar chr)
Please place this function in the unnamed namespace below instead of using
"static".

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
File Source/modules/fetch/FetchHeaderList.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.cpp:157: }
+newline

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
File Source/modules/fetch/FetchHeaderList.h (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.h:46: static bool isValidHeaderValue(const
String&);
Is this function used?

[hiroshige, 2015-09-09 07:01:48]

How about mentioning in CL title/description that this CL also updates the
header value check to RFC 7230?

I also posted draft intent-to-deprecate corresponding to this to crbug.

Note: I assume the issue https://github.com/whatwg/fetch/issues/115 is accepted
and the spec is updated accordingly (i.e. the empty string is considered as a
valid header value).
I'll ping Anne to confirm.

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right):

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:82: 'test\r\n
data', 'test\0', '', ' ', '\x7f', 't\bt', 't\vt',
'', ' ', and '\r\n'.repeat(50000) will be valid.

Also, how about adding tests for all rejected octets, i.e.
'\x01', ..., '\x08', '\x0b', '\x0c', '\x0e', ..., '\x1f'?

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/headers.js (right):

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/headers.js:188: // test for removing
leading and trailing white spaces
nit: How about "Tests for normalizing header values."?
- s/test/Tests/
- Add a period at the end of the statement.
- Including the word of "normalize" or so will help readers to correspond the
tests with the spec.

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchUtils.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
Source/core/fetch/FetchUtils.cpp:19: return chr <= ' ' && (chr == ' ' || chr ==
'\n' || chr == '\t' || chr == '\r');
"chr <= ' ' && " is not needed.

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/H...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:117: if
(!FetchHeaderList::isValidFieldContentRFC7230(normalizedValue)) {
The spec will also allow the empty string:
https://github.com/whatwg/fetch/issues/115.
(The spec is not yet updated though.)

So, how about
1. Creating a new function (e.g. isValidHeaderValueRFC7230()? not sure about the
name though) in FetchHeaderList that returns
value.isEmpty() || FetchHeaderList::isValidFieldContentRFC7230(value)
2. and using the newly created function here?

The newly created function at 1. will need a comment that clarifies that the
function checks the header value according to the latest Fetch API's header
value definition (https://fetch.spec.whatwg.org/#concept-header-value) and RFC
7230 and it is different from isValidHTTPHeaderValue().

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:225: if
(!FetchHeaderList::isValidFieldContentRFC7230(normalizedValue)) {
ditto.

[shiva.jm, 2015-09-10 10:10:27]

updated patch with comments, pls have a look.

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js (right):

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js:82: 'test\r\n
data', 'test\0', '', ' ', '\x7f', 't\bt', 't\vt',
On 2015/09/09 07:01:48, hiroshige (slow) wrote:
> '', ' ', and '\r\n'.repeat(50000) will be valid.
> 
> Also, how about adding tests for all rejected octets, i.e.
> '\x01', ..., '\x08', '\x0b', '\x0c', '\x0e', ..., '\x1f'?

Done.

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/headers.js (right):

https://codereview.chromium.org/1288263003/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/headers.js:188: // test for removing
leading and trailing white spaces
On 2015/09/09 07:01:48, hiroshige (slow) wrote:
> nit: How about "Tests for normalizing header values."?
> - s/test/Tests/
> - Add a period at the end of the statement.
> - Including the word of "normalize" or so will help readers to correspond the
> tests with the spec.

Done.

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchUtils.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
Source/core/fetch/FetchUtils.cpp:17: static bool isHTTPWhitespace(UChar chr)
On 2015/09/09 04:41:50, yhirano wrote:
> Please place this function in the unnamed namespace below instead of using
> "static".

Done.

https://codereview.chromium.org/1288263003/diff/100001/Source/core/fetch/Fetc...
Source/core/fetch/FetchUtils.cpp:19: return chr <= ' ' && (chr == ' ' || chr ==
'\n' || chr == '\t' || chr == '\r');
On 2015/09/09 07:01:48, hiroshige (slow) wrote:
> "chr <= ' ' && " is not needed.

Done.

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
File Source/modules/fetch/FetchHeaderList.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.cpp:157: }
On 2015/09/09 04:41:50, yhirano wrote:
> +newline

Done.

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
File Source/modules/fetch/FetchHeaderList.h (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.h:46: static bool isValidHeaderValue(const
String&);
On 2015/09/09 04:41:50, yhirano wrote:
> Is this function used?

not used removed.

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/H...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:117: if
(!FetchHeaderList::isValidFieldContentRFC7230(normalizedValue)) {
On 2015/09/09 07:01:48, hiroshige (slow) wrote:
> The spec will also allow the empty string:
> https://github.com/whatwg/fetch/issues/115.
> (The spec is not yet updated though.)
> 
> So, how about
> 1. Creating a new function (e.g. isValidHeaderValueRFC7230()? not sure about
the
> name though) in FetchHeaderList that returns
> value.isEmpty() || FetchHeaderList::isValidFieldContentRFC7230(value)
> 2. and using the newly created function here?
> 
> The newly created function at 1. will need a comment that clarifies that the
> function checks the header value according to the latest Fetch API's header
> value definition (https://fetch.spec.whatwg.org/#concept-header-value) and RFC
> 7230 and it is different from isValidHTTPHeaderValue().
> 

Done.

https://codereview.chromium.org/1288263003/diff/100001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:225: if
(!FetchHeaderList::isValidFieldContentRFC7230(normalizedValue)) {
On 2015/09/09 07:01:48, hiroshige (slow) wrote:
> ditto.

Done.

[shiva.jm, 2015-09-10 10:11:44]

shiva.jm@samsung.com changed reviewers:
+ mkwst@chromium.org

[shiva.jm, 2015-09-10 14:12:12]

updated the description with intent link.

[yhirano, 2015-09-11 04:22:25]

LGTM, but please wait for the intent-to-deprecate-to-remove to be approved.

https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/H...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:109: // "1. Normalize value."
This value should be written as |value|.

https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:217: // "1. Normalize value."
ditto

[shiva.jm, 2015-09-11 04:34:16]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-11 04:34:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288263003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288263003/140001

[commit-bot: I haz the power, 2015-09-11 07:26:24]

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

[commit-bot: I haz the power, 2015-09-11 07:26:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shiva.jm, 2015-09-11 07:42:50]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-11 07:43:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288263003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288263003/140001

[commit-bot: I haz the power, 2015-09-11 08:31:57]

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

[commit-bot: I haz the power, 2015-09-11 08:31:57]

Dry run: This issue passed the CQ dry run.

[shiva.jm, 2015-09-14 03:41:48]

Updated the patch with review comments.
From the intent discussion, it looks like we have to do change for both XHR and
Fetch. But anne highlighted only about header value normalization, though Fetch
started
using 7230 spec.

https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/H...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:109: // "1. Normalize value."
On 2015/09/11 04:22:25, yhirano wrote:
> This value should be written as |value|.

Done.

https://codereview.chromium.org/1288263003/diff/140001/Source/modules/fetch/H...
Source/modules/fetch/Headers.cpp:217: // "1. Normalize value."
On 2015/09/11 04:22:25, yhirano wrote:
> ditto

Done.

[shiva.jm, 2015-09-14 03:42:04]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 03:42:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288263003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288263003/160001

[commit-bot: I haz the power, 2015-09-14 04:51:27]

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

[commit-bot: I haz the power, 2015-09-14 04:51:28]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-09-14 09:15:29]

On 2015/09/14 04:51:28, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

> From the intent discussion, it looks like we have to do change for both XHR
and
> Fetch.
Seems yes.

Before proceeding to update XHR, I'd like to collect more accurate stats for
XHR.
I commented in the previous CL that headers with leading/trailing whitespaces
are affected by our update, but in the case of XHRs, they are affected only when
multiple setRequestHeader() is called more than once for a header name, because
leading/trailing header values seem omitted before the request is sent to the
network.
So we can exclude some header values from deprecation count.
e.g.

xhr.setRequestHeader('a', '\tb\t');
results in:
a: b
(so header value normalization has no effect)

xhr.setRequestHeader('a', '\tb\t'); xhr.setRequestHeader('a', '\tc\t');
results in:
a: b\t, \tc
(header value normalization has effect in this case; with normalization we'll
get "a: b, c")

This might decrease the count further.

I'd like to a CL for the change described above using
FetchUtils::normalizeHeaderValue in this CL.
So, could you split the changes of Source/core/fetch/FetchUtils.h and .cpp into
a new CL?

> But anne highlighted only about header value normalization, though Fetch
> started using 7230 spec.
I'll respond with summary of differences between current Blink/Chromium
implementation, RFC 2616 and RFC 7230.

[shiva.jm, 2015-09-14 11:21:24]

On 2015/09/14 09:15:29, hiroshige wrote:
> On 2015/09/14 04:51:28, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> > From the intent discussion, it looks like we have to do change for both XHR
> and
> > Fetch.
> Seems yes.
> 
> Before proceeding to update XHR, I'd like to collect more accurate stats for
> XHR.
> I commented in the previous CL that headers with leading/trailing whitespaces
> are affected by our update, but in the case of XHRs, they are affected only
when
> multiple setRequestHeader() is called more than once for a header name,
because
> leading/trailing header values seem omitted before the request is sent to the
> network.
> So we can exclude some header values from deprecation count.
> e.g.
> 
> xhr.setRequestHeader('a', '\tb\t');
> results in:
> a: b
> (so header value normalization has no effect)
> 
> xhr.setRequestHeader('a', '\tb\t'); xhr.setRequestHeader('a', '\tc\t');
> results in:
> a: b\t, \tc
> (header value normalization has effect in this case; with normalization we'll
> get "a: b, c")
> 
> This might decrease the count further.
> 
> I'd like to a CL for the change described above using
> FetchUtils::normalizeHeaderValue in this CL.
> So, could you split the changes of Source/core/fetch/FetchUtils.h and .cpp
into
> a new CL?
> 
> > But anne highlighted only about header value normalization, though Fetch
> > started using 7230 spec.
> I'll respond with summary of differences between current Blink/Chromium
> implementation, RFC 2616 and RFC 7230.

So, if i get correctly, we want to split the header value normalization
changes in seperate CL ?. so move changes of Source/core/fetch/FetchUtils.h 
and .cpp into a new CL and do not do call normalization api in Headers.cpp for
append() and set() api in these CL, also modify test rigt ?

[hiroshige, 2015-09-15 05:21:23]

> So, if i get correctly, we want to split the header value normalization
> changes in seperate CL ?. so move changes of Source/core/fetch/FetchUtils.h 
> and .cpp into a new CL and do not do call normalization api in Headers.cpp for
> append() and set() api in these CL
Yes, but we don't have to modify tests (just splitting into two):
CL 1 (new CL): Source/core/fetch/FetchUtils.h/cpp changes in Patch Set 9.
  We can commit this once we get l-g-t-ms because we don't call normalization in
Headers in this CL.
CL 2 (this CL): Patch Set 9 excluding Source/core/fetch/FetchUtils.h/cpp.
  We call normalization in Headers. We have to wait the discussion is
stabilized.

[shiva.jm, 2015-09-15 10:52:47]

updated the patch to split into 2 CL, pls have a look.

[yhirano, 2016-09-27 01:19:26]

yhirano@chromium.org changed reviewers:
- yhirano@chromium.org

[yhirano, 2016-09-27 01:19:27]

(removing myself from the reviewer list)