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

Use sanitised result of selected_subprotocol #446

Closed
wants to merge 1 commit into from

Conversation

Copy link

welcome bot commented Feb 15, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@benz0li
Copy link
Author

benz0li commented Feb 15, 2024

FYI @sylvaticus Fixed version deployed at https://coder.jupyter.b-data.ch.


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\.selected_subprotocol/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 15, 2024

Ping @consideRatio

FYI @duytnguyendtn

@sylvaticus
Copy link

I confirm that the sanitation applied to handlers.py solves the issue with the VSCode extension LaTex-Workshop, thank you very much...

@benz0li
Copy link
Author

benz0li commented Feb 15, 2024

Alternatively subprotocols=self.selected_subprotcol as described in #442.

@duytnguyendtn
Copy link

Thanks for putting together this PR! Hopefully we can get some traction from the devs and get this merged

@benz0li benz0li changed the title Use sanitised result of select_subprotocol Use sanitised result of selected_subprotocol Feb 16, 2024
@consideRatio
Copy link
Member

Thank you for digging into this topic @benz0li!!!

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

@benz0li
Copy link
Author

benz0li commented Feb 16, 2024

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.

@consideRatio You find the technical details in #442.

self.select_subprotcol(self.subprotocols) did not work, so I deleted [override function] select_subprotocol in handlers.py and went for [tornado's] self.selected_subprotcol.

Technically, I am not up to such things either. Maybe @duytnguyendtn has more insight.

@duytnguyendtn
Copy link

Haha and you've discovered why I didn't feel brave enough to author the PR! Frankly, I don't have the background to evaluate the safety or the "breaking" nature of this change. I hoped to open a discussion in #442 with the maintainers to help evaluate it's safety and compatibility. Echoing @benz0li, @consideRatio please take a look at my investigation in #442 where I arrived at the two potential solutions at the bottom which @benz0li is basing this PR off of. I tried to make it as thorough as I could and explore the code traces.

Please let me know if I can be of any further assistance!

@benz0li
Copy link
Author

benz0li commented Feb 16, 2024

Because Jupyter Server Proxy borrows a lot from Tornado Web Server, I dare to ping @bdarnell in this case.

IMHO self.subprotocols and self.selected_subprotcol return different results in one case only: When there is no subprotocol proposed.

Former vs latter:

  1. Subprotocol: [] vs None
  2. Header: Sec-Websocket-Protocol: vs 'not sent'
  3. Connection: Aborted (in some cases) vs 'successfully established'

Maybe there is a simpler and saner way to prevent subprotocols=[] when there is no subprotocol proposed.

@benz0li
Copy link
Author

benz0li commented Feb 18, 2024

Closed in favour of #448.

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