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

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

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

jeflem
Copy link
Contributor

@jeflem jeflem commented Jun 12, 2023

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
Copy link

welcome bot commented Jun 12, 2023

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

@martinclaus
Copy link
Collaborator

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 tornadoweb/tornado#2162). This is why we need get_client_protocol here.

@martinclaus martinclaus changed the title Fix protocol for redirect_uri in handlers.py fix(lti13): Fix protocol for redirect_uri in handlers.py Jun 13, 2023
@martinclaus martinclaus changed the title fix(lti13): Fix protocol for redirect_uri in handlers.py fix(lti13): Fix protocol for redirect_uri if behind a reverse proxy with TLS termination Jun 13, 2023
@martinclaus martinclaus merged commit 4e304d6 into jupyterhub:main Jun 13, 2023
@welcome
Copy link

welcome bot commented Jun 13, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

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

Successfully merging this pull request may close these issues.

2 participants