omniauth-oauth2
omniauth-oauth2 copied to clipboard
omniauth
Fix redirect_uri so that it does not include `code` or `state`
I think this is a better fix for #28, and saves many OAuth2 strategies broken by #70.
@agrobbin @sferik Could you review this, if you still have an interest in the issue?
#70 was merged without a spec, so I added specs to make my intention clear. If redirect_uri is given in token_params, it should be honored. If not, query parameters added by the OAuth2 provider in a callback should be stripped off from the redirect_uri value to be posted to the token endpoint, to avoid redirect_uri mismatch.
Coverage decreased (-4.9%) to 80.0% when pulling a4edc6f9fe1ae9f764cd2cf222c9f575703cc480 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Build failures are not my fault, gem dependencies must be updated for older rubies.
@knu you can add the following to the Gemfile to fix one of the CI errors
gem 'rack', '< 2.0.1', platform: :ruby_21
(I do not have write access to the PR otherwise I would do this for you)
Coverage decreased (-4.9%) to 80.0% when pulling f8f6b5fa6eaa126821a6df936f65b7969b7ad18c on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling f8f6b5fa6eaa126821a6df936f65b7969b7ad18c on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling 2c6ff31f7dd589f48069061f3a319b22513c4076 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling 2c6ff31f7dd589f48069061f3a319b22513c4076 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling 0903ac489ade95febb6b25bb1d6e1d4d9501ec36 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling 0903ac489ade95febb6b25bb1d6e1d4d9501ec36 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling 16bd81a9059e61f762526596dce519413b8f3b31 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-4.9%) to 80.0% when pulling 16bd81a9059e61f762526596dce519413b8f3b31 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Coverage decreased (-3.8%) to 81.176% when pulling 23ffea5d4e9f13d838570c89c5939f6525546e22 on knu:fix_redirect_uri into 464fcef6c0a5f1a7526b9701cec37f342ce57ae3 on intridea:master.
Keeping in mind that I am not a maintainer of this gem, and contributed #70 more than 7 years ago, I think what's bitten this gem from my earlier change was that the callback_url is used both by the request_phase and by build_access_token, when build_access_token really should not be the current URL, but rather the original redirect_uri passed in to the authorization request.
RFC 6749 Section 4.1.1 states that the redirect_uri passed in the request phase must abide by Section 3.1.2, which states:
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.
Section 4.1.3 then states:
redirect_uri REQUIRED, if the "redirect_uri" parameter was included in the authorization request as described in Section 4.1.1, and their values MUST be identical.
My change in #70 brought this gem into compliance w/ Section 3.1.2 (consider someone who has a redirect_uri that looks like https://example.com/auth/callback?foo=bar), but violates 4.1.3, since that original redirect_uri (without any subsequent params, i.e. code and/or state) is lost.
I have not used this gem in any projects in quite awhile, but if I'm understanding the specification correctly, my recommendation would be the that the original redirect_uri used in request_phase should be stored in session state, and then used in build_access_token. I think that aligns more closely w/ the specification.
