Skip to content
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

Preserve client case for selected subprotocol #273

Merged
merged 1 commit into from Dec 23, 2020

Conversation

egorbunov
Copy link
Contributor

@egorbunov egorbunov commented Dec 21, 2020

WebSocket implementation in chromium rejects
ws connections in case server side responses with
Sec-WebSocket-Protocol header which value does
not 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:

websocketOptions := &websocket.AcceptOptions{}
if subProtocol := r.Header.Get("Sec-Websocket-Protocol"); subProtocol != "" {
	websocketOptions.Subprotocols = []string{subProtocol}
}
...
ws, err := websocket.Accept(w, r, websocketOptions)
...

After that try to establish connection from javascript in chromium (my current version is 87.0.4280.88):

var ws = new WebSocket(url, "ToKeN")

You will get next error:

WebSocket connection to '...' failed: Error during WebSocket handshake: 'Sec-WebSocket-Protocol' header value 'token' in response does not match any of sent values

So this PR fixes that behavior.

Copy link
Owner

@nhooyr nhooyr left a 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 {
Copy link
Owner

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?

Copy link
Owner

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.

Copy link
Owner

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.

Copy link
Contributor Author

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.
Copy link
Owner

@nhooyr nhooyr left a 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 🚀

@nhooyr nhooyr merged commit e4c3b0f into nhooyr:master Dec 23, 2020
nhooyr pushed a commit that referenced this pull request Jan 9, 2021
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.
nhooyr added a commit that referenced this pull request Jan 9, 2021
nhooyr added a commit that referenced this pull request Jan 9, 2021
There were a few PRs merged into the master branch that were then not
merged into the dev branch. This branch merges those changes in cleanly.

- #261
- #266
- #273
@nhooyr nhooyr mentioned this pull request Jan 9, 2021
nhooyr added a commit that referenced this pull request Jan 9, 2021
There were a few PRs merged into the master branch that were then not
merged into the dev branch. This branch merges those changes in cleanly.

- #261
- #266
- #273
EdgarBarrantes pushed a commit to EdgarBarrantes/websocket that referenced this pull request Aug 4, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants