Header value normalization not happening properly |
|||||
Issue descriptionSee https://github.com/w3c/web-platform-tests/issues/4525 for tests.
,
Jan 20 2017
,
May 31 2017
,
Jun 1 2017
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.
,
Jun 1 2017
,
Jun 1 2017
Oh, ... it looks the UMA has been wrong from the beginning. Please wait.
,
Jun 1 2017
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.
,
Jun 1 2017
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
,
Jun 1 2017
#7: Yeah, I was just confused. You can clean up SetRequestHeaderInternal(). Please see my reply to blink-dev@ for details. Thanks!
,
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
,
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
,
Jun 6 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dtapu...@chromium.org
, Jan 17 2017