diff --git a/Gemfile.lock b/Gemfile.lock index 55f411f6a1fd935725fb14fe1efd3d9e6fdb4e5a..c15e65efdd7e969cc2082c4213bbc14b41eb1e90 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -860,7 +860,7 @@ GEM omniauth-oauth (1.1.0) oauth omniauth (~> 1.0) - omniauth-oauth2 (1.7.1) + omniauth-oauth2 (1.7.2) oauth2 (~> 1.4) omniauth (>= 1.9, < 3) omniauth-oauth2-generic (0.2.2) diff --git a/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb b/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb index 1ede92609a90cc984f8ed8b366931ab35691bfed..c6baae56d3d7509a0ce7d07ec84b508fc73a6685 100644 --- a/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb +++ b/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb @@ -10,31 +10,11 @@ module OmniAuth module Strategies class OAuth2 + alias_method :original_callback_phase, :callback_phase + def callback_phase - error = request.params["error_reason"].presence || request.params["error"].presence - # Monkey patch #1: - # - # Swap the order of these conditions around so the `state` param is verified *first*, - # before using the error params returned by the provider. - # - # This avoids content spoofing attacks by crafting a URL with malicious messages, - # because the `state` param is only present in the session after a valid OAuth2 authentication flow. - if !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state")) - fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) - elsif error - fail!(error, CallbackError.new(request.params["error"], request.params["error_description"].presence || request.params["error_reason"].presence, request.params["error_uri"])) - else - self.access_token = build_access_token - self.access_token = access_token.refresh! if access_token.expired? - super - end - rescue ::OAuth2::Error, CallbackError => e - fail!(:invalid_credentials, e) - rescue ::Timeout::Error, ::Errno::ETIMEDOUT => e - fail!(:timeout, e) - rescue ::SocketError => e - fail!(:failed_to_connect, e) - # Monkey patch #2: + original_callback_phase + # Monkey patch #1: # # Also catch errors from Faraday. # See https://github.com/omniauth/omniauth-oauth2/pull/129 diff --git a/spec/initializers/100_patch_omniauth_oauth2_spec.rb b/spec/initializers/100_patch_omniauth_oauth2_spec.rb index 0c436e4ef4594c04444957424ff8711c99280bc4..c30a1cdeafad52e4516e63351e21882de6df431f 100644 --- a/spec/initializers/100_patch_omniauth_oauth2_spec.rb +++ b/spec/initializers/100_patch_omniauth_oauth2_spec.rb @@ -2,12 +2,10 @@ require 'spec_helper' -RSpec.describe 'OmniAuth::Strategies::OAuth2', type: :strategy do - let(:strategy) { [OmniAuth::Strategies::OAuth2] } - +RSpec.describe 'OmniAuth::Strategies::OAuth2' do it 'verifies the gem version' do current_version = OmniAuth::OAuth2::VERSION - expected_version = '1.7.1' + expected_version = '1.7.2' expect(current_version).to eq(expected_version), <<~EOF New version #{current_version} of the `omniauth-oauth2` gem detected! @@ -18,39 +16,18 @@ EOF end - context 'when a custom error message is passed from an OAuth2 provider' do - let(:message) { 'Please go to https://evil.com' } - let(:state) { 'secret' } - let(:callback_path) { '/users/auth/oauth2/callback' } - let(:params) { { state: state, error: 'evil_key', error_description: message } } - let(:error) { last_request.env['omniauth.error'] } - - before do - env('rack.session', { 'omniauth.state' => state }) - end - - it 'returns the custom error message if the state is valid' do - get callback_path, **params - - expect(error.message).to eq("evil_key | #{message}") - end + context 'when a Faraday exception is raised' do + where(exception: [Faraday::TimeoutError, Faraday::ConnectionFailed]) - it 'returns the custom `error_reason` message if the `error_description` is blank' do - get callback_path, **params.merge(error_description: ' ', error_reason: 'custom reason') - - expect(error.message).to eq('evil_key | custom reason') - end - - it 'returns a CSRF error if the state is invalid' do - get callback_path, **params.merge(state: 'invalid') - - expect(error.message).to eq('csrf_detected | CSRF detected') - end + with_them do + it 'passes the exception to OmniAuth' do + instance = OmniAuth::Strategies::OAuth2.new(double) - it 'returns a CSRF error if the state is missing' do - get callback_path, **params.without(:state) + expect(instance).to receive(:original_callback_phase) { raise exception, 'message' } + expect(instance).to receive(:fail!).with(:timeout, kind_of(exception)) - expect(error.message).to eq('csrf_detected | CSRF detected') + instance.callback_phase + end end end end