Incorrect handling of exceptions in websocket handlers

I've encountered a couple of issues in how Blacksheep handles exceptions during the execution of a WebSocket handler. I'd like to discuss them and propose a solution.

I see two problems:

If an HTTPException is raised before accepting the connection, current behavior is to close the connection, and then try to close it again. This is incorrect and leads to RuntimeError: Unexpected ASGI message 'websocket.close', after sending 'websocket.close'.
If an HTTPException is raised after accepting the connection, Blacksheep closes the connection, sending the HTTP code to the ASGI server. This is incorrect too, and leads to websockets.exceptions.ProtocolError: invalid status code for Uvicorn and Exception: invalid close code 403 (must be 1000 or from [3000, 4999]) for Daphne. Anyway, one should not use HTTP error codes as WebSocket error codes (shame on me I guess 😿 ).

After reading the ASGI spec and comparing various scenarios using Blacksheep, FastAPI , Uvicorn, and Daphne, my thinking is that there's not much sense handling exceptions in WebSocket handlers at all. The ASGI server will do this anyway, adding the appropriate information to the closing message.

Why?

(An excerpt from the ASGI spec about the websocket.close message) If this is sent before the socket is accepted, the server must close the connection with a HTTP 403 error code (Forbidden), and not complete the WebSocket handshake; this may present on some browsers as a different WebSocket error code (such as 1006, Abnormal Closure). If this is sent after the socket is accepted, the server must close the socket with the close code passed in the message (or 1000 if none is specified).

Hovewer, to make Authentication/Authorization work more smoothly, I would propose to handle 401 and 403 errors by simply closing the connection (which in turn will make the ASGI server send 403 to the client, as this errors are raised by the auth framework before the WebSocket handler has a chance to call accept()). Any other exception will be left unhandled, propagating to the ASGI server and leaving a log entry.

Basically, I think that Application._handle_websocket should look like this:

ws = WebSocket(scope, receive, send)
route = self.router.get_ws_match(scope["path"])

if route is None:
    return await ws.close()

ws.route_values = route.values

try:
    return await route.handler(ws)
except HTTPException as exc:
    if exc.status in [401, 403]:
        return await ws.close()
    raise

This is probably not flawless too, but at least that's better than what we have now. This is just my humble perspective on this issue, so I'll be happy to discuss it!

Hi @Klavionik Sorry for replying so late here. I see your point and I like your idea. I will give it a try and check.

PS. I removed the get_ws_match method because I didn't like the inconsistency of having a dedicated method only for WS and also didn't want to add methods for each kind of verb / request.

I tried your suggestion and the integration test checking support for built-in authentication and authorization (test_websocket_auth[websocket-echo-text-http-exp]) fails with an internal server error. I'll take a look at this;

----------------------------------------------------------- Captured stderr call ------------------------------------------------------------
DEBUG:    = connection is CONNECTING
DEBUG:    < GET /websocket-echo-text-http-exp HTTP/1.1
DEBUG:    < host: 127.0.0.1:44556
DEBUG:    < upgrade: websocket
DEBUG:    < connection: Upgrade
DEBUG:    < sec-websocket-key: DUpISl5JgpcIaKd8mGDmdg==
DEBUG:    < sec-websocket-version: 13
DEBUG:    < sec-websocket-extensions: permessage-deflate; client_max_window_bits
DEBUG:    < user-agent: Python/3.11 websockets/11.0.2
ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/uvicorn/protocols/websockets/websockets_impl.py", line 254, in run_asgi
    result = await self.app(self.scope, self.asgi_receive, self.asgi_send)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/server/application.py", line 769, in __call__
    return await self._handle_websocket(scope, receive, send)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/server/application.py", line 735, in _handle_websocket
    return await route.handler(ws)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/middlewares.py", line 6, in middleware_wrapper
    return await handler(request, next_handler)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/server/authentication/__init__.py", line 19, in authentication_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/middlewares.py", line 6, in middleware_wrapper
    return await handler(request, next_handler)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/server/authorization/__init__.py", line 82, in authorization_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/middlewares.py", line 6, in middleware_wrapper
    return await handler(request, next_handler)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/server/compression.py", line 101, in __call__
    response = ensure_response(await handler(request))
                               ^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/blacksheep/server/normalization.py", line 556, in handler
    return ensure_response(await method(request))
                           ^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/itests/app_2.py", line 155, in echo_text_http_exp
    raise BadRequest("Example")
blacksheep.exceptions.BadRequest: Example
DEBUG:    > HTTP/1.1 101 Switching Protocols
DEBUG:    > Upgrade: websocket
DEBUG:    > Connection: Upgrade
DEBUG:    > Sec-WebSocket-Accept: jrES4ownYHGcQRjZGgWKWZeQbF4=
DEBUG:    > Sec-WebSocket-Extensions: permessage-deflate
DEBUG:    > date: Sun, 26 Nov 2023 08:49:27 GMT
DEBUG:    > server: uvicorn
INFO:     connection open
DEBUG:    = connection is OPEN
DEBUG:    = connection is CLOSING
DEBUG:    > CLOSE 1000 (OK) [2 bytes]
DEBUG:    = connection is CLOSED
DEBUG:    ! failing connection with code 1006
DEBUG:    x half-closing TCP connection
DEBUG:    ! failing connection with code 1006
--------------------------------------------------------- Captured stderr teardown ----------------------------------------------------------
ERROR:    closing handshake failed
Traceback (most recent call last):
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 959, in transfer_data
    message = await self.read_message()
              ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 1029, in read_message
    frame = await self.read_data_frame(max_size=self.max_size)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 1104, in read_data_frame
    frame = await self.read_frame(max_size)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 1161, in read_frame
    frame = await Frame.read(
            ^^^^^^^^^^^^^^^^^
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/framing.py", line 68, in read
    data = await reader(2)
           ^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/asyncio/streams.py", line 726, in readexactly
    raise exceptions.IncompleteReadError(incomplete, n)
asyncio.exceptions.IncompleteReadError: 0 bytes read on a total of 2 expected bytes

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/server.py", line 248, in handler
    await self.close()
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 766, in close
    await self.write_close_frame(Close(code, reason))
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 1232, in write_close_frame
    await self.write_frame(True, OP_CLOSE, data, _state=State.CLOSING)
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 1205, in write_frame
    await self.drain()
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 1194, in drain
    await self.ensure_open()
  File "~/projects/github/blacksheep/venv/lib/python3.11/site-packages/websockets/legacy/protocol.py", line 935, in ensure_open
    raise self.connection_closed_exc()
websockets.exceptions.ConnectionClosedError: sent 1000 (OK); no close frame received
INFO:     connection closed
INFO:     Shutting down
INFO:     Waiting for application shutdown.
INFO:     Application shutdown complete.
INFO:     Finished server process [17044]
INFO:     Shutting down
INFO:     Waiting for application shutdown.
INFO:     Application shutdown complete.
INFO:     Finished server process [17043]
INFO:     Shutting down
INFO:     Waiting for application shutdown.
INFO:     Application shutdown complete.
INFO:     Finished server process [17039]

---------- coverage: platform linux, python 3.11.0-final-0 -----------
Coverage HTML written to dir htmlcov

Hey, I'm sorry for the late reply too! I'm quite busy with the new job these days.

I'm afraid to say that the fix included in 2.0.1 doesn't really work. :( Both issues I described in the first message still present and I was able to reproduce them.

1) The issue with closing the WS connection twice left, because there is no return inside the exception handling code. 2) I believe that the server mentioned in the spec is the ASGI server like Uvicorn, not an ASGI application like Blacksheep. We should not call ws.close with an HTTP exception code (like I did in the past and you did in the fix). Both Uvicorn and Daphne are not happy about it.

I see that the code I suggested breaks some integration tests - I believe this is due to 2 reasons.

1) except HTTPException should be replaced with except UnauthorizedError with no check for status code. 2) test_websocket_auth[websocket-echo-text-http-exp] test should probably be changed or deleted.

In short, my idea was to raise on any error and let the ASGI server handle this, except for potential authorization errors coming from the authorization framework (that's why I was trying to check for 401 and 403 statuses, that was a wrong move apparently). This will make the ASGI server return 403 to the client anyway.

@RobertoPrevato If you don't mind, I'll try to implement a PR to support my idea including a test suite in the next few days. Sorry again for keeping you waiting.

@Klavionik I suspected it might not work to resolve the case you opened. I tried your recommendation to simply raise exception and let the ASGI server handle it, and that wasn't working neither because it would result in 500 Internal Server Error in all cases. This is not correct because even before accepting a Web Socket, there are legit use cases for other kinds of responses. Like: the client didn't provide proper input and should receive 400 Bad Request response, etc.

No worries, I also have limited time. I appreciate your help very much. A PR is welcome, just let's make sure that: if the server needs to raise any kind of Exception before accepting the WebSocket request, there is no 500 Internal Server Error response (ideally before the handshake it should support responding to the client with proper status code and not always 403 Forbidden)

I see your point, no worries! :) I'll try to figure something out and then we'll discuss it and iterate until it's done.

For now I managed to take FastAPI, Quart and Litestar and run several tests.

FastAPI raises every exception (both before and after accept) and doesn't even allow the ASGI server to send 403 in response. Litestar silences every exception. It closes the connection normally (making the ASGI server send 403) if the exception is raised before accept and closes the connection with code 4500 if the exception is raised after accept. Quart both raises the exception and closes the connection normally, no matter if it's raised before or after accept.

Don't know how it helps us, just wanted to check with others and find some common ground. Apparently there isn't any at all. :)

Neoteroi / BlackSheep

Incorrect handling of exceptions in websocket handlers #427