Bug 157157

Summary: Allow non-standard HTTP headers in WebSocket handshake
Product: WebKit Reporter: John Wilander <wilander>
Component: WebCore Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, bfulgham, commit-queue, wilander
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155602    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Description John Wilander 2016-04-28 17:04:46 PDT 
Several reports of regressions in the wild mean we have to relax our header policy in the WebSocket handshake.
Comment 1 John Wilander 2016-04-28 17:12:25 PDT 
Created attachment 277666 [details]
Patch
Comment 2 Brent Fulgham 2016-04-28 17:15:23 PDT 
Comment on attachment 277666 [details]
Patch

r=me.
Comment 3 WebKit Commit Bot 2016-04-28 18:07:09 PDT 
Comment on attachment 277666 [details]
Patch

Clearing flags on attachment: 277666

Committed r200219: <http://trac.webkit.org/changeset/200219>
Comment 4 WebKit Commit Bot 2016-04-28 18:07:13 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 5 Alexey Proskuryakov 2016-04-28 19:44:07 PDT 
Comment on attachment 277666 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests since https://bugs.webkit.org/show_bug.cgi?id=157095
> +        tests that non-standard headers are allowed.

Was there a concrete motivation for further behavior change? If that's the case, then we should have a test that verifies that the specific new case keeps working.

Even if not, every behavior change should have a test, so adding a test for e.g. "Foo: bar" would be appropriate.
Comment 6 Brent Fulgham 2016-04-28 20:25:59 PDT 
Comment on attachment 277666 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        tests that non-standard headers are allowed.
> 
> Was there a concrete motivation for further behavior change? If that's the case, then we should have a test that verifies that the specific new case keeps working.
> 
> Even if not, every behavior change should have a test, so adding a test for e.g. "Foo: bar" would be appropriate.

He already created tests for the issues we knew about (Sec-WebSocket-Location, WebSocket-location, etc.), but we have since found more instances of "custom" headers being used by developers.

I think it's fine to remove the tighter control on the headers, but retain the new "non-standard" tests that he added as a check that we don't start blocking them again.

I don't think further tests are warranted for this specific change.
Comment 7 Alexey Proskuryakov 2016-04-29 10:15:21 PDT 
If our reading of the spec is that any headers are allowed, it's still somewhat valuable to test for headers that have never been called out in any version of the spec.