Possible issue with mirroring back the query params that were passed from authorization server

[menisy]

With this update from Doorkeeper, existing authorization flows stopped working. This is no longer the case since this update on Doorkeeper was reverted.

However, it seemed to me that there is a strange behaviour when I was debugging to see why this was no longer working.

A couple of pry's later I figured out what's going on. I will use an example as it's a bit complicated to explain:
Resource server on localhost:3000
Authorization server on localhost:5000

1. Registered client redirect_uri on Doorkeeper (authorization server) side is: http://localhost:3000/auth/some_provider/callback
2. During authorization request phase, the client issues the proper authorization request url like: http://localhost:5000/oauth/authorize?client_id=some_client_id&&redirect_uri=http%3A%2F%2Flocalhost%3A3000%2Fauth%some_provider%2Fcallback&response_type=code&state=some_state
3. Authorization server does the authorization and redirects back to http://localhost:3000/auth/some_provider/callback?code=some_authorization_code&state=some_state
4. Now the client needs to exchange the code for the access token from the authorization server, however, while constructing the request it uses the redirect_uri using this callback_url method defined in omniauth which adds the request.query_string to the callback_url. So the code exchange request becomes: POST http://localhost:5000/oauth/token with params:

client_id: some_client_id,
client_secret: some_client_secret,
code: some_authorization_code,
redirect_uri: http://localhost:3000/auth/some_provider/callback?code=some_authorization_code&state=some_state

and this is where it is questionable whether the code and state should be replied back to the authorization server as part of the redirect_uri even though they didn't originally exist in the request phase. And this is where the reverted update was facing an issue as it couldn't fully match the redirect_uri with these extra query params against the one(s) defined on the authorization server during registration of the client.

This is the source of it in omniauth, I honestly did not dive deep enough in the spec to know what is the expected behaviour, however, this behaviour doesn't look correct.

[BobbyMcWho]

Thanks for the detailed report @menisy, I will need to do some looking into things as I'm not sure off the bat what the correct behavior is. I'd be willing to review a PR and evaluating the risk of any changes if you have a suggestion in the short term

[menisy]

@BobbyMcWho thanks for the prompt response. I have something in mind, I will try to issue a PR soon.

[menisy]

@BobbyMcWho please have a look here whenever you have some time

[BobbyMcWho]

I'm somewhat surprised that the extra params are causing issues, because I've used omniauth-okta before, and okta has some fairly strict redirect uri matching if I recall correctly

[menisy]

@BobbyMcWho according to omniauth-okta's implementation, they're overriding the callback_url to include only full_host and callback_path and not the query_string. This in itself is also not very accurate since the callback url could indeed contain query params which should be maintained through calls. My PR only ensures that whatever callback url was used in the request phase gets reused in the callback phase (which would still maintain query params that existed in the request phase, and not include the extra ones passed from the authorization server (i.e code and state)).

The spec is very clear that the redirect_uri used in the code-token exchange must exactly match the one that was used to obtain the code during the authorization request.

I see that at least #70 and #93 are related to this.