Skip to content

Fix redirect_uri issue with tornado reversed url#674

Merged
omab merged 1 commit intoomab:masterfrom
ByteInternet:fix-tornado-redirect-uri
Oct 5, 2015
Merged

Fix redirect_uri issue with tornado reversed url#674
omab merged 1 commit intoomab:masterfrom
ByteInternet:fix-tornado-redirect-uri

Conversation

@mvschaik
Copy link
Copy Markdown
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
Copy Markdown
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