Set subprotocols=None when there is no subprotocol proposed

benz0li commented 8 months ago

Modify tests to use headers
Cleanup test class SubprotocolWebSocket (Edited to clarify its a test class only)
Add test for empty and no subprotocols
Fix https://github.com/jupyterhub/jupyter-server-proxy/issues/442
Fix https://github.com/jupyterhub/jupyter-server-proxy/issues/445

EDIT

We think this fixes #332, and will assume that for now but could be wrong.

benz0li commented 8 months ago

Current impementation (branch main) fails [newly introduced] test_server_proxy_websocket_no_subprotocols:

pytest

[...]
tests/test_proxies.py::test_server_proxy_websocket_no_subprotocols[notebook] FAILED [ 41%]
[...]
tests/test_proxies.py::test_server_proxy_websocket_no_subprotocols[lab] FAILED [ 89%]
[...]
=================================== FAILURES ===================================
____________ test_server_proxy_websocket_no_subprotocols[notebook] _____________

event_loop = <_UnixSelectorEventLoop running=False closed=False debug=False>
a_server_port_and_token = (46341, '85f23093-d946-4883-86a4-9843955d2d93')

    def test_server_proxy_websocket_no_subprotocols(
        event_loop, a_server_port_and_token: Tuple[int, str]
    ):
>       event_loop.run_until_complete(_websocket_no_subprotocols(a_server_port_and_token))

tests/test_proxies.py:422: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/local/lib/python3.11/asyncio/base_events.py:654: in run_until_complete
    return future.result()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

a_server_port_and_token = (46341, '85f23093-d946-4883-86a4-9843955d2d93')

    async def _websocket_no_subprotocols(a_server_port_and_token: Tuple[int, str]) -> None:
        PORT, TOKEN = a_server_port_and_token
        url = f"ws://{LOCALHOST}:{PORT}/python-websocket/headerssocket"
        conn = await websocket_connect(url)
        await conn.write_message("Hello, world!")
        msg = await conn.read_message()
        headers = json.loads(msg)
>       assert "Sec-Websocket-Protocol" not in headers
E       AssertionError: assert 'Sec-Websocket-Protocol' not in {'Accept-Encoding': 'gzip', 'Connection': 'Upgrade', 'Host': '127.0.0.1:46341', 'Sec-Websocket-Extensions': 'permessage-deflate; client_max_window_bits', ...}

tests/test_proxies.py:416: AssertionError
----------------------------- Captured stderr call -----------------------------
[D 2024-02-18 10:33:50.354 ServerApp] 101 GET /python-websocket/headerssocket (@127.0.0.1) 0.28ms
[I 2024-02-18 10:33:50.354 ServerApp] Trying to establish websocket connection to ws://localhost:53811/headerssocket
[I 240218 10:33:50 web:2348] 101 GET /headerssocket (127.0.0.1) 0.16ms
[I 2024-02-18 10:33:50.355 ServerApp] Websocket connection established to ws://localhost:53811/headerssocket
_______________ test_server_proxy_websocket_no_subprotocols[lab] _______________

event_loop = <_UnixSelectorEventLoop running=False closed=False debug=False>
a_server_port_and_token = (46341, '85f23093-d946-4883-86a4-9843955d2d93')

    def test_server_proxy_websocket_no_subprotocols(
        event_loop, a_server_port_and_token: Tuple[int, str]
    ):
>       event_loop.run_until_complete(_websocket_no_subprotocols(a_server_port_and_token))

tests/test_proxies.py:422: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/local/lib/python3.11/asyncio/base_events.py:654: in run_until_complete
    return future.result()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

a_server_port_and_token = (46341, '85f23093-d946-4883-86a4-9843955d2d93')

    async def _websocket_no_subprotocols(a_server_port_and_token: Tuple[int, str]) -> None:
        PORT, TOKEN = a_server_port_and_token
        url = f"ws://{LOCALHOST}:{PORT}/python-websocket/headerssocket"
        conn = await websocket_connect(url)
        await conn.write_message("Hello, world!")
        msg = await conn.read_message()
        headers = json.loads(msg)
>       assert "Sec-Websocket-Protocol" not in headers
E       AssertionError: assert 'Sec-Websocket-Protocol' not in {'Accept-Encoding': 'gzip', 'Connection': 'Upgrade', 'Host': '127.0.0.1:46341', 'Sec-Websocket-Extensions': 'permessage-deflate; client_max_window_bits', ...}

tests/test_proxies.py:416: AssertionError
----------------------------- Captured stderr call -----------------------------
[D 2024-02-18 10:33:54.011 ServerApp] 101 GET /python-websocket/headerssocket (@127.0.0.1) 0.37ms
[I 2024-02-18 10:33:54.011 ServerApp] Trying to establish websocket connection to ws://localhost:41671/headerssocket
[I 240218 10:33:54 web:2348] 101 GET /headerssocket (127.0.0.1) 0.20ms
[I 2024-02-18 10:33:54.012 ServerApp] Websocket connection established to ws://localhost:41671/headerssocket
[...]

Reason: Header 'Sec-Websocket-Protocol': '' instead of 'no header'/'header not sent'.

benz0li commented 8 months ago

@consideRatio Regarding https://github.com/jupyterhub/jupyter-server-proxy/pull/446#issuecomment-1947990685

For a merge this needs to be vetted with regards to "is this a breaking change" and "is this secure" for example, things that i personally struggle with in this tech-context.

is it a breaking change: No
is this secure: Yes

It would be very helpful for me if you could provide your take on that @benz0li, it would be a great starting point for me to review this change proposal.

When there is no subprotocol proposed, header Sec-Websocket-Protocol should be omitted rather than sent without a value.

Cross reference: RFC 6455 - The WebSocket Protocol > 1. Introduction > 1.9 Subprotocols Using the WebSocket Protocol

FYI @duytnguyendtn

@sylvaticus Fix for Jupyter Server Proxy v4.1.0:

cd /usr/local/lib/python3.11/site-packages/jupyter_server_proxy \
  && cp -a handlers.py handlers.py.orig \
  && sed -i 's/subprotocols=self\.subprotocols/subprotocols=self\.subprotocols if self\.subprotocols else None/g' handlers.py

ℹ️ /usr/local/lib/python3.11/site-packages/ is where Jupyter Server Proxy is installed in my setup. Adapt to your setup.

benz0li commented 8 months ago

Most likely

Fix https://github.com/jupyterhub/jupyter-server-proxy/issues/332

consideRatio commented 8 months ago

Super low on time, seeing test failures made me hesitate on getting review done, I see test failures in the main branch as well though (https://github.com/jupyterhub/jupyter-server-proxy/issues/449), are the test failures here the same as in the main branch @benz0li?

benz0li commented 8 months ago

are the test failures here the same as in the main branch @benz0li?

@consideRatio Yes, they are the same.

jupyterhub / jupyter-server-proxy

Set subprotocols=None when there is no subprotocol proposed #448

EDIT