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.

Sign up for GitHub

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

Jump to bottom

fix broken backward compatibility (callback_url) #82

Closed
wants to merge 1 commit into from
Closed

fix broken backward compatibility (callback_url) #82

wants to merge 1 commit into from

Conversation

zmajstor
Copy link

This is a fix for broken backward compatibility which was introduced with pull request #70

Some OAuth2 providers (like omniauth-google-oauth2) fails if callback_url is not the same in callback_phase and request_phase.
Prior to version 1.4.0 callback_url was overridden (and query_string was discarded) which was problem for @agrobbin and some others.
This pull request propose fix with new option include_query_string (by default set to false, to preserve backward compatibility) and those who needs full callback_url (with query_string) in the will set this option to true: include_query_string: true

@zmajstor zmajstor mentioned this pull request Oct 26, 2015
Closed
@zmajstor zmajstor changed the title fix broken callback_url (backward compatibility) fix broken backward compatibility (callback_url) Oct 26, 2015
@zmajstor
fix broken backward compatibility
3677e2e 
(by default, keep the same callback_url in callback_phase and request_phase)
@agrobbin
Copy link
Contributor

To me the better solution would be to have omniauth-google-oauth2 overwrite callback_url instead of the core omniauth-oauth2 doing so.

@zmajstor
Copy link
Author

@agrobbin yes, I agree, but currently, there might be more than 300 broken projects with this change introduced in version 1.4.0, see this comment:
2615267#commitcomment-13936507

@zmajstor
Copy link
Author

fix switched to the omniauth-google-oauth2 zquestz/omniauth-google-oauth2#205

@zmajstor zmajstor closed this Oct 26, 2015
@booleanbetrayal booleanbetrayal mentioned this pull request Oct 27, 2015
Closed
@iainbeeston iainbeeston mentioned this pull request Oct 30, 2015
Merged
@l8nite
Copy link

l8nite commented Feb 10, 2016

The inclusion of query parameters by default broke our production site for awhile today while we figured out what was happening. It seems the RFC doesn't stipulate the correct behavior for OAuth2 providers with regards to verifying a registered callback url vs the input. In our case, the provider we were using is explicitly verifying the query components of the URI in addition to the scheme, host, and path.

@carsonreinke carsonreinke mentioned this pull request Apr 25, 2016
Merged
JonathonMA added a commit to everydayhero/omniauth-passport that referenced this pull request Sep 9, 2016
@JonathonMA
Fix for omniauth-oauth 1.4.0
6276c15 
See omniauth/omniauth-oauth2#82
pjg added a commit to Hello-bar/omniauth-createsend that referenced this pull request Jun 21, 2017
@pjg
Fix compatibility with omniauth-oauth2 v1.4+
8c550ca 
References:
* omniauth/omniauth-oauth2#70
* omniauth/omniauth-oauth2@2615267
* omniauth/omniauth-oauth2#82
* jdennes#3
* WebTheoryLLC/omniauth-twitch#4
@pjg pjg mentioned this pull request Jun 21, 2017
Merged
pjg added a commit to Hello-bar/omniauth-drip that referenced this pull request Jul 3, 2017
@pjg
Fix compatibility with omniauth-oauth2 v1.4+
97e76d6 
References:
* omniauth/omniauth-oauth2#70
* omniauth/omniauth-oauth2@2615267
* omniauth/omniauth-oauth2#82
* jdennes/omniauth-createsend#3
* WebTheoryLLC/omniauth-twitch#4
@pjg pjg mentioned this pull request Jul 3, 2017
Merged
pjg added a commit to Hello-bar/omniauth-verticalresponse that referenced this pull request Jul 27, 2017
@pjg
Fix compatibility with omniauth-oauth 1.4+
416e1a6 
References:
* omniauth/omniauth-oauth2#70
* omniauth/omniauth-oauth2@2615267
* omniauth/omniauth-oauth2#82
* jdennes/omniauth-createsend#3
* WebTheoryLLC/omniauth-twitch#4
* jdennes/omniauth-createsend#5
* DripEmail/omniauth-drip#6
@pjg pjg mentioned this pull request Jul 27, 2017
Open
webgago added a commit to Hello-bar/omniauth-infusionsoft that referenced this pull request Sep 5, 2017
@webgago
Fix compatibility with omniauth-oauth 1.4+
05fdfeb 
References:
* omniauth/omniauth-oauth2#70
* omniauth/omniauth-oauth2@2615267
* omniauth/omniauth-oauth2#82
* jdennes/omniauth-createsend#3
* WebTheoryLLC/omniauth-twitch#4
* jdennes/omniauth-createsend#5
* DripEmail/omniauth-drip#6
@webgago webgago mentioned this pull request Sep 6, 2017
Merged
pjg added a commit to Hello-bar/omniauth-constantcontact2 that referenced this pull request Oct 6, 2017
@pjg
Fix compatibility with omniauth-oauth 1.4+
ba98609 
References:
* omniauth/omniauth-oauth2#70
* omniauth/omniauth-oauth2@2615267
* omniauth/omniauth-oauth2#82
* jdennes/omniauth-createsend#3
* WebTheoryLLC/omniauth-twitch#4
* jdennes/omniauth-createsend#5
* DripEmail/omniauth-drip#6
* Kosmin/omniauth-verticalresponse#3
@pjg pjg mentioned this pull request Oct 6, 2017
Open
marcboquet added a commit to Kit/omniauth-gumroad that referenced this pull request Dec 27, 2017
@marcboquet
Generate correct callback_url
e2fed32 
omniauth-oauth2 v1.4.0 stopped overriding callback_url, this brings back the default implementation.
omniauth/omniauth-oauth2@85fdbe1

It's the same fix as Shopify/omniauth-shopify-oauth2#32 and others:
omniauth/omniauth-oauth2#82 (reference)
@marcboquet marcboquet mentioned this pull request Dec 27, 2017
Merged
l1h3r pushed a commit to l1h3r/omniauth-infusionsoft that referenced this pull request Mar 16, 2018
@webgago @l1h3r
Fix compatibility with omniauth-oauth 1.4+ and allow http locally (#4)
d4be6d0 
* Fix compatibility with omniauth-oauth 1.4+

References:
* omniauth/omniauth-oauth2#70
* omniauth/omniauth-oauth2@2615267
* omniauth/omniauth-oauth2#82
* jdennes/omniauth-createsend#3
* WebTheoryLLC/omniauth-twitch#4
* jdennes/omniauth-createsend#5
* DripEmail/omniauth-drip#6

* Don't force https if it's localhost

* Allow 127.0.0.1 and localhost to be on http

* Lookup IP for the given host and force ssl

Avoid to force ssl for local IPs (127/8 network)

* Improve resolving

* Improve even more

* Fix regexp

* Allow fe80::* entirely
@ptoomey3
Copy link

ptoomey3 commented Dec 19, 2019 •
edited
Loading

I just stumbled onto this after running into the same issue other folks are having.

It seems the RFC doesn't stipulate the correct behavior for OAuth2 providers with regards to verifying a registered callback url vs the input.

I'm not sure that is true. Looking at the RFC: https://tools.ietf.org/html/rfc6749#section-4.1.3

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.

The RFC says that the redirect_uri you pass to the server when exchange code for access_token must be identical to the one in the original authorization request. The original "fix" in #70 doesn't do that. By including the query params it will in fact vary by definition since the first step in the flow won't have a code in the query string and the final step will.

@ptoomey3 ptoomey3 mentioned this pull request Dec 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zmajstor @agrobbin @l8nite @ptoomey3