-
Notifications
You must be signed in to change notification settings - Fork 313
Preserve client case for selected subprotocol #273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful catch, thank you @egorbunov <3
accept.go
Outdated
key = textproto.CanonicalMIMEHeaderKey(key) | ||
var tokens []string | ||
for _, v := range h[key] { | ||
v = strings.TrimSpace(v) | ||
for _, t := range strings.Split(v, ",") { | ||
t = strings.ToLower(t) | ||
if lower { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we shouldn't do a ToLower
here at all and instead rely on the caller handling case correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this code is also going to cause trouble for Sec-WebSocket-Extensions
. In general, I don't think HTTP header values are case insensitive so I think I made a mistake here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think headerContainsToken
should be the one modified to allow optional case insensitivity as that's the function used to check Connect
and Upgrade
whose values are case insensitive. And then this function should be changed to not modify case at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote the fix. Decided to rename headerContainsToken
to headerContainsTokenIgnoreCase
because this function is only used to deal with Connect
and Upgrade
headers.
HTTP header values, as opposed to header keys, are case sensitive, but implementation of headerTokens() before that patch would return lowered values always. This old behavior could lead to chromium (v87) WebSocket rejecting connnection because negotiated subprotocol, returned in Sec-WebSocket-Protocol header (lowered be headerToken() function) would not match one sent by client, in case client specified value with capital letters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely!! Thanks again @egorbunov 🚀
HTTP header values, as opposed to header keys, are case sensitive, but implementation of headerTokens() before this patch would return lowered values always. This old behavior could lead to chromium (v87) WebSocket rejecting connnection because negotiated subprotocol, returned in Sec-WebSocket-Protocol header (lowered be headerToken() function) would not match one sent by client, in case client specified value with capital letters.
HTTP header values, as opposed to header keys, are case sensitive, but implementation of headerTokens() before this patch would return lowered values always. This old behavior could lead to chromium (v87) WebSocket rejecting connnection because negotiated subprotocol, returned in Sec-WebSocket-Protocol header (lowered be headerToken() function) would not match one sent by client, in case client specified value with capital letters.
WebSocket implementation in chromium rejects
ws connections in case server side responses with
Sec-WebSocket-Protocol
header which value doesnot match one of the values sent in the same header by client
In case you use subprotocols to send case-sensitive text
(for example, auth tokens) you end up with inability
to establish websocket connection from chromium.
How to reproduce:
Initialize websocket like this:
After that try to establish connection from javascript in chromium (my current version is
87.0.4280.88
):You will get next error:
So this PR fixes that behavior.