omniauth / omniauth-oauth2

An abstract OAuth2 strategy for OmniAuth.
MIT License
502 stars 304 forks source link

Reuse callback_url between request phase and callback phase #142

Open menisy opened 3 years ago

menisy commented 3 years ago

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 commented 3 years ago

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

BobbyMcWho commented 3 years ago

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 commented 3 years ago

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.