New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 681769 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
OoO until Feb 4th
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 455093



Sign in to add a comment

Header value normalization not happening properly

Project Member Reported by annevank...@gmail.com, Jan 17 2017

Issue description

Components: Blink>Network>FetchAPI
annevankesteren@ if you can be sure to set the Component to at least "Blink". So that we can triage the web platform issues immediately.

Comment 2 by ricea@chromium.org, Jan 20 2017

Components: Blink>Network>XHR
Status: Available (was: Untriaged)
Owner: raphael....@intel.com
Status: Started (was: Available)
See the log of the work done in 2015 at  bug 455099 .

I checked the most recent data of Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230. 0.9999999% of inputs are valid.

I think we can proceed with adding the normalization, but please take a look at the thread to make sure we're ok with it.
Blocking: 455093
Oh, ... it looks the UMA has been wrong from the beginning. Please wait.
yhirano asked me in https://chromium-review.googlesource.com/c/519144/ about possible simplifications to XHR::SetRequestHeaderInternal(), but I think answering that depends on the UMA counters you mentioned.

SetRequestHeaderInternal() could be dropped altogether if we don't need the RFC7230 checks I think, but from comment #6 it's not clear if that's how we want to proceed.
Sorry, I was wrong. The UMA is correct. NVM #6.

I've updated the old blink-dev@ thread with analysis on compatibility risk.
https://groups.google.com/a/chromium.org/d/msg/blink-dev/K5jd8Y5l6pI/KQW1nyAhBwAJ

#7: Yeah, I was just confused. You can clean up SetRequestHeaderInternal(). Please see my reply to blink-dev@ for details. Thanks!
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d562e29584e78ee12ff1295f6ca8878ca8544df6

commit d562e29584e78ee12ff1295f6ca8878ca8544df6
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Fri Jun 02 17:22:48 2017

XHR: Remove Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230 counter

As discussed in
https://groups.google.com/a/chromium.org/d/msg/blink-dev/K5jd8Y5l6pI/KQW1nyAhBwAJ,
it should be safe to remove the use counter at this point and start
normalizing header values as mandated by the XHR and Fetch specs.

In this CL, we only drop the use counter and related code, thus greatly
simplifying SetRequestHeaderInternal(). The only reason that method was not
removed altogether in the same CL is that other methods call it to set the
Content-Type header without having to go throuh setRequestHeader() and
passing an ExceptionState to it.

There should be no behavior change with this CL.

Bug:  681769 
Change-Id: I248e8085443e0dbaf55964eb618ceaa951de4782
Reviewed-on: https://chromium-review.googlesource.com/521105
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#476702}
[modify] https://crrev.com/d562e29584e78ee12ff1295f6ca8878ca8544df6/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
[modify] https://crrev.com/d562e29584e78ee12ff1295f6ca8878ca8544df6/third_party/WebKit/Source/platform/network/HTTPParsers.cpp
[modify] https://crrev.com/d562e29584e78ee12ff1295f6ca8878ca8544df6/third_party/WebKit/Source/platform/network/HTTPParsers.h
[modify] https://crrev.com/d562e29584e78ee12ff1295f6ca8878ca8544df6/third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp
[modify] https://crrev.com/d562e29584e78ee12ff1295f6ca8878ca8544df6/tools/metrics/histograms/histograms.xml

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb671c7cb427aadc0e2e946ff426775610c25f98

commit bb671c7cb427aadc0e2e946ff426775610c25f98
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Fri Jun 02 20:01:00 2017

Fetch, XHR: Normalize header values before checking them when required.

Adjust to the following spec commits:
* https://github.com/whatwg/fetch/commit/6c00fe28e7a361d2b7e0dda776ebccfaa4c439a5
  "Reference the new HTTP RFCs. Normalize header values"
* https://github.com/whatwg/xhr/commit/90c79d0c0a5dff16266c4b6673eca8bb512343f6
  "Make setRequestHeader() normalize header values just like Fetch does"

In essence, we were skipping the normalization step, which strips leading
and trailing HTTP whitespace characters (0x09, 0x0A, 0x0D and 0x20) from
header values in calls to:
* Headers.append()
* Headers.set()
* XMLHttpRequest.setRequestHeader()

and led to failures in web-platform-tests.

Bug:  681769 
Change-Id: I2dee23a88eabe58c005fbdb353485eeb44be0e21
Reviewed-on: https://chromium-review.googlesource.com/519144
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476767}
[delete] https://crrev.com/3c6caa287dacd94ee5e64eedaf44ea987f287bf8/third_party/WebKit/LayoutTests/external/wpt/fetch/api/headers/header-values-normalize-expected.txt
[delete] https://crrev.com/3c6caa287dacd94ee5e64eedaf44ea987f287bf8/third_party/WebKit/LayoutTests/external/wpt/fetch/api/headers/headers-normalize-expected.txt
[modify] https://crrev.com/bb671c7cb427aadc0e2e946ff426775610c25f98/third_party/WebKit/LayoutTests/http/tests/fetch/resources/fetch-test-helpers.js
[modify] https://crrev.com/bb671c7cb427aadc0e2e946ff426775610c25f98/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/inject-header-expected.txt
[modify] https://crrev.com/bb671c7cb427aadc0e2e946ff426775610c25f98/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/inject-header.html
[modify] https://crrev.com/bb671c7cb427aadc0e2e946ff426775610c25f98/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
[modify] https://crrev.com/bb671c7cb427aadc0e2e946ff426775610c25f98/third_party/WebKit/Source/modules/fetch/Headers.cpp

Labels: M-61
Status: Fixed (was: Started)

Sign in to add a comment