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

Always requiring redirect_uri is not compliant to RFC 6749 #1678

Open
westnordost opened this issue Nov 22, 2023 · 2 comments
Open

Always requiring redirect_uri is not compliant to RFC 6749 #1678

westnordost opened this issue Nov 22, 2023 · 2 comments

Comments

@westnordost
Copy link

It looks like doorkeeper always requires the redirect_uri parameter to be set in authorization requests.

def validate_redirect_uri
return false if redirect_uri.blank?
Helpers::URIChecker.valid_for_authorization?(
redirect_uri,
client.redirect_uri,
)
end

This does not seem to be compliant to RFC 6749 - The OAuth 2.0 Authorization Framework:

  • 4.1.1. Authorization Request lists redirect_uri as OPTIONAL

  • 4.1.3. Access Token Request lists redirect_uri only as REQUIRED if it was supplied in the authorization request

  • 3.1.2.3. Dynamic Configuration clarifies that redirect_uri is REQUIRED only in case no redirection URI has been specified during client registration:

    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.

    When a redirection URI is included in an authorization request, the authorization server MUST compare and match the value received against at least one of the registered redirection URIs (or URI components) as defined in [RFC3986] Section 6, if any redirection URIs were registered. If the client registration included the full redirection URI, the authorization server MUST compare the two URIs using simple string comparison as defined in [RFC3986] Section 6.2.1.

I understand that by default, doorkeeper also does not allow leaving the redirect url blank during client registration (but it can be changed in the configuration).

Expected behavior

  • if client registered a redirection URI, the redirect_uri parameter is optional in the authorization request
  • the redirect_uri parameter is only required in the access token request if it was specified in the authorization request. In this case, it must match

Actual behavior

  • redirect_uri parameter is always required in the authorization request and access token request

Related

@westnordost
Copy link
Author

Note that in the draft OAuth 2.1 specification, redirect_uri has been removed from the access token request, or, for backward compatibility, made optional. The reason is

In OAuth 2.1, authorization code injection is prevented by the code_challenge and code_verifier parameters, making the inclusion of the redirect_uri parameter serve no purpose in the token request. As such, it has been removed.

(see https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-09#section-10.2-2)

@nbulaj
Copy link
Member

nbulaj commented Mar 14, 2024

4.1.1. Authorization Request lists redirect_uri as OPTIONAL

4.1.3. Access Token Request lists redirect_uri only as REQUIRED if it was supplied in the authorization request

3.1.2.3. Dynamic Configuration clarifies that redirect_uri is REQUIRED only in case no redirection URI has been specified during client registration:

What I was thinking about is:

Access Token Request always have to check redirect_uri because grant always have a redirect_uri from authorization request (which in turn allow it to be blank, but in such case grant copies redirect URI from the client - it's from 3.1.2.3 - "i_f no redirection URI has been registered, the client MUST include a redirection URI"_). So at the end redirect uri is always present for Access token request 🤔

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

No branches or pull requests

2 participants