Doorkeeper: Redirect URL should allow query param

[amngibson]

I was recently working on an Amazon Echo app. There are two problems with doorkeeper and Alexa/Echo.

1) Amazon does not pass redirect_uri in oauth/authorize and doorkeeper requires it. However, I see that is being debated in a different thread. So not creating this issue for that.

I have found a workaround in Amazon for that, but it presented another issue.

2) The redirect URL for Alexa is in this format "https://pitangui.amazon.com/spa/skill/account-linking-status.html?vendorId=XXXXXXXXXXX

I've narrowed the issue down to this method in this file:

def validate_redirect_uri
return false unless redirect_uri.present?
Helpers::URIChecker.native_uri?(redirect_uri) ||
Helpers::URIChecker.valid_for_authorization?(redirect_uri, client.redirect_uri)
end

https://github.com/doorkeeper-gem/doorkeeper/blob/master/lib/doorkeeper/oauth/pre_authorization.rb

Unless I am reading the spec wrong, doorkeeper should allow for this.

Section 3.1.2
The redirection endpoint URI MUST be an absolute URI as defined by
[RFC3986] Section 4.3. The endpoint URI MAY include an
"application/x-www-form-urlencoded" formatted (per Appendix B) query
component ([RFC3986] Section 3.4), which MUST be retained when adding
additional query parameters. The endpoint URI MUST NOT include a
fragment component.

[tute]

That is not application/x-www-form-urlencoded.

[rectifyer]

@amngibson I had this issue too and figured it out. When saving the URL in your OAuth app, just remove the query string part ?vendorId=XXXXXXXXXXX and only use https://pitangui.amazon.com/spa/skill/account-linking-status.html as your redirect_uri.

Looking in the code at https://github.com/doorkeeper-gem/doorkeeper/blob/master/lib/doorkeeper/oauth/helpers/uri_checker.rb you can see on line 14 it removes the query string from the 1st url, but not the 2nd. That's why the comparison fails.

The workaround is likely fine, but I'm not sure if this is an actual bug in the comparison or not.

[tute]

@amngibson is this still an issue for you? Does anything need to change in doorkeeper? Thanks!

[tute]

Closing due to lack of activity. @amngibson, if this is still an issue for you, let me know and we continue triaging this. Thanks!

[amngibson]

@rectifyer solution worked for me. Sorry on delayed response.