Bug 157157 - Allow non-standard HTTP headers in WebSocket handshake
Summary: Allow non-standard HTTP headers in WebSocket handshake
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords:
Depends on: 155602
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-28 17:04 PDT by John Wilander
Modified: 2016-04-29 10:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.40 KB, patch)
2016-04-28 17:12 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.