Possible issue with OmniAuth-OAuth2 generic strategy regarding redirect_url validation

[ablancag]

Hi there.

I was following a tutorial on how to use Doorkeeper in an OAuth2 provider and OmniAuth + OmniAuthOauth2 in a client when I found this issue / incompatibility.

The issue is: OmniAuth-OAuth2, during the callback phase, when requesting the token from the provider running Doorkeeper, retains the query string received in the callback, meaning that the new request to fetch the token includes a query string with two parameters: code and state.

This is further documented in omniauth/omniauth-oauth2#81, which itself refers to http://tools.ietf.org/html/rfc6749#section-3.1.2 to justify their implementation.

The version of OmniAuth-OAuth2 that is incompatible with Doorkeeper is version 1.4.0 and later. The version that works is 1.3.1.

The first thought I had about this was that the solution appeared to be to implement a check where a query string is accounted for, in method validate_redirect_uri, in file ./doorkeeper/lib/doorkeeper/oauth/authorization_code_request.rb. But after rereading the standard, I ended up with some doubts, which I hope are clarified by this issue. I believe the get_token call should be done with the base redirect_url, without any query string, being a separate request (as is right now). I believe also that the query string received during the callback phase is not required in further calls. But I may be wrong, since the OmniAuth-OAuth2 maintainer believes otherwise.

So, what do you think?

[t-anjan]

I spent a good part of 4 hours debugging this issue just now.

omniauth/omniauth-oauth2#70 : The offending PR in omniauth-oauth2 gem.

For now, I just fixed it using the suggestions from all the other broken omniauth provider gems. Example: decioferreira/omniauth-linkedin-oauth2#29

So, in my custom Omniauth strategy file, I just added the following method:

def callback_url
  full_host + script_name + callback_path
end

Works for me now.

[tute]

Another workaround seems to be locking that gem version gem 'omniauth-oauth2', '~> 1.3.1'.

@sferik do you think doorkeeper behavior needs to be updated to be in line with the part of the spec you quote? I wonder if it's worth the time-consuming debugging sessions that this would put on users' shoulders.

[ablancag]

I still have my doubts regarding the quote: what does the standard say about the call to fetch the token? Does it say anything about a query string?

That would be a way of clarifying what to do with this issue.

[tute]

This doesn't seem like a bug in doorkeeper, so I'll close this issue. If it is, please let me know and we'll reopen it, and work through it. Thank you!