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 should be the last parameter #874

Closed
wants to merge 2 commits into from

Conversation

barredterra
Copy link

@barredterra barredterra commented Feb 6, 2024

https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2 says:

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.

Query parameters added after the redirect URI may be interpreted as part of the redirect URI, resulting in an "Invalid Redirect URI" error from the authorization server. Adding redirect_uri as the last query parameter seems to be an easy way to avoid this.

Example:

oauth = OAuth2Session(
	client_id="my-client-id",
	redirect_uri="https://example.org/oauth/callback/abc",
	scope=["my-scope"],
)
authorization_url, state = oauth.authorization_url(self.authorization_uri, access_type=offline)
authorization_url == "https://accounts.google.com/o/oauth2/v2/auth?response_type=code&client_id=my-client-id&redirect_uri=https://example.org/oauth/callback/abc&scope=my-scope&state=my-state&access_type=offline"

Now the authorization server might think that the redirect URI is "https://example.org/oauth/callback/abc&scope=my-scope&state=my-state&access_type=offline", which does not match the registered URI "https://example.org/oauth/callback/abc".

After this PR, the resulting authorization_url would be "https://accounts.google.com/o/oauth2/v2/auth?response_type=code&client_id=my-client-id&scope=my-scope&state=my-state&access_type=offline&redirect_uri=https://example.org/oauth/callback/abc", so the redirect_uri cannot be interpreted in a wrong way.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started the CI. is it possible to adjust the unit test to see if any regression happens?

@barredterra
Copy link
Author

Please carefully verify that the problem and fix are valid, since this is my first contribution here and I don't really know what I'm doing. 😄

@barredterra barredterra deleted the redirect_uri_last branch March 20, 2024 14:30
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.

None yet

2 participants