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 redirect_uri issue with tornado reversed url #674

Merged
merged 1 commit into from
Oct 5, 2015

Conversation

mvschaik
Copy link
Contributor

When reversing URLs, tornado doesn't interpret the regex optional symbol '?'. This causes the redirect_uri to be https://example.com/complete/mybackend/? with the question mark appended. Some providers will simply append to this uri, causing URLs like https://example.com/complete/mybackend/??code=.... with two question marks. This makes the interpretation of the query string fail.

The provider in this case is django-oidc-provider. Arguably that library should be smarter in constructing the redirection, but removing the question mark from the uri solves these kind of issues.

Alternatively we could strip the question mark from the uri in the tornado strategy in here, but this seemed simpler.

When reversing URLs, tornado doesn't interpret the regex optional symbol
'?'. This causes the redirect_uri to be
https://example.com/complete/mybackend/? with the question mark
appended. Some providers will simply append to this uri, causing URLs
like https://example.com/complete/mybackend/??code=.... with two
question marks. This makes the interpretation of the query string fail.

The provider in this case is
https://github.com/juanifioren/django-oidc-provider. Arguably that
library should be smarter in constructing the redirection, but removing
the question mark from the uri solves these kind of issues.

Alternatively we could strip the question mark from the uri in the
tornado strategy, but this seemed simpler.
omab added a commit that referenced this pull request Oct 5, 2015
Fix redirect_uri issue with tornado reversed url
@omab omab merged commit f11788b into omab:master Oct 5, 2015
@omab
Copy link
Owner

omab commented Oct 5, 2015

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants