Skip to content

Verify if is a websocket connection - #1443

Closed
MauroAntonino wants to merge 2 commits into
python-websockets:mainfrom
MauroAntonino:main
Closed

Verify if is a websocket connection#1443
MauroAntonino wants to merge 2 commits into
python-websockets:mainfrom
MauroAntonino:main

Conversation

@MauroAntonino

Copy link
Copy Markdown

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
Copy Markdown

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
MauroAntoninoforce-pushed the main branch 4 times, most recently from 41ca85e to cd44f73CompareFebruary 26, 2024 12:27
@MauroAntonino

Copy link
Copy Markdown
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
aaugustinforce-pushed the main branch 2 times, most recently from dba135f to e10eebaCompareJuly 21, 2024 13:32
@aaugustin

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@aaugustin

Copy link
Copy Markdown
Member

Superseded by #1500.

Sign up for freeto 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

@MauroAntonino@davidawesome02@aaugustin