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

Improve OmniAuth version check to allow anything from 1.0 forward #5327

Merged
merged 9 commits into from
Feb 1, 2021

Conversation

carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Jan 7, 2021

This should enable people to try OmniAuth 2 currently in pre-release.

Closes #5326, And I believe also closes #5236. (for now at least)

TO-DO

This should enable people to try OmniAuth 2 currently in pre-release.
@dcangulo
Copy link

Any chance that this will get merged? OmniAuth 2 has been released.

@carlosantoniodasilva
Copy link
Member Author

Please check the issue for more info. OmniAuth 2 was released yesterday, but the work here hasn't been finished. Thanks.

@BobbyMcWho
Copy link

BobbyMcWho commented Jan 12, 2021

@dcangulo It's also likely that some of your OmniAuth strategies will need updates, so I'd take a look at the strategies you use and make PRs where necessary to help the maintainers out.

@ohlhaver
Copy link

This should enable people to try OmniAuth 2 currently in pre-release.

Closes #5326

Not sure if this is the right place here but after upgrading to 8bb358c I am running into the "Not found. Authentication passthru." error when trying to authenticate with Facebook (using omniauth-facebook) which didn’t happen before the upgrade.

@carlosantoniodasilva
Copy link
Member Author

Thanks for testing and passing along the feedback @ohlhaver. I did notice a few tests failed on OmniAuth2 once I got it running locally, I'm gonna see if there's any change required in Devise to support it.

@geoffharcourt
Copy link

@ohlhaver is that authentication to Facebook via a GET request? I believe that's part of the resolution for the CVE that motivated the 2.0 release.

@ohlhaver
Copy link

@ohlhaver is that authentication to Facebook via a GET request? I believe that's part of the resolution for the CVE that motivated the 2.0 release.

Not absolutely sure. It's the call to user_facebook_omniauth_authorize_path without any further parameters, so I guess it's a GET request?

kirushik pushed a commit to kirushik/critical_chain that referenced this pull request Jan 18, 2021
But probably we're still blocked on
heartcombo/devise#5327
jkowens and others added 3 commits January 19, 2021 15:19
omniauth-openid v2.0.1 was just released opening support for omniauth
v2, so we can bundle update everything from the released gems now.
@carlosantoniodasilva
Copy link
Member Author

@ohlhaver if you're testing out this branch please make sure to get the latest, and let me know if you run into any trouble.

If you've customized the Devise shared links template, or have your own links/buttons to initiate the "Facebook OmniAuth" authorization, those need to be changed to do a POST request like mentioned above. See the diff for an example.

For links you can change them to use method: :post (and make sure you have something like rails or jquery ujs to handle those properly), or alternatively you can change to button_to or a manual form. Please take a look at the OmniAuth release info here.

@thebravoman
Copy link

For anyone now getting the error I resolved it with:

-gem "omniauth"
+gem "omniauth", "~> 1.9.1" # Can not move to 2.0 because of devise - https://github.com/heartcombo/devise/pull/5327

@yshmarov
Copy link

gem "omniauth", "~> 1.9.1" # Can not move to 2.0 because of devise - #5327

Indeed, it does help as a temporary solution

@carlosantoniodasilva
Copy link
Member Author

@thebravoman @yshmarov this is essentially preventing/ignoring the OmniAuth upgrade, if that's what you want to achieve for now?

If you'd like, you should be able to try out OmniAuth 2 by pointing Devise to this branch. If you happen to give it a shot, please report back with any feedback. Thanks.

@yshmarov
Copy link

First I tried this # gem 'devise', :git => "https://github.com/heartcombo/devise.git", ref: '8bb358cf80a632d3232c3f548ce7b95fd94b6eb2' as per https://stackoverflow.com/questions/65702896/latest-omniauth-facebook-gem-breaks-devise. It helped to start the server, but omniauth did not work with that.

@carlosantoniodasilva
Copy link
Member Author

I'd recommend using the branch ref instead of the git ref directly:

gem 'devise', github: 'heartcombo/devise', branch: 'ca-omniauth-2'

With that you should be able to run bundle update devise omniauth which should hopefully give you OmniAuth 2 and this Devise branch. That should allow the app to boot up.

Lastly, if you've copied over the Devise shared links on your app, or if you have your own links to initiate the OmniAuth authentication flow, you need to make sure they're changed to use a form. (you can do that by using link_to with method: :post option for example, or using button_to, if that works for your app.) Please note that this is a requirement change in how OmniAuth work due to a security issue, read more.

@thebravoman
Copy link

Oh yes @yshmarov @carlosantoniodasilva I also see this a temporary solution that is perfectly fine for me. I will happily wait for the next release of devise that is using omniauth-2.

unless OmniAuth::VERSION =~ /^1\./
raise "You are using an old OmniAuth version, please ensure you have 1.0.0.pr2 version or later installed."
if Gem::Version.new(OmniAuth::VERSION) < Gem::Version.new('1.0.0')
raise "You are using an old OmniAuth version, please ensure you have 1.0.0 version or later installed."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use gem 'omniauth', '>= 1.0.0.pr2' here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean use the gem command instead of the manual version check here? That's probably best yes, I'll update.

Or is it about the pr2 version? I thought we could just set to 1.0+ stable and don't allow the pr/rc ones anymore.

There was very little change between the two omniauth/omniauth@v1.0.0.pr2...v1.0.0, and hopefully there's no one else running those pre-stable versions.

And a note/warning about how it might break apps that don't update their
integration accordingly as OmniAuth now expects.
@CornerstoneII
Copy link

For anyone now getting the error I resolved it with:

-gem "omniauth"
+gem "omniauth", "~> 1.9.1" # Can not move to 2.0 because of devise - https://github.com/heartcombo/devise/pull/5327

Still works in 2021 yo!

@carlosantoniodasilva
Copy link
Member Author

@CornerstoneII well, this is all 2021 ;)

It'd be great if you could try out this branch though, to help make sure it works.

I'm gonna work on a new release containing these changes this week.

This reverts commit 628f2fb.

We should be run green on OmniAuth 2.x now.
I'm sure more people will hit issues so I'm trying to add more guidance
here about how to upgrade... maybe that should be in its own wiki but
I'll keep it all in the changelog for now.
@leni1
Copy link

leni1 commented Feb 10, 2021

Is it okay to still use this?

gem 'devise', github: 'heartcombo/devise', branch: 'ca-omniauth-2'

Or leaving out the branch name is fine?

@carlosantoniodasilva
Copy link
Member Author

@leni1 since this has been merged you can point to the master branch instead (omitting it should work too for now, I think.)

I kept the branch around to avoid breaking anyone already relying on it, but it will be deleted at some point so better to rely on master.

@thebravoman
Copy link

thebravoman commented Feb 17, 2021

I see that this is merged. When do you expect it will be released?

@carlosantoniodasilva
Copy link
Member Author

@thebravoman no specific ETA yet, please see here for more info: #5326 (comment)

@thebravoman
Copy link

@carlosantoniodasilva thanks. Looking forward to.

@PedroPedrassolli
Copy link

@carlosantoniodasilva No rush, but still no ETA?

schneems added a commit to codetriage/CodeTriage that referenced this pull request Mar 11, 2021
Email from GitHub

```
[GitHub API] Deprecation notice for authentication via URL query parameters
GitHub
	
Mar 6, 2021, 4:37 AM (5 days ago)
	
	
to Richard
Hi @schneems,

On March 6th, 2021 at 10:37 (UTC) your application (CodeTriage) used an access token (with the User-Agent Faraday v0.17.3) as part of a query parameter to access an endpoint through the GitHub API:

https://api.github.com/user

Please use the Authorization HTTP header instead as using the `access_token` query parameter is deprecated.

Depending on your API usage, we'll be sending you this email reminder on a monthly basis.

Visit https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param for more information about suggested workarounds and removal dates.

Thanks,
The GitHub Team
```

Need to use devise from GitHub due to this not being released yet heartcombo/devise#5327
@Mikopet
Copy link

Mikopet commented Mar 16, 2021

we don't have a new release yet containing these changes for OmniAuth.

Hello!

How soon it is possible to bump a new patch version with this fix?
It's interesting because omniauth 1.9.1 have a known security vulnerability, which is fixed in v2.0.0. Not a serious thing maybe, but dependabot is very dedicated against it.
And some companies security checks/workflow is broken now.

Perhaps, it is possible to release only this fix alone in a patch version?

According to my test the branch ca-omniauth-2 working well.

Can I do something to help you beyond the testing?

@carlosantoniodasilva
Copy link
Member Author

@Mikopet you can use devise master branch directly instead of this one. (I didn't delete it because I was pretty sure some people were relying on it directly, but it will eventually be removed)

The fix is bigger than a patch version would deserve I think. but I'm not ready to bump major, and I wanted to include a few other things before releasing a new version (minor in this case), but I haven't been able to wrap them up. I'll see about doing that release anyway with what's in master.

schneems added a commit to codetriage/CodeTriage that referenced this pull request Apr 4, 2021
Email from GitHub

```
[GitHub API] Deprecation notice for authentication via URL query parameters
GitHub

Mar 6, 2021, 4:37 AM (5 days ago)

to Richard
Hi @schneems,

On March 6th, 2021 at 10:37 (UTC) your application (CodeTriage) used an access token (with the User-Agent Faraday v0.17.3) as part of a query parameter to access an endpoint through the GitHub API:

https://api.github.com/user

Please use the Authorization HTTP header instead as using the `access_token` query parameter is deprecated.

Depending on your API usage, we'll be sending you this email reminder on a monthly basis.

Visit https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param for more information about suggested workarounds and removal dates.

Thanks,
The GitHub Team
```

Need to use devise from GitHub due to this not being released yet heartcombo/devise#5327
schneems added a commit to codetriage/CodeTriage that referenced this pull request Apr 4, 2021
Updates omniauth because GitHub is deprecating query params which is used by the old omniauth.

To do this I updated to Omniauth 2+ which also required an update of devise. There's a change to the omniauth API which is talked about here:

heartcombo/devise#5236

Basically:
- Omniauth2 requires post (instead of GET)
- Omniauth 2 also needs this `omniauth-rails_csrf_protection` gem.

I added the gem and updated all `link_to` and `button_to` to include a `method: :post`. There is one controller redirect which apparently still seems to work, but it might be broken. I'm not sure how we could possibly preserve the existing behavior since you cannot redirect to a post. 

This gets tests to pass though. So it's good enough for the short term.

## Deprecation Email from GitHub

```
[GitHub API] Deprecation notice for authentication via URL query parameters
GitHub

Mar 6, 2021, 4:37 AM (5 days ago)

to Richard
Hi @schneems,

On March 6th, 2021 at 10:37 (UTC) your application (CodeTriage) used an access token (with the User-Agent Faraday v0.17.3) as part of a query parameter to access an endpoint through the GitHub API:

https://api.github.com/user

Please use the Authorization HTTP header instead as using the `access_token` query parameter is deprecated.

Depending on your API usage, we'll be sending you this email reminder on a monthly basis.

Visit https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param for more information about suggested workarounds and removal dates.

Thanks,
The GitHub Team
```

Need to use devise from GitHub due to this not being released yet heartcombo/devise#5327

# This is the commit message #2:

WIP Move omniauth links to post

Omniauth 2 requires post. heartcombo/devise#5236

```
Install the gem OmniAuth - Rails CSRF Protection
Add the link user_facebook_omniauth_authorize_path method: :post
```

TODO: Convert the rest of the links to `user_github_omniauth_authorize_path` to be post
@alec-c4
Copy link

alec-c4 commented Apr 22, 2021

Hey! Any news with new release?

@carlosantoniodasilva
Copy link
Member Author

I just pushed v4.8 including this change, please give it a try and report back if you have any issues. Thanks.

@carlosantoniodasilva carlosantoniodasilva deleted the ca-omniauth-2 branch April 29, 2021 11:54
@md5
Copy link

md5 commented May 7, 2021

In our applications, we have been using a custom failure_app for many years to redirect the user directly to the omniauth_authorize_path instead of showing them the /sign_in page provided by the #new action in our subclass of Devise::SessionsController. This was done on the assumption that there is no reason for the user to be shown a page in the application only to have to click a button to get redirected to the identity provider's login page. The only time the user sees the sessions#new page with the "Sign in" button is after they explicitly log out.

However, it looks like the change in behavior in OmniAuth rendered our handling of this use case inoperable. Given that a GET will no longer work to initiate the request phase, we can't simply redirect to omniauth_authorize_path anymore. Instead, it looks like we're going to have to craft a page that contains a hidden form using POST and containing the CSRF token that is immediately submitted with JavaScript in order to send the user to the omniauth_authorize_path using a POST (where they will then be redirected to the SAML IdP's login page, since we're using omniauth-saml with our corporate SAML IdP).

Just thought I would add a comment here to help others who are in the same boat or in case some part of my analysis of the failures we're seeing after attempting to update to OmniAuth 2.0 is incorrect.

@carlosantoniodasilva
Copy link
Member Author

carlosantoniodasilva commented May 7, 2021

@md5 thanks for the mention here, that makes sense. Apologies for the extra work this is going to add to your team.

Please note that it is still possible to use GET, keeping the existing behavior, but it's really advisable not to, as that was the reason for the security issue in the first place. (see their release notes: https://github.com/omniauth/omniauth/releases/tag/v2.0.0 and the associated CVE info: https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284). There's also more information about how an attacker could use this to take control over someone else's account in this issue: omniauth/omniauth#809.

In the v2 release notes there's a section on how to use GET and how to prevent the warning OmniAuth will start raising then. You can do that, but like mentioned above, not super recommended, and if you do so it might be good to research alternatives to prevent the attack in question. (I don't know of anything of the top of my head).

h4l pushed a commit to cambridge-collection/spacefinder that referenced this pull request Feb 2, 2022
this is required to sidestep the fact that Devise has compatible Omniauth
versions hardcoded as '1.x.x' and Omniauth has recently gone '2.0.x'.

the Devise maintainer is planning a release that will include the fix for
this, in the meantime advised for people to keep using this branch.
See heartcombo/devise#5327 for more details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Devise is hard-coded to OmniAuth 1.x.x Error: Not found. Authentication passthru. with any Omniauth strategy