- Notifications
You must be signed in to change notification settings - Fork329
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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
| v=strings.TrimSpace(v) | ||
| for_,t:=rangestrings.Split(v,",") { | ||
| t=strings.ToLower(t) | ||
| iflower { |
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 aToLower 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 forSec-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 thinkheaderContainsToken should be the one modified to allow optional case insensitivity as that's the function used to checkConnect andUpgrade 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 renameheaderContainsToken toheaderContainsTokenIgnoreCase because this function is only used to deal withConnect andUpgrade 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) WebSocketrejecting connnection because negotiated subprotocol,returned in Sec-WebSocket-Protocol header (loweredbe headerToken() function) would not match one sentby client, in case client specified value with capitalletters.
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) WebSocketrejecting connnection because negotiated subprotocol,returned in Sec-WebSocket-Protocol header (loweredbe headerToken() function) would not match one sentby client, in case client specified value with capitalletters.
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) WebSocketrejecting connnection because negotiated subprotocol,returned in Sec-WebSocket-Protocol header (loweredbe headerToken() function) would not match one sentby client, in case client specified value with capitalletters.
Uh oh!
There was an error while loading.Please reload this page.
WebSocket implementation in chromium rejects
ws connections in case server side responses with
Sec-WebSocket-Protocolheader 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.