Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redirect_uri so that it does not include code or state #100

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

knu
Copy link

@knu knu commented Nov 14, 2016

I think this is a better fix for #28, and saves many OAuth2 strategies broken by #70.

@knu
Copy link
Author

knu commented Nov 14, 2016

@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.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling a4edc6f on knu:fix_redirect_uri into 464fcef on intridea:master.

@knu
Copy link
Author

knu commented Nov 14, 2016

Build failures are not my fault, gem dependencies must be updated for older rubies.

@tdm00
Copy link

tdm00 commented Nov 14, 2016

@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)

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling f8f6b5f on knu:fix_redirect_uri into 464fcef on intridea:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling f8f6b5f on knu:fix_redirect_uri into 464fcef on intridea:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling 2c6ff31 on knu:fix_redirect_uri into 464fcef on intridea:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling 2c6ff31 on knu:fix_redirect_uri into 464fcef on intridea:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling 0903ac4 on knu:fix_redirect_uri into 464fcef on intridea:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling 0903ac4 on knu:fix_redirect_uri into 464fcef on intridea:master.

@coveralls
Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling 16bd81a on knu:fix_redirect_uri into 464fcef on intridea:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 80.0% when pulling 16bd81a on knu:fix_redirect_uri into 464fcef on intridea:master.

@coveralls
Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-3.8%) to 81.176% when pulling 23ffea5 on knu:fix_redirect_uri into 464fcef on intridea:master.

@jonhue
Copy link

jonhue commented Apr 14, 2017

This should be merged.

@ghiculescu
Copy link

very keen for this. seems weird that #28 was merged even though it appears to go against the spec.

@agrobbin
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants