Send headers with Sec-WebSocket-* capitalization #71
Conversation
...but don't be picky about which capitalization we receive
|
WDYT to leave old and incorrect(😢) versions for a back compatibility and other incorrect clients? |
|
@cristaloleg I don't know of any servers that require a lowercase S, but I do know of servers (like agar) that require an uppercase S. Sending the uppercase version from the RFC should allow for compatibility with the most websocket servers. If anyone knows of a server that only accepts lowercase s headers, please comment! |
|
Hi @qguv. I think you are talking about a bug inside some particular server implementation. HTTP RFC says this:
Thus it is nevermind |
|
But, on the other hand, the "message headers registry" lists one that you suggested. 🤔 8000 |
|
@gobwas Yes exactly. If you look at the HTTP RFCs, it shouldn't matter. It's a shame that the WebSocket RFC breaks the convention on header normalization set in the HTTP RFC. But because I've seen servers in the wild that enforce the capital S and no servers in the wild that enforce lower-case S without accepting the capitalized version, I think it's a good idea to stay as close to the WebSocket RFC as possible. |
|
Just pushed some updates. I fixed an issue that was breaking existing tests, and I added some new tests to catch issues with capitalization going forward. |
|
I'd also like to point out that both Firefox and Chromium send headers with a capital S: You can test this by running a simple ncat server: and creating a new websocket in an about:blank tab in your browser of choice: You should see the headers in |
http.go
Outdated
| headerSecExtensions = textproto.CanonicalMIMEHeaderKey("Sec-Websocket-Extensions") | ||
| headerSecKey = textproto.CanonicalMIMEHeaderKey("Sec-Websocket-Key") | ||
| headerSecAccept = textproto.CanonicalMIMEHeaderKey("Sec-Websocket-Accept") | ||
| headerHost = "Host" |
There was a problem hiding this comment.
Could you please add the same Canonical version for these headers for better symmetry?
|
Hi @qguv, commented few lines in the diff, other things looks good. |
| } | ||
| if test.err == nil { | ||
| test.res.Header.Set(headerSecAccept, string(makeAccept(test.nonce))) | ||
| test.res.Header[headerSecAccept] = []string{string(makeAccept(test.nonce))} |
There was a problem hiding this comment.
The (http.Header) Set() method calls (textproto.MIMEHeader) Set() which canonicalizes the header key before setting the header. This causes tests to fail because the expected response (forced to lowercase-s headers) does not match the actual response (which now contains uppercase-S headers). We can bypass canonicalization by setting the key directly in the map that the textproto.MIMEHeader is based on.
|
Hi @qguv, sorry for the late reply! |
|
Also, I am about bump the patch version to |
|
Thanks @gobwas! Yes, the merge looks good. |
|
Hey @gobwas, are you still planning to tag a new release? |
|
Hi there! Yes, I am. But currently Im on vacation, sorry. Will do that at Sunday probably. |
|
@qguv done! 🎆 |
According to the RFC, the
Sec-WebSocket-*headers should have a capital S inside the word WebSocket. This is a shame, because most other headers are canonicalized differently.Some websocket servers are particularly picky about the capitalization. The agar.io server (or maybe the Cloudflare servers it's behind) will reject connections with the canonical spelling, giving a 400 in response.
You can try it. This will succeed:
But this will fail, giving a 400 Bad Request:
This patch fixes this: ws will now accept any capitalization it receives, but use the RFC-correct capitalization when sending headers either as a response or as an upgrade request.
Thanks for this library! The zero-copy functionality is really nice for us.