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

Reuse callback_url between request phase and callback phase #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

menisy
Copy link

@menisy menisy commented Oct 6, 2021

As explained in #141, the callback_url being used in the callback phase is different than the one sent to the authorization server during the request phase (TLDR: During the callback phase, state and code are included as query params within the redirect_uri). This does not comply with the spec which is very clear that the redirect_uri (aka callback_url) in the code-token exchange request must match the one sent in the authorization request.

According to the spec:

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
.

PS: It was a bit tricky to write the tests in a way to test this flow within the callback_phase method given the amount of mocking needed for the happy case (especially for parent class logic), so I opted for testing the build_access_token method.

@menisy
Copy link
Author

menisy commented Oct 10, 2021

@BobbyMcWho can we get some love here? Please and thank you ❤️

@BobbyMcWho
Copy link
Member

Sorry, I saw this and gave it some thought, but I'm not entirely sure if this could potentially cause cookie overflows if the redirect url was particularly long. I'd also have to release this PR in its current state in a major version bump since it's a breaking change from how it functioned previously

@menisy
Copy link
Author

menisy commented Oct 12, 2021

Okay I also thought about the cookie overflow and you do have a point there. I can change the implementation to remove the code and state params from the callback_url.
Please note that the current behaviour does not abide by the spec which clearly states that the redirect_uri specified in the request phase needs to exactly match the one being used in the callback phase and hence we need to fix this ASAP.

I'm also not sure whether we should consider this as a breaking change since the auth provider is (or should) already expect this given that this is the OAuth2 specification. I wouldn't release this in a patch version but I would consider maybe a minor version.

dshorthouse added a commit to dshorthouse/omniauth-oauth2 that referenced this pull request Jul 20, 2022
As in the subject line & perhaps a little easier to manage than omniauth#142.
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