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

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

Closed
wants to merge 1 commit into from

Conversation

benz0li
Copy link

@benz0li benz0li commented Feb 18, 2024

EDIT

@benz0li
Copy link
Author

benz0li commented Feb 18, 2024

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'.

- Modify tests to use headers
- Remove SubprotocolWebSocket
- Add test for empty and no subprotocols
- Fix jupyterhub#442
- Fix jupyterhub#445
@benz0li benz0li force-pushed the fix-442-and-445-simple branch from cb1788b to 7f156bc Compare February 18, 2024 11:58
@benz0li
Copy link
Author

benz0li commented Feb 19, 2024

@consideRatio Regarding #446 (comment)

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.

  1. is it a breaking change: No
  2. 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
Copy link
Author

benz0li commented Feb 19, 2024

@consideRatio
Copy link
Member

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 (#449), are the test failures here the same as in the main branch @benz0li?

@benz0li
Copy link
Author

benz0li commented Feb 19, 2024

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

@consideRatio Yes, they are the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants