Skip to content

Commit

Permalink
Keep proxying all requested subprotocols
Browse files Browse the repository at this point in the history
When jupyter-server-proxy proxies websockets, its finalizes the
websocket handshake between client/proxy before it initiates the
proxy/server websocket handshake.

In 4.1.1 the thinking was that it was a better compromise to not forward
all subprotocol choices in the proxy/server handshake if we had
prematurely picked a single choice in the client/proxy handshake. This
turns out to have introduced a regression though, as at least bokeh
had been using secondary subprotocol choices to pass other information
such as base64 encoded JSON with keys like `session_id`, `session_expiry`
and `__bk__zlib_`.

This commit makes sure we keep passing all requested subprotocols, even
though we have prematurely picked a specific ahead of time - which is a
bug tracked in
jupyterhub#459.
  • Loading branch information
consideRatio committed Mar 13, 2024
1 parent 81c483b commit f9d6fcd
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 21 deletions.
32 changes: 17 additions & 15 deletions jupyter_server_proxy/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ def __init__(self, *args, **kwargs):
"rewrite_response",
tuple(),
)
self._requested_subprotocols = None
super().__init__(*args, **kwargs)

# Support/use jupyter_server config arguments allow_origin and allow_origin_pat
Expand Down Expand Up @@ -520,27 +521,21 @@ async def start_websocket_connection():
self.log.info(f"Trying to establish websocket connection to {client_uri}")
self._record_activity()
request = httpclient.HTTPRequest(url=client_uri, headers=headers)
subprotocols = (
[self.selected_subprotocol] if self.selected_subprotocol else None
)
self.ws = await pingable_ws_connect(
request=request,
on_message_callback=message_cb,
on_ping_callback=ping_cb,
subprotocols=subprotocols,
subprotocols=self._requested_subprotocols,
resolver=resolver,
)
self._record_activity()
self.log.info(f"Websocket connection established to {client_uri}")
if (
subprotocols
and self.ws.selected_subprotocol != self.selected_subprotocol
):
if self.ws.selected_subprotocol != self.selected_subprotocol:
self.log.warn(
f"Websocket subprotocol between proxy/server ({self.ws.selected_subprotocol}) "
f"became different than for client/proxy ({self.selected_subprotocol}) "
"due to https://github.com/jupyterhub/jupyter-server-proxy/issues/459. "
f"Requested subprotocols were {subprotocols}."
f"Requested subprotocols were {self._requested_subprotocols}."
)

# Wait for the WebSocket to be connected before resolving.
Expand Down Expand Up @@ -578,20 +573,27 @@ def select_subprotocol(self, subprotocols):
"""
Select a single Sec-WebSocket-Protocol during handshake.
Note that this subprotocol selection should really be delegated to the
server we proxy to, but we don't! For this to happen, we would need to
delay accepting the handshake with the client until we have successfully
handshaked with the server. This issue is tracked via
https://github.com/jupyterhub/jupyter-server-proxy/issues/459.
Overrides `tornado.websocket.WebSocketHandler.select_subprotocol` that
includes an informative docstring:
https://github.com/tornadoweb/tornado/blob/v6.4.0/tornado/websocket.py#L337-L360.
"""
# Stash all requested subprotocols to be re-used as requested
# subprotocols in the proxy/server handshake to be performed later. At
# least bokeh has used additional subprotocols to pass credentials,
# making this a required workaround for now.
#
self._requested_subprotocols = subprotocols if subprotocols else None

if subprotocols:
self.log.debug(
f"Client sent subprotocols: {subprotocols}, selecting the first"
)
# FIXME: Subprotocol selection should be delegated to the server we
# proxy to, but we don't! For this to happen, we would need
# to delay accepting the handshake with the client until we
# have successfully handshaked with the server. This issue is
# tracked in https://github.com/jupyterhub/jupyter-server-proxy/issues/459.
#
return subprotocols[0]
return None

Expand Down
11 changes: 5 additions & 6 deletions tests/test_proxies.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ async def test_server_proxy_websocket_headers(a_server_port_and_token: Tuple[int
[
(None, None, None, None),
(["first"], ["first"], "first", "first"),
(["first", "second"], ["first", "second"], "first", "first"),
# IMPORTANT: The tests below verify current bugged behavior, and the
# commented out tests is what we want to succeed!
#
Expand All @@ -369,14 +370,12 @@ async def test_server_proxy_websocket_headers(a_server_port_and_token: Tuple[int
# before the proxy/server handshake, and that makes it
# impossible. We currently instead just pick the first
# requested protocol no matter what what subprotocol the
# server picks.
# server picks and warn if there is a mismatch retroactively.
#
# Bug 1 - server wasn't passed all subprotocols:
(["first", "second"], ["first"], "first", "first"),
# (["first", "second"], ["first", "second"], "first", "first"),
# Tracked in https://github.com/jupyterhub/jupyter-server-proxy/issues/459.
#
# Bug 2 - server_responded doesn't match proxy_responded:
(["first", "favored"], ["first"], "first", "first"),
# Bug - server_responded doesn't match proxy_responded:
(["first", "favored"], ["first", "favored"], "favored", "first"),
# (["first", "favored"], ["first", "favored"], "favored", "favored"),
(
["please_select_no_protocol"],
Expand Down

0 comments on commit f9d6fcd

Please sign in to comment.