diff options
-rw-r--r-- | .gitlab/ci/review.gitlab-ci.yml | 1 | ||||
-rw-r--r-- | .rubocop.yml | 3 | ||||
-rw-r--r-- | app/models/list.rb | 2 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | app/services/metrics/dashboard/clone_dashboard_service.rb | 8 | ||||
-rw-r--r-- | app/services/metrics/dashboard/update_dashboard_service.rb | 6 | ||||
-rw-r--r-- | app/services/test_hooks/base_service.rb | 2 | ||||
-rw-r--r-- | app/services/test_hooks/project_service.rb | 14 | ||||
-rw-r--r-- | app/services/test_hooks/system_service.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/199438-fix-logs-encoding.yml | 5 | ||||
-rw-r--r-- | danger/database/Dangerfile | 3 | ||||
-rw-r--r-- | lib/gitlab/encoding_helper.rb | 13 | ||||
-rw-r--r-- | lib/gitlab/request_profiler/middleware.rb | 6 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | rubocop/cop/ban_catch_throw.rb | 34 | ||||
-rw-r--r-- | rubocop/cop/gitlab/keys-first-and-values-first.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/encoding_helper_spec.rb | 6 | ||||
-rw-r--r-- | spec/rubocop/cop/ban_catch_throw_spec.rb | 30 |
18 files changed, 123 insertions, 19 deletions
diff --git a/.gitlab/ci/review.gitlab-ci.yml b/.gitlab/ci/review.gitlab-ci.yml index 130e9085f03..c8d633bcdf3 100644 --- a/.gitlab/ci/review.gitlab-ci.yml +++ b/.gitlab/ci/review.gitlab-ci.yml @@ -85,6 +85,7 @@ review-deploy: needs: - job: review-build-cng artifacts: false + resource_group: "review/${CI_COMMIT_REF_NAME}" allow_failure: true before_script: - '[[ ! -d "ee/" ]] || export GITLAB_EDITION="ee"' diff --git a/.rubocop.yml b/.rubocop.yml index 2833d31986b..ebc27c4cc9e 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -344,3 +344,6 @@ Style/MultilineWhenThen: Style/FloatDivision: Enabled: false + +Cop/BanCatchThrow: + Enabled: true diff --git a/app/models/list.rb b/app/models/list.rb index b2ba796e3dc..64247fdb983 100644 --- a/app/models/list.rb +++ b/app/models/list.rb @@ -97,6 +97,6 @@ class List < ApplicationRecord private def can_be_destroyed - throw(:abort) unless destroyable? + throw(:abort) unless destroyable? # rubocop:disable Cop/BanCatchThrow end end diff --git a/app/models/project.rb b/app/models/project.rb index dd0a6c0de0d..d6a52b37f19 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2430,7 +2430,7 @@ class Project < ApplicationRecord if repository_storage.blank? || repository_with_same_path_already_exists? errors.add(:base, _('There is already a repository with that name on disk')) - throw :abort + throw :abort # rubocop:disable Cop/BanCatchThrow end end diff --git a/app/services/metrics/dashboard/clone_dashboard_service.rb b/app/services/metrics/dashboard/clone_dashboard_service.rb index 2720bfdafcd..3b06a7713d7 100644 --- a/app/services/metrics/dashboard/clone_dashboard_service.rb +++ b/app/services/metrics/dashboard/clone_dashboard_service.rb @@ -22,6 +22,7 @@ module Metrics end end + # rubocop:disable Cop/BanCatchThrow def execute catch(:error) do throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized? @@ -33,6 +34,7 @@ module Metrics success(result.merge(http_status: :created, dashboard: dashboard_details)) end end + # rubocop:enable Cop/BanCatchThrow private @@ -60,6 +62,7 @@ module Metrics Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch) end + # rubocop:disable Cop/BanCatchThrow def dashboard_template @dashboard_template ||= begin throw(:error, error(_('Not found.'), :not_found)) unless self.class.allowed_dashboard_templates.include?(params[:dashboard]) @@ -67,7 +70,9 @@ module Metrics params[:dashboard] end end + # rubocop:enable Cop/BanCatchThrow + # rubocop:disable Cop/BanCatchThrow def branch @branch ||= begin throw(:error, error(_('There was an error creating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name? @@ -76,6 +81,7 @@ module Metrics params[:branch] end end + # rubocop:enable Cop/BanCatchThrow def new_or_default_branch? !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch] @@ -89,6 +95,7 @@ module Metrics @new_dashboard_path ||= File.join(USER_DASHBOARDS_DIR, file_name) end + # rubocop:disable Cop/BanCatchThrow def file_name @file_name ||= begin throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid? @@ -96,6 +103,7 @@ module Metrics File.basename(params[:file_name]) end end + # rubocop:enable Cop/BanCatchThrow def target_file_type_valid? File.extname(params[:file_name]) == ALLOWED_FILE_TYPE diff --git a/app/services/metrics/dashboard/update_dashboard_service.rb b/app/services/metrics/dashboard/update_dashboard_service.rb index e29fc47678f..dccdedf61db 100644 --- a/app/services/metrics/dashboard/update_dashboard_service.rb +++ b/app/services/metrics/dashboard/update_dashboard_service.rb @@ -7,6 +7,7 @@ module Metrics ALLOWED_FILE_TYPE = '.yml' USER_DASHBOARDS_DIR = ::Metrics::Dashboard::ProjectDashboardService::DASHBOARD_ROOT + # rubocop:disable Cop/BanCatchThrow def execute catch(:error) do throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized? @@ -17,6 +18,7 @@ module Metrics success(result.merge(http_status: :created, dashboard: dashboard_details)) end end + # rubocop:enable Cop/BanCatchThrow private @@ -44,6 +46,7 @@ module Metrics Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch) end + # rubocop:disable Cop/BanCatchThrow def branch @branch ||= begin throw(:error, error(_('There was an error updating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name? @@ -52,6 +55,7 @@ module Metrics params[:branch] end end + # rubocop:enable Cop/BanCatchThrow def new_or_default_branch? !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch] @@ -69,6 +73,7 @@ module Metrics File.extname(params[:file_name]) == ALLOWED_FILE_TYPE end + # rubocop:disable Cop/BanCatchThrow def file_name @file_name ||= begin throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid? @@ -76,6 +81,7 @@ module Metrics File.basename(CGI.unescape(params[:file_name])) end end + # rubocop:enable Cop/BanCatchThrow def update_dashboard_content ::PerformanceMonitoring::PrometheusDashboard.from_json(params[:file_content]).to_yaml diff --git a/app/services/test_hooks/base_service.rb b/app/services/test_hooks/base_service.rb index 8b5439c00bf..ebebf29c28b 100644 --- a/app/services/test_hooks/base_service.rb +++ b/app/services/test_hooks/base_service.rb @@ -18,7 +18,7 @@ module TestHooks return error('Testing not available for this hook') end - error_message = catch(:validation_error) do + error_message = catch(:validation_error) do # rubocop:disable Cop/BanCatchThrow sample_data = self.__send__(trigger_data_method) # rubocop:disable GitlabSecurity/PublicSend return hook.execute(sample_data, trigger_key) # rubocop:disable Cop/AvoidReturnFromBlocks diff --git a/app/services/test_hooks/project_service.rb b/app/services/test_hooks/project_service.rb index a71278e8b8b..aa80cc928b9 100644 --- a/app/services/test_hooks/project_service.rb +++ b/app/services/test_hooks/project_service.rb @@ -11,7 +11,7 @@ module TestHooks private def push_events_data - throw(:validation_error, s_('TestHooks|Ensure the project has at least one commit.')) if project.empty_repo? + throw(:validation_error, s_('TestHooks|Ensure the project has at least one commit.')) if project.empty_repo? # rubocop:disable Cop/BanCatchThrow Gitlab::DataBuilder::Push.build_sample(project, current_user) end @@ -20,14 +20,14 @@ module TestHooks def note_events_data note = project.notes.first - throw(:validation_error, s_('TestHooks|Ensure the project has notes.')) unless note.present? + throw(:validation_error, s_('TestHooks|Ensure the project has notes.')) unless note.present? # rubocop:disable Cop/BanCatchThrow Gitlab::DataBuilder::Note.build(note, current_user) end def issues_events_data issue = project.issues.first - throw(:validation_error, s_('TestHooks|Ensure the project has issues.')) unless issue.present? + throw(:validation_error, s_('TestHooks|Ensure the project has issues.')) unless issue.present? # rubocop:disable Cop/BanCatchThrow issue.to_hook_data(current_user) end @@ -36,21 +36,21 @@ module TestHooks def merge_requests_events_data merge_request = project.merge_requests.first - throw(:validation_error, s_('TestHooks|Ensure the project has merge requests.')) unless merge_request.present? + throw(:validation_error, s_('TestHooks|Ensure the project has merge requests.')) unless merge_request.present? # rubocop:disable Cop/BanCatchThrow merge_request.to_hook_data(current_user) end def job_events_data build = project.builds.first - throw(:validation_error, s_('TestHooks|Ensure the project has CI jobs.')) unless build.present? + throw(:validation_error, s_('TestHooks|Ensure the project has CI jobs.')) unless build.present? # rubocop:disable Cop/BanCatchThrow Gitlab::DataBuilder::Build.build(build) end def pipeline_events_data pipeline = project.ci_pipelines.first - throw(:validation_error, s_('TestHooks|Ensure the project has CI pipelines.')) unless pipeline.present? + throw(:validation_error, s_('TestHooks|Ensure the project has CI pipelines.')) unless pipeline.present? # rubocop:disable Cop/BanCatchThrow Gitlab::DataBuilder::Pipeline.build(pipeline) end @@ -58,7 +58,7 @@ module TestHooks def wiki_page_events_data page = project.wiki.list_pages(limit: 1).first if !project.wiki_enabled? || page.blank? - throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.')) + throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.')) # rubocop:disable Cop/BanCatchThrow end Gitlab::DataBuilder::WikiPage.build(page, current_user, 'create') diff --git a/app/services/test_hooks/system_service.rb b/app/services/test_hooks/system_service.rb index fedf9c6799b..5c7961f417d 100644 --- a/app/services/test_hooks/system_service.rb +++ b/app/services/test_hooks/system_service.rb @@ -18,7 +18,7 @@ module TestHooks def merge_requests_events_data merge_request = MergeRequest.of_projects(current_user.projects.select(:id)).first - throw(:validation_error, s_('TestHooks|Ensure one of your projects has merge requests.')) unless merge_request.present? + throw(:validation_error, s_('TestHooks|Ensure one of your projects has merge requests.')) unless merge_request.present? # rubocop:disable Cop/BanCatchThrow merge_request.to_hook_data(current_user) end diff --git a/changelogs/unreleased/199438-fix-logs-encoding.yml b/changelogs/unreleased/199438-fix-logs-encoding.yml new file mode 100644 index 00000000000..c8a803f43e6 --- /dev/null +++ b/changelogs/unreleased/199438-fix-logs-encoding.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 error caused by Kubernetes logs not being encoded in UTF-8 +merge_request: 25999 +author: +type: fixed diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 16740cb867d..a0a2959bab5 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -29,6 +29,8 @@ DB_FILES_MESSAGE = <<~MSG The following files require a review from the Database team: MSG +DATABASE_APPROVED_LABEL = 'database::approved' + non_geo_db_schema_updated = !git.modified_files.grep(%r{\Adb/schema\.rb}).empty? geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb}).empty? @@ -46,6 +48,7 @@ if geo_migration_created && !geo_db_schema_updated end return unless gitlab_danger.ci? +return if gitlab.mr_labels.include?(DATABASE_APPROVED_LABEL) db_paths_to_review = helper.changes_by_category[:database] diff --git a/lib/gitlab/encoding_helper.rb b/lib/gitlab/encoding_helper.rb index 88729babb2b..67f8d691a77 100644 --- a/lib/gitlab/encoding_helper.rb +++ b/lib/gitlab/encoding_helper.rb @@ -50,7 +50,7 @@ module Gitlab detect && detect[:type] == :binary end - def encode_utf8(message) + def encode_utf8(message, replace: "") message = force_encode_utf8(message) return message if message.valid_encoding? @@ -64,7 +64,7 @@ module Gitlab '' end else - clean(message) + clean(message, replace: replace) end rescue ArgumentError nil @@ -94,8 +94,13 @@ module Gitlab message.force_encoding("UTF-8") end - def clean(message) - message.encode("UTF-16BE", undef: :replace, invalid: :replace, replace: "".encode("UTF-16BE")) + def clean(message, replace: "") + message.encode( + "UTF-16BE", + undef: :replace, + invalid: :replace, + replace: replace.encode("UTF-16BE") + ) .encode("UTF-8") .gsub("\0".encode("UTF-8"), "") end diff --git a/lib/gitlab/request_profiler/middleware.rb b/lib/gitlab/request_profiler/middleware.rb index 99958d7a211..7050aee3847 100644 --- a/lib/gitlab/request_profiler/middleware.rb +++ b/lib/gitlab/request_profiler/middleware.rb @@ -51,7 +51,7 @@ module Gitlab def call_with_call_stack_profiling(env) ret = nil report = RubyProf::Profile.profile do - ret = catch(:warden) do + ret = catch(:warden) do # rubocop:disable Cop/BanCatchThrow @app.call(env) end end @@ -67,7 +67,7 @@ module Gitlab def call_with_memory_profiling(env) ret = nil report = MemoryProfiler.report do - ret = catch(:warden) do + ret = catch(:warden) do # rubocop:disable Cop/BanCatchThrow @app.call(env) end end @@ -99,7 +99,7 @@ module Gitlab if ret.is_a?(Array) ret else - throw(:warden, ret) + throw(:warden, ret) # rubocop:disable Cop/BanCatchThrow end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index babf073fd4a..7a1a8eb2656 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20999,6 +20999,9 @@ msgstr "" msgid "Unable to connect to server: %{error}" msgstr "" +msgid "Unable to convert Kubernetes logs encoding to UTF-8" +msgstr "" + msgid "Unable to fetch unscanned projects" msgstr "" diff --git a/rubocop/cop/ban_catch_throw.rb b/rubocop/cop/ban_catch_throw.rb new file mode 100644 index 00000000000..42301d5512f --- /dev/null +++ b/rubocop/cop/ban_catch_throw.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Bans the use of 'catch/throw', as exceptions are better for errors and + # they are equivalent to 'goto' for flow control, with all the problems + # that implies. + # + # @example + # # bad + # catch(:error) do + # throw(:error) + # end + # + # # good + # begin + # raise StandardError + # rescue StandardError => err + # # ... + # end + # + class BanCatchThrow < RuboCop::Cop::Cop + MSG = "Do not use catch or throw unless a gem's API demands it." + + def on_send(node) + receiver, method_name, _ = *node + + return unless receiver.nil? && %i[catch throw].include?(method_name) + + add_offense(node, location: :expression) + end + end + end +end diff --git a/rubocop/cop/gitlab/keys-first-and-values-first.rb b/rubocop/cop/gitlab/keys-first-and-values-first.rb index 9b68957cba2..544f9800304 100644 --- a/rubocop/cop/gitlab/keys-first-and-values-first.rb +++ b/rubocop/cop/gitlab/keys-first-and-values-first.rb @@ -26,7 +26,7 @@ module RuboCop elsif node.descendants.first.method_name == :keys '.each_key' else - throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first") + throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first") # rubocop:disable Cop/BanCatchThrow end upto_including_keys_or_values = node.descendants.first.source_range diff --git a/spec/lib/gitlab/encoding_helper_spec.rb b/spec/lib/gitlab/encoding_helper_spec.rb index d091b6c1601..e6dfd8728aa 100644 --- a/spec/lib/gitlab/encoding_helper_spec.rb +++ b/spec/lib/gitlab/encoding_helper_spec.rb @@ -128,6 +128,12 @@ describe Gitlab::EncodingHelper do expect { ext_class.encode_utf8('') }.not_to raise_error end + it 'replaces invalid and undefined chars with the replace argument' do + str = 'hællo'.encode(Encoding::UTF_16LE).force_encoding(Encoding::ASCII_8BIT) + + expect(ext_class.encode_utf8(str, replace: "\u{FFFD}")).to eq("h�llo") + end + context 'with strings that can be forcefully encoded into utf8' do let(:test_string) do "refs/heads/FixSymbolsTitleDropdown".encode("ASCII-8BIT") diff --git a/spec/rubocop/cop/ban_catch_throw_spec.rb b/spec/rubocop/cop/ban_catch_throw_spec.rb new file mode 100644 index 00000000000..b4c277fc429 --- /dev/null +++ b/spec/rubocop/cop/ban_catch_throw_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../rubocop/cop/ban_catch_throw' + +describe RuboCop::Cop::BanCatchThrow do + include CopHelper + + subject(:cop) { described_class.new } + + it 'registers an offense when `catch` or `throw` are used' do + inspect_source("catch(:foo) {\n throw(:foo)\n}") + + aggregate_failures do + expect(cop.offenses.size).to eq(2) + expect(cop.offenses.map(&:line)).to eq([1, 2]) + expect(cop.highlights).to eq(['catch(:foo)', 'throw(:foo)']) + end + end + + it 'does not register an offense for a method called catch or throw' do + inspect_source("foo.catch(:foo) {\n foo.throw(:foo)\n}") + + expect(cop.offenses).to be_empty + end +end |