search

jupyterhub / jupyter-server-proxy

Jupyter notebook server extension to proxy web services.
https://jupyter-server-proxy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
347 stars 148 forks source link

Ensure no blank `Sec-Websocket-Protocol` headers and warn if websocket subprotocol edge case occur #458

Closed consideRatio closed 6 months ago

consideRatio commented 6 months ago

This PR ensures we don't pass a blank Sec-Websocket-Protocol header (Sending a blank header is incorrect) when we proxy a websocket. This fixes #442, fixes #445, is assumed to fix #332, and closes #448.

This project currently proxies websockets by first accepting an incomming websocket connection were a subprotocol is established, and then opening a websocket connection to a server to proxy to. This is a problem and can lead to bugs that we now warn about. This PR adds tests to document our current problematic handling.

Thank you @duytnguyendtn, @zoghbi-a, and @benz0li for working this - this is building a lot on their investigation and conclusions, which were critical as I didn't understand much about this before.

benz0li commented 6 months ago

@consideRatio I merged main and consideRatio/test-subprotocols into benz0li/fix-442-and-445-simple, adapted the Proxy tests and ran the test suite: It also passes the empty and no subrotocol tests.

consideRatio commented 6 months ago

Rebased to be put on top of #457 not bundled with this PR but with commits now in main.

duytnguyendtn commented 6 months ago

I was able to confirm this fixed our issue on the NASA side (FYI @zoghbi-a)! Thanks so much @consideRatio for patching this bug and @benz0li for helping with the investigation! Glad to see a powerful example of open source community coming together to fix this issue :)

We look forward to the 4.1.1 release!