Bug 167112 - HTTP Header values validation is too strict
Summary: HTTP Header values validation is too strict
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-16 23:57 PST by Anne van Kesteren
Modified: 2018-06-06 21:22 PDT (History)
12 users (show)

See Also:


Attachments
Patch (17.04 KB, patch)
2017-01-18 11:24 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Adding missing expectations (42.47 KB, patch)
2017-01-18 11:27 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (940.48 KB, application/zip)
2017-01-18 12:29 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (767.50 KB, application/zip)
2017-01-18 12:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.52 MB, application/zip)
2017-01-18 12:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (3.73 MB, application/zip)
2017-01-18 12:44 PST, Build Bot
no flags Details
Rebasing bogus name test (44.02 KB, patch)
2017-01-19 09:01 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2018-05-17 13:13 PDT, youenn fablet
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.85 MB, application/zip)
2018-05-17 14:11 PDT, Build Bot
no flags Details
Patch (12.91 KB, patch)
2018-05-17 14:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (13.55 KB, patch)
2018-06-06 20:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anne van Kesteren 2017-01-16 23:57:20 PST
See tests added in https://github.com/w3c/web-platform-tests/pull/4525.
Comment 1 John Wilander 2017-01-17 16:53:01 PST
I'd like to have information here in the bug of what we are (too) strict about, what should be relaxed, and why. Thanks!
Comment 2 youenn fablet 2017-01-17 17:45:43 PST
(In reply to comment #1)
> I'd like to have information here in the bug of what we are (too) strict
> about, what should be relaxed, and why. Thanks!

I introduced a while back enforcement of the ABNF for header values.
This is now obsolete and no other browser is implementing it.
Fetch API is temporarily defining validation rules for header values.
I guess that once HTTPBis WG will carry on the changes to the corresponding RFC, fetch spec will just refer to it.

The restriction I added is roughly that all characters below 0x20 (except for tab) would lead to make a header value invalid.

The proposed validation rule is defined in https://fetch.spec.whatwg.org/#concept-header
Comment 3 youenn fablet 2017-01-18 11:24:02 PST
Created attachment 299159 [details]
Patch
Comment 4 youenn fablet 2017-01-18 11:27:36 PST
Created attachment 299160 [details]
Adding missing expectations
Comment 5 Build Bot 2017-01-18 12:29:39 PST
Comment on attachment 299160 [details]
Adding missing expectations

Attachment 299160 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2910917

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-name.htm
Comment 6 Build Bot 2017-01-18 12:29:42 PST
Created attachment 299165 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-01-18 12:30:12 PST
Comment on attachment 299160 [details]
Adding missing expectations

Attachment 299160 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2910913

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-name.htm
Comment 8 Build Bot 2017-01-18 12:30:16 PST
Created attachment 299166 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-01-18 12:37:45 PST
Comment on attachment 299160 [details]
Adding missing expectations

Attachment 299160 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2910918

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-name.htm
Comment 10 Build Bot 2017-01-18 12:37:49 PST
Created attachment 299167 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-01-18 12:44:25 PST
Comment on attachment 299160 [details]
Adding missing expectations

Attachment 299160 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2910921

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-name.htm
Comment 12 Build Bot 2017-01-18 12:44:29 PST
Created attachment 299168 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 13 youenn fablet 2017-01-19 09:01:27 PST
Created attachment 299246 [details]
Rebasing bogus name test
Comment 14 John Wilander 2017-02-09 13:42:13 PST
The relaxation of isValidHTTPHeaderValue() affects XMLHttpRequest too. Are we expected to change legacy APIs with Fetch changes? Are we currently breaking things with our XHR behavior?
Comment 15 youenn fablet 2017-02-09 21:03:52 PST
(In reply to comment #14)
> The relaxation of isValidHTTPHeaderValue() affects XMLHttpRequest too. Are
> we expected to change legacy APIs with Fetch changes? Are we currently
> breaking things with our XHR behavior?

AFAIAK, we are not breaking things but we are not consistent with other browsers nor aligned anymore with the specs.

XHR is not added any feature but is still evolving, mainly because of it being defined in terms of fetch.
Comment 16 youenn fablet 2018-05-17 13:13:36 PDT
Created attachment 340629 [details]
Patch
Comment 17 Build Bot 2018-05-17 14:11:31 PDT
Comment on attachment 340629 [details]
Patch

Attachment 340629 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7714170

New failing tests:
http/tests/xmlhttprequest/set-bad-headervalue.html
Comment 18 Build Bot 2018-05-17 14:11:32 PDT
Created attachment 340639 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 19 youenn fablet 2018-05-17 14:29:27 PDT
Created attachment 340643 [details]
Patch
Comment 20 Alex Christensen 2018-05-17 16:25:27 PDT
Comment on attachment 340643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340643&action=review

This makes us match Chrome and Firefox.  r=me

> Source/WebCore/platform/network/HTTPParsers.cpp:113
> +// See https://fetch.spec.whatwg.org/#concept-header until RFC 7230 gets fixed.

I'm not sure "until RFC 7230 gets fixed" is the right thing to say here.
Comment 21 youenn fablet 2018-06-06 20:29:49 PDT
Created attachment 342114 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2018-06-06 21:20:47 PDT
The commit-queue encountered the following flaky tests while processing attachment 342114 [details]:

css3/filters/crash-filter-animation-invalid-url.html bug 186381 (authors: jhoneycutt@apple.com and simon.fraser@apple.com)
The commit-queue is continuing to process your patch.
Comment 23 WebKit Commit Bot 2018-06-06 21:21:28 PDT
Comment on attachment 342114 [details]
Patch for landing

Clearing flags on attachment: 342114

Committed r232572: <https://trac.webkit.org/changeset/232572>
Comment 24 WebKit Commit Bot 2018-06-06 21:21:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2018-06-06 21:22:33 PDT
<rdar://problem/40880447>