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

Incorrectly returning multiple Sec-WebSocket-Protocol headers #141

Open
eltigerchino opened this issue Feb 20, 2025 · 0 comments · May be fixed by #142
Open

Incorrectly returning multiple Sec-WebSocket-Protocol headers #141

eltigerchino opened this issue Feb 20, 2025 · 0 comments · May be fixed by #142
Labels
bug Something isn't working

Comments

@eltigerchino
Copy link
Contributor

eltigerchino commented Feb 20, 2025

Environment

Node v22.13.1
[email protected]

Reproduction

https://github.com/eltigerchino/crossws-duplicate-headers-repro
Reproduction instructions are included in the repository.

Describe the bug

I'm trying to select a WebSocket protocol during an upgrade request by returning the Sec-WebSocket-Protocol header with a specific value. However, the ws, uNetworking/uWebSockets.js, and Deno libraries, also return this header if the client sends it. This causes the response to be invalid, since only one instance of the protocol header should exist in the response.

handleProtocols takes two arguments:

protocols {Set} The list of WebSocket subprotocols indicated by the client in the Sec-WebSocket-Protocol header.
request {http.IncomingMessage} The client HTTP GET request.
The returned value sets the value of the Sec-WebSocket-Protocol header in the HTTP 101 response. If returned value is false, the header is not added in the response.

If handleProtocols is not set, then the first of the client's requested subprotocols is used.

https://github.com/websockets/ws/blob/master/doc/ws.md#new-websocketserveroptions-callback

Fortunately, this behaviour can be turned off by setting the Node adapter's option serverOptions.handleProtocols. However, it might be better to standardise this behaviour in crossws itself. The other adapters do not automatically return the protocol header. Thus, the upgrade hook can be responsible for selecting the protocol by returning the header or omitting the header.

Alternatively, we could go the opposite direction and make all adapters always return the first protocol the client sends but allow overwriting the header from the upgrade hook.

Additional context

Warning: The server can't send more than one Sec-WebSocket-Protocol header. If the server doesn't want to use any subprotocol, it shouldn't send any Sec-WebSocket-Protocol header. Sending a blank header is incorrect. The client may close the connection if it doesn't get the subprotocol it wants.

https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#subprotocols

Either a single value representing the subprotocol the server is ready to use or null. The value chosen MUST be derived from the client's handshake, specifically by selecting one of the values from the |Sec-WebSocket-Protocol| field that the server is willing to use for this connection (if any). If the client's handshake did not contain such a header field or if the server does not agree to any of the client's requested subprotocols, the only acceptable value is null. The absence of such a field is equivalent to the null value (meaning that if the server does not wish to agree to one of the suggested subprotocols, it MUST NOT send back a |Sec-WebSocket-Protocol| header field in its response). The empty string is not the same as the null value for these purposes and is not a legal value for this field. The ABNF for the value of this header field is (token), where the definitions of constructs and rules are as given in [RFC2616].

https://datatracker.ietf.org/doc/html/rfc6455#section-4.2.2

Logs

WebSocket connection to 'ws://localhost:8000/' failed:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant