propagate check_origin of JupyterHandler to enable CORS websocket access

[fhoehle]

Thanks for this extremly helpful package. Exposing applications like code-server using jupyter-vscode-proxy via an inverting proxy e.g. gcr.io/inverting-proxy fails due to not allowed cross origin websockets. This seccurity rule was introduced in tornado>=4 see check_origin and its doc string. jupyter-server provides a configurable check_origin function consuming allow_origin and allow_origin_pat being very fine tunable cross origin permissions. This PR exposes the jupyter-server-proxy to this functionality and being similar configured as the jupyter-server regarding cross origin acceptance.

Please comment/review I'm open to suggestions and feedback.

[welcome]

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

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

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! 🎉

[fhoehle]

This issue could be related/fixed by this PR #248.

[janjagusch]

@consideRatio could you maybe take a look at this, please? this would be very useful for us, too. 🙂

[consideRatio]

@fhoehle I think for this PR to be accepted, i think it will need to not make the project require jupyter_server.

These questions will likely be relevant to address, and anyones thoughts about those would be useful review input.

• Is or could this be a breaking change?
• Is this something that can / should be implemented also when notebook is used as a server instead of jupyter_server, or should this be a feature only for use with jupyter_server?

@manics @minrk @yuvipanda - this is at least a small PR in lines of code changed, but I'm not knowledgeable enough about this project or about CORS to review this. Could you give it a glance?

[minrk]

@consideRatio I think jupyter_server is already required.

I think this is fine, but I also think it can be simpler by putting it on the base ProxyHandler, as is done in jupyter_server when a class needs to inherit from WebSocketHandler before JupyterHandler:

    def check_origin(self, origin=None):
        return JupyterHandler.check_origin(self, origin)

[fhoehle]

@consideRatio and @minrk thanks for your review and input.

I think this is fine, but I also think it can be simpler by putting it on the base ProxyHandler, as is done in jupyter_server when a class needs to inherit from WebSocketHandler before JupyterHandler:

    def check_origin(self, origin=None):
        return JupyterHandler.check_origin(self, origin)

I adapted the PR according to @minrk proposal. In the beginning I was unsure if would be more suitable to add the reassignement of check_origin in the outer most layer of inheritance. BTW thanks for updating the test suite.

[consideRatio]

Thanks @minrk for helping out! Does this LGTY now?

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[minrk]

Yes, thank you!

[consideRatio]

I labelled this as a bugfix, is that reasonable with regards to how it will be listed in the changelog. Is it an enhancement, or breaking change?

[fhoehle]

Is it an enhancement, or breaking change?

I would consider this an enhancement. Because this functionality could achieved by overwriting the origin header by the inverting proxy.

[consideRatio]

@fhoehle what about breaking / non-breaking change?

[fhoehle]

@fhoehle what about breaking / non-breaking change?

@consideRatio I would see it as non breaking, because existing configuration does not become invalid. But if someone is using this on an exposed system without any proxy service web-sockets will become accessible.

It would be very helpful for me to use a release including this functionality. Would that be possible @consideRatio @minrk ? Do you need more input besides the points already mentioned?

[minrk]

Bugfix is a fine label for changelog purposes, since it fixes the code's behavior to do what it is intended to do. While it's technically conceivable to rely on the incorrect behavior, I wouldn't consider fixing that a breaking change.

It's very common for changes to not cleanly divide into bug/enhancement/feature/etc. and I would like to avoid spending too much time on the labeling, as long as it gets a label.

Do you need more input besides the points already mentioned?

No, I think this is all done. Just need to get the release ready.

[fhoehle]

@minrk thanks!