search

jupyterhub / ltiauthenticator

A JupyterHub authenticator for LTI
https://ltiauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
68 stars 54 forks source link

fix(lti13): Fix protocol for redirect_uri if behind a reverse proxy with TLS termination #165

Closed jeflem closed 1 year ago

jeflem commented 1 year ago

If JupyterHub is behind a reverse proxy, protocol used in redirect URI may be wrong. Typically, user agent und proxy communicate via HTTPS, whereas proxy and hub communicate via HTTP. Up to now HTTP will make it into the redirect URI although the redirect URI will be visited by the user agent. Thus, LTI platform will complain about incorrect redirect URI.

This PR fixes the problem. Similar situations occur in other handlers and work correctly, see handlers.py, line 66 for instance.

I tested the proposed modification on my setup (HTTPS, reverse proxy , Moodle as LTI platform), but do not have a complete dev environment in this setup . Thus, I did not run automatic tests.

Someone having deeper insight into the ltiauthenticator code should carefully check the proposed modification @martinclaus ?

Many thanks and best regards,

Jens

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
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. 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! :wave:
Welcome to the Jupyter community! :tada:

martinclaus commented 1 year ago

Hi @jeflem, thanks for spotting this inconsistency in determining the scheme when constructing URLs. I actually bumped into the same issue two days ago when setting up a test instance but didn't had time to do anything about it.

tornado uses the last value from the X-Forwarded-Proto header, which is http in a properly set up reverse proxy setting. What we want is the protocol the browser sees, which is the first value stored in the header (see https://github.com/tornadoweb/tornado/pull/2162). This is why we need get_client_protocol here.

welcome[bot] commented 1 year ago

Congrats on your first merged pull request in this project! :tada: congrats Thank you for contributing, we are very proud of you! :heart: