Fix redirect_uri validation with query params

[pluff]

Summary

According to OAuth 2.0 protocol redirect_uri
shouldn't contain any extra query params.
We should consider exact query params list match
when comparing redirect_uris

More on topic: https://tools.ietf.org/id/draft-ietf-oauth-security-topics-18.html#name-protecting-redirect-based-f

Fixes #1527

[nbulaj]

Hey @pluff , thanks for the PR 🤝 !
I checked the document and think that you're right so 🟢 light

[rmm5t]

Concern. This should have never been incorporated into a patch-level release (5.5.2 -> 5.5.3). This is a breaking change, even if it is more of a direct interpretation of the spec.

[rmm5t]

@pluff @nbulaj omniauth-oauth2 sends the full callback URL (with query params) as the redirect_uri during the authorization code verification. I think this PR makes sense for the initial PreAuthorization#validate_redirect_uri check, but it doesn't make sense for the AuthorizationCodeRequest#validate_redirect_uri check of the redirect_uri.

Thoughts?

In other words, this PR breaks integration with omniauth-oauth2.

[rmm5t]

To be fair, It may very well be the case that this is a long standing issue with omniauth-oauth2:
omniauth/omniauth-oauth2#70 (comment)

[menisy]

Is this still allowing the state query param to be allowed?
https://www.oauth.com/oauth2-servers/redirect-uris/redirect-uri-registration/#per-request

[nbulaj]

Above one sounds reasonable 🤔 ☝️

[menisy]

@nbulaj doesn't seem to work, I produced a failing spec. I can work on it.

[nbulaj]

Yeah I checked original RFC and found a lot of spec for the state parameter:

• https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.2
• https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1
• ... and more ...

[menisy]

@nbulaj let me know if this looks okay, please and thank you!

[menisy]

Seems like I misinterpreted the spec about the state param being part of the redirect_uri.

However, this implementation is also not very accurate as per this excerpt from the spec:
https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.2

The authorization server SHOULD require the client to provide the
complete redirection URI (the client MAY use the "state" request
parameter to achieve per-request customization). If requiring the
registration of the complete redirection URI is not possible, the
authorization server SHOULD require the registration of the URI
scheme, authority, and path (allowing the client to dynamically vary
only the query component of the redirection URI when requesting
authorization).

With this change, this is no longer possible!

[nbulaj]

Released 5.5.4 with reverted changes

[menisy]

@nbulaj thanks a lot for the prompt action, let's look into this more closely once you (we) have the time.

[nbulaj]

@pluff had to revert the PR because of different rules for redirect URI comparison across the docs. We should look at the original RFC more closely and attentively. Sorry for that 😞

[menisy]

Seems to be more of an issue on the client side:
omniauth/omniauth-oauth2#141

It feels a bit like egg/chicken situation, the more I read the spec the more I'm getting confused TBH.

[nbulaj]

Yeah, the same thoughts. One of the ideas is to check OAuth implementation in other frameworks/languages to check main trend-offs for redirect URI validation. General RFC allows even pattern matching, but we still don't support it. So.. we have to "fight" with it :)

[menisy]

After some thought, it seems to be that there are 2 distinct cases which we probably need to treat differently.

First case, during the authorization request (pre_authorization in doorkeeper context), we compare the redirect_uri against the registered uri(s) for the client, in this case, we can be a bit more permissive about query params as per the spec (could be a configurable flag).

Second case, during the code-token exchange request (authorization_code_request in doorkeeper context), we need to compare the received redirect_uri against the one that was stored in the oauth_access_grant record, here however, we need to be strict and match them exactly as per the spec.

The thing is that we're using the same logic valid_for_authorization? for the 2 cases and this is where we might need to make a distinction.

I also figured out that the omniauth-oauth2 gem is not really abiding by the spec since it sends a different redirect_uri in the code-token exchange from the one that was used in the authorization request phase and this needs to be corrected. I will try to address this and issue a PR to fix this on their side.

Afterwards, I will try to look into introducing the logic (and possible configuration option) for the distinction between these 2 cases so that we're more abiding to the spec.