diff options
25 files changed, 169 insertions, 39 deletions
diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue index d4d3038f066..5a6b1c19027 100644 --- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/not_diffable.vue @@ -1,5 +1,5 @@ <template> <div class="nothing-here-block"> - {{ __('This diff was suppressed by a .gitattributes entry.') }} + {{ __("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") }} </div> </template> diff --git a/app/models/concerns/integrations/slack_mattermost_notifier.rb b/app/models/concerns/integrations/slack_mattermost_notifier.rb index a919fc840fd..cb6fafa8de0 100644 --- a/app/models/concerns/integrations/slack_mattermost_notifier.rb +++ b/app/models/concerns/integrations/slack_mattermost_notifier.rb @@ -17,7 +17,7 @@ module Integrations 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/integrations/bamboo.rb b/app/models/integrations/bamboo.rb index dbd7aedf4fe..fef2774c593 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/integrations/base_issue_tracker.rb b/app/models/integrations/base_issue_tracker.rb index 6c24f762cd5..3fd67205e92 100644 --- a/app/models/integrations/base_issue_tracker.rb +++ b/app/models/integrations/base_issue_tracker.rb @@ -107,7 +107,7 @@ module Integrations 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/integrations/drone_ci.rb b/app/models/integrations/drone_ci.rb index 096f7093b8c..0f021356815 100644 --- a/app/models/integrations/drone_ci.rb +++ b/app/models/integrations/drone_ci.rb @@ -51,9 +51,12 @@ module Integrations 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/integrations/external_wiki.rb b/app/models/integrations/external_wiki.rb index fec435443fa..2a8d598117b 100644 --- a/app/models/integrations/external_wiki.rb +++ b/app/models/integrations/external_wiki.rb @@ -39,7 +39,7 @@ module Integrations 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/integrations/mock_ci.rb b/app/models/integrations/mock_ci.rb index d31f6381767..a0eae9e4abf 100644 --- a/app/models/integrations/mock_ci.rb +++ b/app/models/integrations/mock_ci.rb @@ -55,7 +55,7 @@ module Integrations # # => 'running' # 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/integrations/teamcity.rb b/app/models/integrations/teamcity.rb index 8284d5963ae..3f14c5d82b3 100644 --- a/app/models/integrations/teamcity.rb +++ b/app/models/integrations/teamcity.rb @@ -170,7 +170,7 @@ module Integrations 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) @@ -180,7 +180,8 @@ module Integrations "<buildType id=#{build_type.encode(xml: :attr)}/>"\ '</build>', headers: { 'Content-type' => 'application/xml' }, - basic_auth: basic_auth + basic_auth: basic_auth, + use_read_total_timeout: true ) end diff --git a/app/models/integrations/unify_circuit.rb b/app/models/integrations/unify_circuit.rb index 03363c7c8b0..834222834e9 100644 --- a/app/models/integrations/unify_circuit.rb +++ b/app/models/integrations/unify_circuit.rb @@ -49,7 +49,8 @@ module Integrations 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/integrations/webex_teams.rb b/app/models/integrations/webex_teams.rb index 3f420331035..6fd82a32035 100644 --- a/app/models/integrations/webex_teams.rb +++ b/app/models/integrations/webex_teams.rb @@ -44,7 +44,7 @@ module Integrations 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/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb index ea51dca8a42..5248834a2f2 100644 --- a/app/models/protected_branch/push_access_level.rb +++ b/app/models/protected_branch/push_access_level.rb @@ -20,7 +20,7 @@ class ProtectedBranch::PushAccessLevel < ApplicationRecord def check_access(user) if user && deploy_key.present? - return true if user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user) + return user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user) end super diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 77d2139b3d1..1d5b38575bb 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -42,6 +42,7 @@ class WebHookService @uniqueness_token = uniqueness_token @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout, + use_read_total_timeout: true, allow_local_requests: hook.allow_local_requests? } end @@ -68,7 +69,7 @@ class WebHookService { status: :success, http_status: response.code, - message: response.to_s + message: response.body } rescue *Gitlab::HTTP::HTTP_ERRORS, Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError => e diff --git a/app/views/projects/diffs/viewers/_not_diffable.html.haml b/app/views/projects/diffs/viewers/_not_diffable.html.haml index 7c55e272f56..63034331f6a 100644 --- a/app/views/projects/diffs/viewers/_not_diffable.html.haml +++ b/app/views/projects/diffs/viewers/_not_diffable.html.haml @@ -1,2 +1,2 @@ .nothing-here-block - = _("This diff was suppressed by a .gitattributes entry.") + = _("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index dcd4bbdabf5..35581952f4a 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -250,7 +250,7 @@ module Gitlab end def diffable? - repository.attributes(file_path).fetch('diff') { true } + diffable_by_attribute? && !text_with_binary_notice? end def binary_in_repo? @@ -366,6 +366,15 @@ module Gitlab private + def diffable_by_attribute? + repository.attributes(file_path).fetch('diff') { true } + end + + # NOTE: Files with unsupported encodings (e.g. UTF-16) are treated as binary by git, but they are recognized as text files during encoding detection. These files have `Binary files a/filename and b/filename differ' as their raw diff content which cannot be used. We need to handle this special case and avoid displaying incorrect diff. + def text_with_binary_notice? + text? && has_binary_notice? + end + def fetch_blob(sha, path) return unless sha diff --git a/lib/gitlab/diff/parser.rb b/lib/gitlab/diff/parser.rb index 4a47e4b80b6..adb711ca89f 100644 --- a/lib/gitlab/diff/parser.rb +++ b/lib/gitlab/diff/parser.rb @@ -6,7 +6,7 @@ module Gitlab include Enumerable def parse(lines, diff_file: nil) - return [] if lines.blank? + return [] if lines.blank? || Git::Diff.has_binary_notice?(lines.first) @lines = lines line_obj_index = 0 diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 53df0b7b389..8325eadce2f 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -33,6 +33,8 @@ module Gitlab SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze + BINARY_NOTICE_PATTERN = %r(Binary files a\/(.*) and b\/(.*) differ).freeze + class << self def between(repo, head, base, options = {}, *paths) straight = options.delete(:straight) || false @@ -131,8 +133,13 @@ module Gitlab def patch_hard_limit_bytes Gitlab::CurrentSettings.diff_max_patch_bytes end - end + def has_binary_notice?(text) + return false unless text.present? + + text.start_with?(BINARY_NOTICE_PATTERN) + end + end def initialize(raw_diff, expanded: true) @expanded = expanded @@ -215,7 +222,7 @@ module Gitlab end def has_binary_notice? - @diff.start_with?('Binary') + self.class.has_binary_notice?(@diff) end private 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/locale/gitlab.pot b/locale/gitlab.pot index f34bea613e8..b82dc3d5259 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14032,6 +14032,9 @@ msgstr "" msgid "File renamed with no changes." msgstr "" +msgid "File suppressed by a .gitattributes entry or the file's encoding is unsupported." +msgstr "" + msgid "File synchronization concurrency limit" msgstr "" @@ -33273,9 +33276,6 @@ msgstr "" msgid "This diff is collapsed." msgstr "" -msgid "This diff was suppressed by a .gitattributes entry." -msgstr "" - msgid "This directory" msgstr "" diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index cbd1ae628d1..add4af2bcdb 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -253,7 +253,7 @@ RSpec.describe 'Expand and collapse diffs', :js do click_link('Expand all') # Wait for elements to appear to ensure full page reload - expect(page).to have_content('This diff was suppressed by a .gitattributes entry') + expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") expect(page).to have_content('This source diff could not be displayed because it is too large.') expect(page).to have_content('too_large_image.jpg') find('.note-textarea') diff --git a/spec/features/projects/diffs/diff_show_spec.rb b/spec/features/projects/diffs/diff_show_spec.rb index e47f36c4b7a..56506ada3ce 100644 --- a/spec/features/projects/diffs/diff_show_spec.rb +++ b/spec/features/projects/diffs/diff_show_spec.rb @@ -174,4 +174,14 @@ RSpec.describe 'Diff file viewer', :js, :with_clean_rails_cache do end end end + + context 'when the the encoding of the file is unsupported' do + before do + visit_commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') + end + + it 'shows it is not diffable' do + expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") + end + end end diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb index 03a9b9bd21e..d401c42fed7 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiff do it 'does not highlight files marked as undiffable in .gitattributes' do allow_next_instance_of(Gitlab::Diff::File) do |instance| - allow(instance).to receive(:diffable?).and_return(false) + allow(instance).to receive(:diffable_by_attribute?).and_return(false) end expect_next_instance_of(Gitlab::Diff::File) do |instance| diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 78be89c449b..1800d2d6b60 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -186,26 +186,46 @@ RSpec.describe Gitlab::Diff::File do end describe '#diffable?' do - let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } - let(:diffs) { commit.diffs } + context 'when attributes exist' do + let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } + let(:diffs) { commit.diffs } - before do - info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do - File.join(project.repository.path_to_repo, 'info') + before do + info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + File.join(project.repository.path_to_repo, 'info') + end + + FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path) + File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n") end - FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path) - File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n") + it "returns true for files that do not have attributes" do + diff_file = diffs.diff_file_with_new_path('LICENSE') + expect(diff_file.diffable?).to be_truthy + end + + it "returns false for files that have been marked as not being diffable in attributes" do + diff_file = diffs.diff_file_with_new_path('README.md') + expect(diff_file.diffable?).to be_falsey + end end - it "returns true for files that do not have attributes" do - diff_file = diffs.diff_file_with_new_path('LICENSE') - expect(diff_file.diffable?).to be_truthy + context 'when the text has binary notice' do + let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') } + + it "returns false" do + expect(diff_file.diffable?).to be_falsey + end end - it "returns false for files that have been marked as not being diffable in attributes" do - diff_file = diffs.diff_file_with_new_path('README.md') - expect(diff_file.diffable?).to be_falsey + context 'when the content is binary' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + it "returns true" do + expect(diff_file.diffable?).to be_truthy + end end end @@ -729,6 +749,18 @@ RSpec.describe Gitlab::Diff::File do end end + context 'when the the encoding of the file is unsupported' do + let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') } + + it 'returns a Not Diffable viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::NotDiffable) + end + + it { expect(diff_file.highlighted_diff_lines).to eq([]) } + it { expect(diff_file.parallel_diff_lines).to eq([]) } + end + describe '#diff_hunk' do context 'when first line is a match' do let(:raw_diff) do diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index 7448ae0b2ea..c8069f82f04 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/spec/lib/gitlab/diff/parser_spec.rb @@ -146,6 +146,16 @@ eos it { expect(parser.parse(nil)).to eq([]) } end + context 'when it is a binary notice' do + let(:diff) do + <<~END + Binary files a/test and b/test differ + END + end + + it { expect(parser.parse(diff.each_line)).to eq([]) } + end + describe 'tolerates special diff markers in a content' do it "counts lines correctly" do diff = <<~END 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') diff --git a/spec/models/protected_branch/push_access_level_spec.rb b/spec/models/protected_branch/push_access_level_spec.rb index 17a589f0485..fa84cd660cb 100644 --- a/spec/models/protected_branch/push_access_level_spec.rb +++ b/spec/models/protected_branch/push_access_level_spec.rb @@ -44,7 +44,7 @@ RSpec.describe ProtectedBranch::PushAccessLevel do let(:can_push) { true } before_all do - project.add_guest(user) + project.add_maintainer(user) end context 'when this push_access_level is tied to a deploy key' do |