diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 12:55:30 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 12:55:39 +0000 |
commit | 5b9d26900b7201f71e584c331f833ddfe43ad8f2 (patch) | |
tree | 00002669df6c47e95a8478aed0db4bc958f5172b | |
parent | 848e7081180b0a92222c717a145b07c396b42f22 (diff) | |
download | gitlab-ce-5b9d26900b7201f71e584c331f833ddfe43ad8f2.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-2-stable-ee
-rw-r--r-- | config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb | 42 | ||||
-rw-r--r-- | doc/api/dependencies.md | 10 | ||||
-rw-r--r-- | spec/initializers/100_patch_omniauth_oauth2_spec.rb | 56 |
3 files changed, 103 insertions, 5 deletions
diff --git a/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb b/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb index 760fcba5935..1ede92609a9 100644 --- a/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb +++ b/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb @@ -1,14 +1,46 @@ # 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 diff --git a/doc/api/dependencies.md b/doc/api/dependencies.md index c8b928ab5b2..6e9c37980ac 100644 --- a/doc/api/dependencies.md +++ b/doc/api/dependencies.md @@ -11,6 +11,9 @@ This API is in an alpha stage and considered unstable. The response payload may be subject to change or breakage across GitLab releases. +> - Introduced in GitLab 12.1. +> - Pagination introduced in 14.4. + Every call to this endpoint requires authentication. To perform this call, user should be authorized to read repository. To see vulnerabilities in response, user should be authorized to read [Project Security Dashboard](../user/application_security/security_dashboard/index.md#project-security-dashboard). @@ -60,3 +63,10 @@ Example response: } ] ``` + +## Dependencies pagination + +By default, `GET` requests return 20 results at a time because the API results +are paginated. + +Read more on [pagination](index.md#pagination). diff --git a/spec/initializers/100_patch_omniauth_oauth2_spec.rb b/spec/initializers/100_patch_omniauth_oauth2_spec.rb new file mode 100644 index 00000000000..0c436e4ef45 --- /dev/null +++ b/spec/initializers/100_patch_omniauth_oauth2_spec.rb @@ -0,0 +1,56 @@ +# 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 |