From ed903367e78907253ca4319a7f22cf49cbcd048d Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 30 Jun 2021 11:40:21 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- app/models/integrations/bamboo.rb | 1 + app/models/project_services/drone_ci_service.rb | 6 ++-- .../project_services/external_wiki_service.rb | 2 +- .../project_services/issue_tracker_service.rb | 2 +- app/models/project_services/mock_ci_service.rb | 2 +- .../project_services/slack_mattermost/notifier.rb | 2 +- app/models/project_services/teamcity_service.rb | 5 +-- .../project_services/unify_circuit_service.rb | 3 +- app/models/project_services/webex_teams_service.rb | 2 +- app/services/web_hook_service.rb | 3 +- lib/gitlab/http.rb | 18 ++++++++-- spec/lib/gitlab/http_spec.rb | 41 ++++++++++++++++++++++ 12 files changed, 74 insertions(+), 13 deletions(-) diff --git a/app/models/integrations/bamboo.rb b/app/models/integrations/bamboo.rb index 82111c7322e..5f43eae1e8f 100644 --- a/app/models/integrations/bamboo.rb +++ b/app/models/integrations/bamboo.rb @@ -173,6 +173,7 @@ module Integrations query_params[:os_authType] = 'basic' params[:basic_auth] = basic_auth + params[:use_read_total_timeout] = true params end diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb index ab1ba768a8f..581bd8077ad 100644 --- a/app/models/project_services/drone_ci_service.rb +++ b/app/models/project_services/drone_ci_service.rb @@ -50,9 +50,11 @@ class DroneCiService < CiService end def calculate_reactive_cache(sha, ref) - response = Gitlab::HTTP.try_get(commit_status_path(sha, ref), + response = Gitlab::HTTP.try_get( + commit_status_path(sha, ref), verify: enable_ssl_verification, - extra_log_info: { project_id: project_id }) + extra_log_info: { project_id: project_id }, + use_read_total_timeout: true) status = if response && response.code == 200 && response['status'] diff --git a/app/models/project_services/external_wiki_service.rb b/app/models/project_services/external_wiki_service.rb index f49b008533d..6798b8fb695 100644 --- a/app/models/project_services/external_wiki_service.rb +++ b/app/models/project_services/external_wiki_service.rb @@ -38,7 +38,7 @@ class ExternalWikiService < Integration end def execute(_data) - response = Gitlab::HTTP.get(properties['external_wiki_url'], verify: true) + response = Gitlab::HTTP.get(properties['external_wiki_url'], verify: true, use_read_total_timeout: true) response.body if response.code == 200 rescue StandardError nil diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index 099e3c336dd..ba78b1a8dc3 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/app/models/project_services/issue_tracker_service.rb @@ -106,7 +106,7 @@ class IssueTrackerService < Integration result = false begin - response = Gitlab::HTTP.head(self.project_url, verify: true) + response = Gitlab::HTTP.head(self.project_url, verify: true, use_read_total_timeout: true) if response message = "#{self.type} received response #{response.code} when attempting to connect to #{self.project_url}" diff --git a/app/models/project_services/mock_ci_service.rb b/app/models/project_services/mock_ci_service.rb index bd6344c6e1a..ebe6179d742 100644 --- a/app/models/project_services/mock_ci_service.rb +++ b/app/models/project_services/mock_ci_service.rb @@ -56,7 +56,7 @@ class MockCiService < CiService # # def commit_status(sha, ref) - response = Gitlab::HTTP.get(commit_status_path(sha), verify: false) + response = Gitlab::HTTP.get(commit_status_path(sha), verify: false, use_read_total_timeout: true) read_commit_status(response) rescue Errno::ECONNREFUSED :error diff --git a/app/models/project_services/slack_mattermost/notifier.rb b/app/models/project_services/slack_mattermost/notifier.rb index 1a78cea5933..39ee5718248 100644 --- a/app/models/project_services/slack_mattermost/notifier.rb +++ b/app/models/project_services/slack_mattermost/notifier.rb @@ -17,7 +17,7 @@ module SlackMattermost class HTTPClient def self.post(uri, params = {}) params.delete(:http_options) # these are internal to the client and we do not want them - Gitlab::HTTP.post(uri, body: params) + Gitlab::HTTP.post(uri, body: params, use_read_total_timeout: true) end end end diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index 6fc24a4778c..c3390dd9c25 100644 --- a/app/models/project_services/teamcity_service.rb +++ b/app/models/project_services/teamcity_service.rb @@ -169,7 +169,7 @@ class TeamcityService < CiService end def get_path(path) - Gitlab::HTTP.try_get(build_url(path), verify: false, basic_auth: basic_auth, extra_log_info: { project_id: project_id }) + Gitlab::HTTP.try_get(build_url(path), verify: false, basic_auth: basic_auth, extra_log_info: { project_id: project_id }, use_read_total_timeout: true) end def post_to_build_queue(data, branch) @@ -179,7 +179,8 @@ class TeamcityService < CiService ""\ '', headers: { 'Content-type' => 'application/xml' }, - basic_auth: basic_auth + basic_auth: basic_auth, + use_read_total_timeout: true ) end diff --git a/app/models/project_services/unify_circuit_service.rb b/app/models/project_services/unify_circuit_service.rb index 5f43388e1c9..f4e93cf8448 100644 --- a/app/models/project_services/unify_circuit_service.rb +++ b/app/models/project_services/unify_circuit_service.rb @@ -48,7 +48,8 @@ class UnifyCircuitService < ChatNotificationService response = Gitlab::HTTP.post(webhook, body: { subject: message.project_name, text: message.summary, - markdown: true + markdown: true, + use_read_total_timeout: true }.to_json) response if response.success? diff --git a/app/models/project_services/webex_teams_service.rb b/app/models/project_services/webex_teams_service.rb index 3d92d3bb85e..37f8a1af6f1 100644 --- a/app/models/project_services/webex_teams_service.rb +++ b/app/models/project_services/webex_teams_service.rb @@ -43,7 +43,7 @@ class WebexTeamsService < ChatNotificationService def notify(message, opts) header = { 'Content-Type' => 'application/json' } - response = Gitlab::HTTP.post(webhook, headers: header, body: { markdown: message.summary }.to_json) + response = Gitlab::HTTP.post(webhook, headers: header, body: { markdown: message.summary }.to_json, use_read_total_timeout: true) response if response.success? end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 654d9356739..c2f12c133c1 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -41,6 +41,7 @@ class WebHookService @hook_name = hook_name.to_s @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout, + use_read_total_timeout: true, allow_local_requests: hook.allow_local_requests? } end @@ -67,7 +68,7 @@ class WebHookService { status: :success, http_status: response.code, - message: response.to_s + message: response.body } rescue SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::EHOSTUNREACH, Net::OpenTimeout, Net::ReadTimeout, Gitlab::HTTP::BlockedUrlError, Gitlab::HTTP::RedirectionTooDeep, diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb index be87dcc0ff9..7e45cd216f5 100644 --- a/lib/gitlab/http.rb +++ b/lib/gitlab/http.rb @@ -8,9 +8,10 @@ module Gitlab class HTTP BlockedUrlError = Class.new(StandardError) RedirectionTooDeep = Class.new(StandardError) + ReadTotalTimeout = Class.new(Net::ReadTimeout) HTTP_TIMEOUT_ERRORS = [ - Net::OpenTimeout, Net::ReadTimeout, Net::WriteTimeout + Net::OpenTimeout, Net::ReadTimeout, Net::WriteTimeout, Gitlab::HTTP::ReadTotalTimeout ].freeze HTTP_ERRORS = HTTP_TIMEOUT_ERRORS + [ SocketError, OpenSSL::SSL::SSLError, OpenSSL::OpenSSLError, @@ -23,6 +24,7 @@ module Gitlab read_timeout: 20, write_timeout: 30 }.freeze + DEFAULT_READ_TOTAL_TIMEOUT = 20.seconds include HTTParty # rubocop:disable Gitlab/HTTParty @@ -41,7 +43,19 @@ module Gitlab options end - httparty_perform_request(http_method, path, options_with_timeouts, &block) + unless options.has_key?(:use_read_total_timeout) + return httparty_perform_request(http_method, path, options_with_timeouts, &block) + end + + start_time = Gitlab::Metrics::System.monotonic_time + read_total_timeout = options.fetch(:timeout, DEFAULT_READ_TOTAL_TIMEOUT) + + httparty_perform_request(http_method, path, options_with_timeouts) do |fragment| + elapsed = Gitlab::Metrics::System.monotonic_time - start_time + raise ReadTotalTimeout, "Request timed out after #{elapsed} seconds" if elapsed > read_total_timeout + + block.call fragment if block + end rescue HTTParty::RedirectionTooDeep raise RedirectionTooDeep rescue *HTTP_ERRORS => e diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index 308f7f46251..71e80de9f89 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -27,6 +27,47 @@ RSpec.describe Gitlab::HTTP do end end + context 'when reading the response is too slow' do + before do + stub_const("#{described_class}::DEFAULT_READ_TOTAL_TIMEOUT", 0.001.seconds) + + WebMock.stub_request(:post, /.*/).to_return do |request| + sleep 0.002.seconds + { body: 'I\m slow', status: 200 } + end + end + + let(:options) { {} } + + subject(:request_slow_responder) { described_class.post('http://example.org', **options) } + + specify do + expect { request_slow_responder }.not_to raise_error + end + + context 'with use_read_total_timeout option' do + let(:options) { { use_read_total_timeout: true } } + + it 'raises a timeout error' do + expect { request_slow_responder }.to raise_error(Gitlab::HTTP::ReadTotalTimeout, /Request timed out after ?([0-9]*[.])?[0-9]+ seconds/) + end + + context 'and timeout option' do + let(:options) { { use_read_total_timeout: true, timeout: 10.seconds } } + + it 'overrides the default timeout when timeout option is present' do + expect { request_slow_responder }.not_to raise_error + end + end + end + end + + it 'calls a block' do + WebMock.stub_request(:post, /.*/) + + expect { |b| described_class.post('http://example.org', &b) }.to yield_with_args + end + describe 'allow_local_requests_from_web_hooks_and_services is' do before do WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') -- cgit v1.2.3