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

Fix unhandled error on HTTP2 upgrade #658

Closed
wants to merge 1 commit into from
Closed

Fix unhandled error on HTTP2 upgrade #658

wants to merge 1 commit into from

Conversation

jonathanneve
Copy link
Contributor

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

If you send an HTTP2 upgrade request to the server, we catch the upgrade event and close the socket when it turns out not to be a websocket upgrade request. In some cases, this leads to an error event, which in turn causes the server to crash.

I created a bare bones project that demonstrates this problem: https://github.com/jonathanneve/socketiotest
Run the server locally (https://github.com/jonathanneve/socketiotest/blob/main/index.js) and then test it using the included python test case (https://github.com/jonathanneve/socketiotest/blob/main/test.py). The server should crash.

This is the same issue as described here: socketio/socket.io#3564

New behaviour

The error event is caught and (in debug mode) logged to the console. This prevents the error from bubbling up and causing the server to crash.

Other information (e.g. related issues)

Note that in my situation, the intention is not actually to use HTTP2: I came across the issue by running a vulnerability scanner (Nessus) against my app, which caused it to crash. On closer examination, I found that Nessus was sending an HTTP2 upgrade request. The goal of this PR is not to handle HTTP2, but simply to harden the app, in that a simple upgrade request should not cause a crash.

Note also that I was not able to reproduce the issue using a Node client, as I have less control over the socket options in Node, which is why I used Python for the test case. Using the Nessus scanner (as opposed to my test case), I found that the app crashes only with certain versions of Node (older than v16.16.0), which suggests that the error is contingent on the exact behavior of the socket, which presumably has changed in different Node versions. Either way though, this PR addresses the error in all cases, since it catches any error that occurs while closing the socket.

darrachequesne pushed a commit that referenced this pull request Nov 20, 2022
Before this change, receiving an HTTP2 upgrade would make the server
crash:

> Error: read ECONNRESET
>    at TCP.onStreamRead (node:internal/stream_base_commons:217:20) {
>  errno: -104,
>  code: 'ECONNRESET',
>  syscall: 'read'
> }

This can be reproduced with Node.js v14.15.3, v16.18.1 and v18.12.1.
@darrachequesne
Copy link
Member

darrachequesne commented Nov 20, 2022

Merged as 425e833 and released as 6.2.1. Thanks a lot for the reproducible example 👍

@jonathanneve
Copy link
Contributor Author

Thanks for the quick release! It would be great to also make a new socket.io release to include this fix.

@darrachequesne
Copy link
Member

@jonathanneve here we go: socket.io@4.5.4

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.

3 participants