Skip to content

Bump omniauth-oauth2 version#3

Merged
jsspiegel merged 2 commits intomasterfrom
bump-omniauth-oauth2-version
Apr 4, 2023
Merged

Bump omniauth-oauth2 version#3
jsspiegel merged 2 commits intomasterfrom
bump-omniauth-oauth2-version

Conversation

@jsspiegel
Copy link

@jsspiegel jsspiegel commented Apr 3, 2023

https://zearn.kanbanize.com/ctrl_board/51/cards/5566/details/

Bump the highest allowed omniauth-oauth2 version from <= 1.5 to < 2.0 to resolve a dependabot issue we're having in our main repo.

@jsspiegel jsspiegel requested review from Strasserl and ruddzw April 3, 2023 14:13
@jsspiegel jsspiegel force-pushed the bump-omniauth-oauth2-version branch from 9362fbc to d98411e Compare April 3, 2023 14:20
@jsspiegel
Copy link
Author

To give a quick summary of the dependabot issue and how this fixes it, in our main repo we currently use omniauth-oauth2 v1.5, which uses omniauth v1.2, which is insecure. omniauth v2 is the first secure version, and omniauth-oauth2 v1.8 is both the latest and only version that uses omniauth v2, so we want to update to that. omniauth-classlink and omniauth-google-oauth2 are both already capable of supporting omniauth-oauth2 v1.8 (the versions they require are ~> 1.4 and >= 1.5, respectively). omniauth-clever is the issue, as it currently requires versions >= 1.1, <= 1.5 of omniauth-oauth2, and this PR bumps the highest allowed version to < 2.0. omniauth-oauth2 v1.8 is the currently the latest version, but setting the highest allowed version to < 2.0 should be safe, as minor version updates shouldn't introduce any breaking changes. For reference, this PR was based on this open PR on the main omniauth-clever repo.

Copy link

@ruddzw ruddzw left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Though I'm pretty sure that '>= 1.1', '<=2.0' is equivalent to the shorter '~> 1.1', so you may want to consider that.

Btw, as a full resolution to the issue, did you look at https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284 ? Seems like avoiding GET requests might be important for actually avoiding any security issue. I know that can't be part of this PR, but just checking anyway.

@jsspiegel
Copy link
Author

Seems fine to me. Though I'm pretty sure that '>= 1.1', '<=2.0' is equivalent to the shorter '~> 1.1', so you may want to consider that.

Btw, as a full resolution to the issue, did you look at https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284 ? Seems like avoiding GET requests might be important for actually avoiding any security issue. I know that can't be part of this PR, but just checking anyway.

Switched to the ~> syntax, thanks! In terms of avoiding GET requests, are you saying we need to upgrade to a non-vulnerable version of omniauth and avoid GET requests, or that either one of those alone is enough to avoid the vulnerability?

@ruddzw
Copy link

ruddzw commented Apr 4, 2023

@jsspiegel I didn't look too far into it but it seems like both are required

@jsspiegel
Copy link
Author

As discussed on Slack, the game plan for fixing this security vulnerability is going to be:

  1. Upgrade omniauth to v2.0 in our Dent repo, via the upgrade in this PR.
  2. Install omniauth-rails_csrf_protection in our Dent repo.
  3. Modify our front-end to change the SSO sign-in links to buttons that send a POST request.

@jsspiegel jsspiegel merged commit bfad502 into master Apr 4, 2023
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.

3 participants