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 WS exception handling #446

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

Klavionik
Copy link
Contributor

@RobertoPrevato After some considerations, I decided to try this approach. Please, let me know, what you think about it. Is it good enough to you?

  1. No matching WebSocket path? Close the connection, the ASGI server will respond with 403.
  2. An exception raised during handshake? Close the connection, the ASGI server will respond with 403.
  3. An exception raised after handshake? Close the connection with code 1011 (which seems to be equivalent to Internal Server Error, see https://www.rfc-editor.org/rfc/rfc6455.html#section-7.4.1). We return the error reason too, but we take care to truncate it to prevent control frame too long errors.

We had a discussion about responding with a more precise status code if the connection was closed before accept, but it seems to be impossible in the ASGI framework. We are not in control of the initial HTTP request, so to speak. We cannot make the protocol server reject the connection with a status code different to 403, and if we try to pass an HTTP status code as a WebSocket close code, it just won't work. Clients and servers like Uvicorn/Daphne will only see this as an invalid WebSocket code.

@Klavionik Klavionik force-pushed the fix-ws-exception-handling branch from 1472160 to e8216c6 Compare December 14, 2023 21:46
@Klavionik Klavionik changed the title Fix ws exception handling Fix WS exception handling Dec 14, 2023
@Klavionik Klavionik force-pushed the fix-ws-exception-handling branch 2 times, most recently from c8545fb to fe7a3fe Compare December 14, 2023 22:12
@Klavionik Klavionik force-pushed the fix-ws-exception-handling branch from fe7a3fe to c4056c1 Compare December 14, 2023 22:13
@Klavionik Klavionik force-pushed the fix-ws-exception-handling branch from c4056c1 to 1bcd31d Compare December 14, 2023 22:17
@RobertoPrevato
Copy link
Member

Hi @Klavionik
Thank You for helping with this! I appreciate very much your contributions. I am too tired now to review, but I can do it tomorrow. :)

@RobertoPrevato
Copy link
Member

@Klavionik I reviewed and tested your changes. I think what you did is good, and I thank you for investigating and sharing how various ASGI servers handle this case. From my point of view the ASGI specification here doesn't make any sense.
Essentially if a WebSocket handler needs to return a Bad Request response, or an Unauthorized response with error details before the handshake even completes (like Access Token expired, input parameter is wrong, etc) - it is not possible to provide a user friendly error message to the client. Always a dull 403 Forbidden error without details.

At the very least, I added a logging.exception line to your branch, to get some information about the Exception in the application logs:

image

@RobertoPrevato RobertoPrevato merged commit 6936f1f into Neoteroi:main Dec 15, 2023
12 checks passed
@Klavionik
Copy link
Contributor Author

Glad to help, as always! I wonder what reasons were behind the decision to make ASGI servers always respond 403.

I think you can close #427 (at least for now 😄).

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.

2 participants