search

juanifioren / django-oidc-provider

OpenID Connect and OAuth2 provider implementation for Djangonauts.
http://django-oidc-provider.readthedocs.org
MIT License
423 stars 238 forks source link

Error response appends extra ? to redirect_uri #355

Open geoff-va opened 4 years ago

geoff-va commented 4 years ago

When the redirect_uri already contains a query string, if there is an error with the authorization request then an additional ? is appended to the redirect_uri with the error query params.

Example: redirect_uri=https://some.domain.com/index?action=authorize

becomes: redirect_uri=https://some.domain.com/index?action=authorize?error=login_required&error_description=The Authorization Server requires End-User authentication&...

I couldn't find anywhere in the spec that says you can't already have a query string as part of the redirect_uri. Perhaps the solution would be to check the redirect_uri and only append the ? if a query string doesn't already exist on the redirect_uri? If it does we append & instead of ?.

Open to suggestions if anyone has experience with this situation.

toti1212 commented 4 years ago

Hi @geoff-va ! Good to know this! I have no experience with that bug, but, I realized that the function create_response_uri could be better to fix that.

I have a problem with the same function when the redirect_uri is a deep link like slack://test That function returns me something like slack:?code=xxx

geoff-va commented 4 years ago

@toti1212 I think the error (in my case at least) is coming from create_uri in errors.AuthorizeError. I'm going to try to make that a little more robust.

I think the ultimate goal there is to leave the redirect_uri intact and append our parameters to either the query string or the query fragment (if implicit).

Good point about deep linking and non-http schemas, too! I'll consider that as well. I haven't looked into create_response_uri yet.