cheahjs commented 1 month ago

Describe the bug When configured to upgrade from polling to websockets, but the server is incapable of a websocket connection, the upgrade request (eg GET /socket.io/?EIO=4&transport=websocket&sid=OOOgsMLnIxkuk9GMAABb) is treated as a long-polling request, which consumes server-emitted events in the queue, but never reaches clients because clients such as browsers would drop the response body since it's not valid websocket.

With the javascript client, this can cause race conditions where the websocket upgrade request consumes the CONNECT message, and the client breaks as a result.

To Reproduce

Here is an example for reproducing this behaviour:

server.py

```python import socketio import uvicorn sio = socketio.AsyncServer( async_mode="asgi", logger=True, engineio_logger=True, always_connect=True, cors_allowed_origins="*", ) app = socketio.ASGIApp( sio, static_files={ "/": "index.html", }, ) @sio.on async def connect(sid, environ): print(f"Connected: {sid}") uvicorn.run(app, port=8000) ```

index.html - Javascript client to show that it races and occasionally never connects.

```html Socket.IO test

Socket.IO test

Check the console logs

```

nginx.conf for replicating a reverse proxy that is websocket-incapable (start with `nginx -c $(pwd)/nginx.conf -g 'daemon off;'`)

``` events { worker_connections 1024; } error_log /dev/stderr; http { server { access_log /dev/stdout; listen 9000; server_name localhost; location / { proxy_pass http://localhost:8000; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; } } } ```

upgrade.sh script to show that the websocket upgrade is treated as long-polling and consumes events

```bash #!/usr/bin/env bash echo "Starting handshake" # Extract "sid":"..." from the response sid=$(curl --silent 'http://localhost:9000/socket.io/?EIO=4&transport=polling' | grep -o '"sid":"[^"]*"' | cut -d'"' -f4) echo "sid: $sid" echo "" echo "Sending CONNECT" curl -X POST 'http://localhost:9000/socket.io/?EIO=4&transport=polling&sid='"$sid" -d '40' & echo "" echo "Attempting to consume any events via polling prior to upgrade - racing" # Run in the background to race with the upgrade curl --silent 'http://localhost:9000/socket.io/?EIO=4&transport=polling&sid='"$sid" & echo "" echo "Attempting to upgrade" echo "" # Use raw netcat to see responses ( cat <

Expected behavior A websocket upgrade request without the correct Upgrade or Connection headers is treated as an invalid upgrade request instead of a long-polling request.

Logs

Logs showing websocket request consuming events:

X21WGdx4LOZ5o2IaAAAA: Sending packet OPEN data {'sid': 'X21WGdx4LOZ5o2IaAAAA', 'upgrades': ['websocket'], 'pingTimeout': 20000, 'pingInterval': 25000}
INFO:     None:0 - "GET /socket.io/?EIO=4&transport=polling HTTP/1.0" 200 OK
X21WGdx4LOZ5o2IaAAAA: Received packet MESSAGE data 0
X21WGdx4LOZ5o2IaAAAA: Sending packet MESSAGE data 0{"sid":"eFny2zquOZD4tST1AAAB"}
INFO:     None:0 - "POST /socket.io/?EIO=4&transport=polling&sid=X21WGdx4LOZ5o2IaAAAA HTTP/1.0" 200 OK
INFO:     None:0 - "GET /socket.io/?EIO=4&transport=websocket&sid=X21WGdx4LOZ5o2IaAAAA HTTP/1.0" 200 OK
X21WGdx4LOZ5o2IaAAAA: Sending packet PING data None
INFO:     None:0 - "GET /socket.io/?EIO=4&transport=polling&sid=X21WGdx4LOZ5o2IaAAAA HTTP/1.0" 200 OK

Starting handshake
sid: X21WGdx4LOZ5o2IaAAAA

Sending CONNECT

Attempting to consume any events via polling prior to upgrade - racing

Attempting to upgrade

OKHTTP/1.1 200 OK
Server: nginx/1.27.1
Date: Sun, 08 Sep 2024 09:50:38 GMT
Content-Type: text/plain; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
Access-Control-Allow-Credentials: true

20
40{"sid":"eFny2zquOZD4tST1AAAB"}
0

Attempting to consume any events via polling after upgrade

And logs showing the other side of the race, where long-polling gets the CONNECT request:

Starting handshake
sid: SmD5EdXgFXvdpkXKAAAE

Sending CONNECT

Attempting to consume any events via polling prior to upgrade - racing

Attempting to upgrade

OK40{"sid":"0q1gVOXl-rq1-TDLAAAF"}
Attempting to consume any events via polling after upgrade

cheahjs commented 1 month ago

Code flow (for the async server):

Existing session GET request:

https://github.com/miguelgrinberg/python-engineio/blob/629ee17ae5e50d5b6a382279b9659235b08a5e8c/src/engineio/async_server.py#L281-L288

Socket checks for Upgrade and Connection: upgrade headers to perform an upgrade, but falls back to assuming it's a long-polling request regardless of the transport query parameter.

https://github.com/miguelgrinberg/python-engineio/blob/629ee17ae5e50d5b6a382279b9659235b08a5e8c/src/engineio/async_socket.py#L80-L95

This is handled correctly in the case of attempting a direct websocket connection without upgrading an existing session (`transport == upgrade_header == 'websocket' check):

https://github.com/miguelgrinberg/python-engineio/blob/629ee17ae5e50d5b6a382279b9659235b08a5e8c/src/engineio/async_server.py#L268-L280

miguelgrinberg commented 1 month ago

@cheahjs if your server is not going to accept WebSocket upgrade requests then I suggest you add the allow_upgrades=False option to avoid the unnecessary WebSocket connection failure.

miguelgrinberg / python-engineio

Broken websocket upgrade requests treated as long-polling request, consuming events #367

Socket.IO test