-
Notifications
You must be signed in to change notification settings - Fork 147
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
Handler subprotocol method fails to detect empty list #442
Comments
Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗 |
- Modify tests to use headers - Remove SubprotocolWebSocket - Add test for empty and no subprotocols - Fix jupyterhub#442 - Fix jupyterhub#445
- Modify tests to use headers - Remove SubprotocolWebSocket - Add test for empty and no subprotocols - Fix jupyterhub#442 - Fix jupyterhub#445
Bug description
Hello JSP developers!
My colleague @zoghbi-a and I are attempting to proxy socket.io using JSP and have run into a subprotocol issue that I've started to track down. Please note I am coming as a JSP user with not much networking experience. I'd like to open a discussion about this issue and would be happy to be told I'm just not using JSP properly and corrected. I also didn't want to open a PR in the event I'm misunderstanding something. Thank you for your patience!
Effectively, we're receiving strange headers on our socket.io server that are resulting in a HTTP 400 Bad Request. Tracking down the HTTP response being sent by the proxy, I noticed the following fields being sent in the HTTP header:
Specifically, I noticed that last line:
As I understand it, when a websocket protocol is not specified, the
Sec-Websocket-Protocol
is supposed to be omitted, rather than sent without a value.The following is my best attempt to understand how this condition is arising (all links are permalinked to the current
main
commit:a65437a
):The HTTP 400 error is originating from the following HTTPRequest in JSP in attempting to start a websocket connection:
jupyter-server-proxy/jupyter_server_proxy/handlers.py
Lines 492 to 497 in a65437a
The cause is the value of
self.subprotocols
. It's receiving an empty list[]
. This empty list is originating from:https://github.com/tornadoweb/tornado/blob/65a9e48f8ce645f104e3e0aa772222e70b0376d9/tornado/websocket.py#L902
most likely due to a subprotocol header not being provided on our end. This harkens back to another user's issue: #47
Following the trace, the JSP handler's
select_subprotocol
appears to be processing the empty list properly:jupyter-server-proxy/jupyter_server_proxy/handlers.py
Lines 533 to 539 in a65437a
by properly falling back on Tornado's WebSocketHandler, which properly returns
None
:https://github.com/tornadoweb/tornado/blob/65a9e48f8ce645f104e3e0aa772222e70b0376d9/tornado/websocket.py#L337-L360
However,
select_subprotocol
also caches the provided value:jupyter-server-proxy/jupyter_server_proxy/handlers.py
Line 535 in a65437a
which is what is used websocket connection method:
jupyter-server-proxy/jupyter_server_proxy/handlers.py
Line 496 in a65437a
thereby ignoring the "sanitized" subprotocol value provided by Tornado's WebSocketHandler.
TLDR solution:
I feel that either the
jupyter_server_proxy.handlers.ProxyHandler.proxy_open
should be directly using the sanitized result ofjupyter_server_proxy.handlers.ProxyHandler.select_subprotocol
:or,
jupyter_server_proxy.handlers.ProxyHandler.select_subprotocol
should instead use the Tornado WebSocketHandler's cached value of the final selected subprotocol: https://github.com/tornadoweb/tornado/blob/65a9e48f8ce645f104e3e0aa772222e70b0376d9/tornado/websocket.py#L903Please let me know your thoughts! I appreciate your time and effort on this project :)
The text was updated successfully, but these errors were encountered: