Skip to content

feat: add sec-websocket-version to response headers when request sec-websocket-version is invalid #2291

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

Merged

Conversation

samuel871211
Copy link
Contributor

@samuel871211 samuel871211 commented Jun 28, 2025

While test locally with postman, send invalid sec-websocket-version, I found that the server doesn't include sec-websocket-version: 13,8 in response headers
no-sec-websocket-version-header-present

  1. Based on MDN Document
    https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Sec-WebSocket-Version
If the server doesn't support the version, or any header in the handshake is not understood or has an incorrect value, the server should send a response with status [400 Bad Request](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/400) and immediately close the socket. It should also include Sec-WebSocket-Version in the 400 response, listing the versions that it does support.
  1. Based on RFC6455
    https://datatracker.ietf.org/doc/html/rfc6455#section-4.4
The following example demonstrates version negotiation described above:

      GET /chat HTTP/1.1
      Host: server.example.com
      Upgrade: websocket
      Connection: Upgrade
      ...
      Sec-WebSocket-Version: 25

   The response from the server might look as follows:

      HTTP/1.1 400 Bad Request
      ...
      Sec-WebSocket-Version: 13, 8, 7

   Note that the last response from the server might also look like:

      HTTP/1.1 400 Bad Request
      ...
      Sec-WebSocket-Version: 13
      Sec-WebSocket-Version: 8, 7

   The client now repeats the handshake that conforms to version 13:

      GET /chat HTTP/1.1
      Host: server.example.com
      Upgrade: websocket
      Connection: Upgrade
      ...
      Sec-WebSocket-Version: 13

This PR add sec-websocket-version to response headers when request sec-websocket-version is invalid.

  • npm run lint
  • npm run test
  • npm run integration

@lpinca
Copy link
Member

lpinca commented Jun 28, 2025

FWIW, given that

The request MUST include a header field with the name
|Sec-WebSocket-Version|.  The value of this header field MUST be
13.

I think it makes sense to drop support for version 8, but it is a breaking change.

@lpinca lpinca merged commit 33f5dba into websockets:master Jun 28, 2025
62 checks passed
@lpinca
Copy link
Member

lpinca commented Jun 28, 2025

Thank you.

@samuel871211 samuel871211 deleted the add-sec-websocket-version-when-400 branch June 28, 2025 13:01
@samuel871211
Copy link
Contributor Author

FWIW, given that

The request MUST include a header field with the name
|Sec-WebSocket-Version|.  The value of this header field MUST be
13.

I think it makes sense to drop support for version 8, but it is a breaking change.

I thought so. Do you plan to drop support for version 8?

@lpinca
Copy link
Member

lpinca commented Jun 28, 2025

Maybe, but it is not a priority. Supporting version 8 does not cost anything.

$ git grep '=== 8'
lib/websocket-server.js:          req.headers[`${version === 8 ? 'sec-websocket-origin' : 'origin'}`],

@samuel871211
Copy link
Contributor Author

Agree, since it does not dramatically reduce the bundle size of this project, and may require a major version bump, I don't see the benefit of it.

@lpinca
Copy link
Member

lpinca commented Jun 28, 2025

The only valid reason is strict compliance with RFC 6455, but it does not worth a major version bump per se. It should be added to the list of breaking changes for the next major version.

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.

2 participants