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

Doorkeeper::Errors::InvalidRedirectUri Raised When No Redirect URI Set #1682

Open
brent-cybrid opened this issue Dec 4, 2023 · 4 comments · May be fixed by #1701
Open

Doorkeeper::Errors::InvalidRedirectUri Raised When No Redirect URI Set #1682

brent-cybrid opened this issue Dec 4, 2023 · 4 comments · May be fixed by #1701

Comments

@brent-cybrid
Copy link

Steps to reproduce

Create an application with no redirect_uri set, i.e., redirect_uri=nil.

Set allow_blank_redirect_uri = true in the Doorkeeper config.

Hit GET /oauth/authorize with redirect_uri='' or redirect_uri=nil or omit the redirect_uri parameter.

Observe an Doorkeeper::Errors::InvalidRedirectUri exception with the message The requested redirect uri is malformed or doesn't match client redirect URI.

Expected behavior

In versions 5.6.6 and before an authorization code was returned.

Actual behavior

An Doorkeeper::Errors::InvalidRedirectUri exception with the message The requested redirect uri is malformed or doesn't match client redirect URI.

System configuration

Set allow_blank_redirect_uri = true in the Doorkeeper config.

@brent-cybrid brent-cybrid changed the title Doorkeeper::Errors::InvalidRedirectUri Raised When no Redirect URI Set Doorkeeper::Errors::InvalidRedirectUri Raised When No Redirect URI Set Dec 4, 2023
@brent-cybrid
Copy link
Author

Has this been looked at?

@nbulaj
Copy link
Member

nbulaj commented Feb 12, 2024

Hey @brent-cybrid .

In versions 5.6.6 and before an authorization code was returned.

I believe it was always like this 🤔

https://github.com/doorkeeper-gem/doorkeeper/blob/main/lib/doorkeeper/oauth/pre_authorization.rb#L100-L107

In any case this behavior should be reviewed ,m more details here #1678

@nbulaj nbulaj linked a pull request Mar 14, 2024 that will close this issue
@ThisIsMissEm
Copy link
Contributor

I believe the logic in the spec is that the Application must have a redirect_uri value, however, when doing the authorization request, you can omit it — though behavior when an application has multiple redirect URIs registered is a little unclear; The best information I can find is under "Dynamic Configuration":

If multiple redirection URIs have been registered, if only part of the redirection URI has been registered, or if no redirection URI has been registered, the client MUST include a redirection URI with the authorization request using the "redirect_uri" request parameter.

Refs:

So the example given where the Application has no redirect_uri's registered AND no redirect_uri is passed to the Authorization Endpoint (GET /oauth/authorize) MUST fail with Doorkeeper::Errors::InvalidRedirectUri

The setting allow_blank_redirect_uri = true only allows for situations where the Application HAS a redirect_uri registered, but none is supplied during the request to the Authorization Endpoint.

@ThisIsMissEm
Copy link
Contributor

So I think this is a "won't fix", but that the issue in #1678 is valid and should be fixed.

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 a pull request may close this issue.

3 participants