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

Verify if is a websocket connection #1443

Closed
wants to merge 2 commits into from

Conversation

MauroAntonino
Copy link

Now, every HTTP request is handled as if it were a WebSocket connection. However, when an HTTP request is made and is expected to close the connection, an error arises because it is not supposed to receive an HTTP connection in the handler.

This PR verifies if it is actually a WebSocket connection before handling it.

@davidawesome02
Copy link

I may be wrong; but according to mozilla the upgrade header can be used for http requests, not just switching to websockets, so it should be explicitly checked for the upgrade to websocket.

This feature seams to be generally unused and depreciated, so in most cases should not matter.

@MauroAntonino MauroAntonino force-pushed the main branch 4 times, most recently from 41ca85e to cd44f73 Compare February 26, 2024 12:27
@MauroAntonino
Copy link
Author

Yes, you are right. It must check if the header has the websocket string explicitly. I made a commit to fix right now.

@aaugustin aaugustin force-pushed the main branch 2 times, most recently from dba135f to e10eeba Compare July 21, 2024 13:32
@aaugustin
Copy link
Member

Can you provide more information about what you're doing, what you're expecting, and what you're getting?

when an HTTP request is made and is expected to close the connection

In this case, the call to connection.handshake(...) — ten lines above the code that you changed — is expected to raise an exception. As a consequence, that code shouldn't be reached.

If this doesn't work as expected, the fix consists in making connection.handshake(...) raise an exception, because that's the method in charge of telling if this is a WebSocket connection or not.

We mustn't reimplement some of the protocol logic outside of the Sans-I/O layer.

@aaugustin
Copy link
Member

(And sorry for the late answer — I haven't found time to work on this project in the recent months.)

@aaugustin
Copy link
Member

Superseded by #1500.

@aaugustin aaugustin closed this Sep 10, 2024
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