Verify state before using errors from OAuth2 OmniAuth providers

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.

Changelog: security

config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb

 # frozen_string_literal: true
 
+# See https://github.com/omniauth/omniauth-oauth2/blob/v1.7.1/lib/omniauth/strategies/oauth2.rb#L84-L101
+# for the original version of this code.
+#
+# Note: We need to override `callback_phase` directly (instead of using a module with `include` or `prepend`),
+# because the method has a `super` call which needs to go to the `OmniAuth::Strategy` module,
+# and it also deletes `omniauth.state` from the session as a side effect.
+
 module OmniAuth
   module Strategies
     class OAuth2
-      alias_method :original_callback_phase, :callback_phase
-
-      # Monkey patch until PR is merged and released upstream
-      # https://github.com/omniauth/omniauth-oauth2/pull/129
       def callback_phase
-        original_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:
+      #
+      # Also catch errors from Faraday.
+      # See https://github.com/omniauth/omniauth-oauth2/pull/129
+      # and https://github.com/oauth-xx/oauth2/issues/152
+      #
+      # This can be removed with https://gitlab.com/gitlab-org/gitlab/-/issues/340933
       rescue ::Faraday::TimeoutError, ::Faraday::ConnectionFailed => e
         fail!(:timeout, e)
       end

spec/initializers/100_patch_omniauth_oauth2_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'OmniAuth::Strategies::OAuth2', type: :strategy do
+  let(:strategy) { [OmniAuth::Strategies::OAuth2] }
+
+  it 'verifies the gem version' do
+    current_version = OmniAuth::OAuth2::VERSION
+    expected_version = '1.7.1'
+
+    expect(current_version).to eq(expected_version), <<~EOF
+      New version #{current_version} of the `omniauth-oauth2` gem detected!
+
+      Please check if the monkey patches in `config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb`
+      are still needed, and either update/remove them, or bump the version in this spec.
+
+    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
+
+    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
+
+    it 'returns a CSRF error if the state is missing' do
+      get callback_path, **params.without(:state)
+
+      expect(error.message).to eq('csrf_detected | CSRF detected')
+    end
+  end
+end