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

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

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Feb 21, 2024

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.

@consideRatio consideRatio force-pushed the test-subprotocols branch 2 times, most recently from a61619a to 4d9cd5b Compare February 21, 2024 19:11
@benz0li
Copy link

benz0li commented Feb 22, 2024

@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 consideRatio changed the title Improve websocket subprotocol request to backend server Ensure no blank Sec-Websocket-Protocol headers and warn about other websocket issues Feb 23, 2024
@consideRatio consideRatio changed the title Ensure no blank Sec-Websocket-Protocol headers and warn about other websocket issues Ensure no blank Sec-Websocket-Protocol headers and warn about another websocket issue Feb 23, 2024
@consideRatio consideRatio changed the title Ensure no blank Sec-Websocket-Protocol headers and warn about another websocket issue Ensure no blank Sec-Websocket-Protocol headers and warn if websocket subprotocol edge case occur Feb 23, 2024
@consideRatio consideRatio marked this pull request as ready for review February 23, 2024 07:41
@consideRatio consideRatio requested review from minrk and manics February 23, 2024 07:41
@consideRatio
Copy link
Member Author

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

@consideRatio consideRatio merged commit 1b9f84b into jupyterhub:main Feb 23, 2024
13 checks passed
@duytnguyendtn
Copy link

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!

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