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

Keep proxying all requested subprotocols #462

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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