From 9d5573c70ae3ef3f550bb06b62cc640165683a94 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 3 Jun 2021 03:09:45 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- Gemfile | 4 +- Gemfile.lock | 16 ++- .../extensions/code_block_highlight.js | 2 +- .../content_editor/extensions/image.js | 45 ++++++- .../contextual_sidebar_variant.scss | 115 +++++++++++++----- .../stylesheets/framework/variables.scss | 3 + app/assets/stylesheets/performance_bar.scss | 2 +- .../layouts/nav/sidebar/_admin.html.haml | 4 +- .../nav/sidebar/_context_menu_body.html.haml | 4 +- .../layouts/nav/sidebar/_profile.html.haml | 4 +- .../shared/nav/_scope_menu_body.html.haml | 4 +- .../postgres_hll_batch_counting.yml | 8 -- config/initializers/mailer_retries.rb | 41 +++++++ .../operations/sidekiq_memory_killer.md | 16 +-- lib/gitlab/error_tracking.rb | 24 ++-- lib/gitlab/error_tracking/log_formatter.rb | 18 ++- .../processor/grpc_error_processor.rb | 9 +- .../processor/sanitizer_processor.rb | 33 +++++ spec/frontend/fixtures/api_markdown.rb | 2 +- spec/initializers/mailer_retries_spec.rb | 25 ++++ .../context_payload_generator_spec.rb | 2 +- .../error_tracking/log_formatter_spec.rb | 8 +- .../context_payload_processor_spec.rb | 18 +-- .../processor/grpc_error_processor_spec.rb | 19 +-- .../processor/sanitizer_processor_spec.rb | 96 +++++++++++++++ .../processor/sidekiq_processor_spec.rb | 18 +-- spec/lib/gitlab/error_tracking_spec.rb | 70 ++++++----- spec/requests/api/helpers_spec.rb | 2 +- .../discussion_comments_shared_example.rb | 4 + .../features/variable_list_shared_examples.rb | 2 + 30 files changed, 473 insertions(+), 145 deletions(-) delete mode 100644 config/feature_flags/development/postgres_hll_batch_counting.yml create mode 100644 config/initializers/mailer_retries.rb create mode 100644 lib/gitlab/error_tracking/processor/sanitizer_processor.rb create mode 100644 spec/initializers/mailer_retries_spec.rb create mode 100644 spec/lib/gitlab/error_tracking/processor/sanitizer_processor_spec.rb diff --git a/Gemfile b/Gemfile index e97348f17e8..d41bdfa0361 100644 --- a/Gemfile +++ b/Gemfile @@ -301,7 +301,9 @@ gem 'gitlab-license', '~> 1.5' gem 'rack-attack', '~> 6.3.0' # Sentry integration -gem 'sentry-raven', '~> 3.1' +gem 'sentry-ruby', '~> 4.4.0' +gem 'sentry-sidekiq', '~> 4.4.0' +gem 'sentry-rails', '~> 4.4.0' # PostgreSQL query parsing # diff --git a/Gemfile.lock b/Gemfile.lock index 59cbd84a3e6..837edfffdf0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1172,8 +1172,18 @@ GEM selenium-webdriver (3.142.7) childprocess (>= 0.5, < 4.0) rubyzip (>= 1.2.2) - sentry-raven (3.1.2) + sentry-rails (4.4.0) + railties (>= 5.0) + sentry-ruby-core (~> 4.4.0.pre.beta) + sentry-ruby (4.4.2) + concurrent-ruby (~> 1.0, >= 1.0.2) faraday (>= 1.0) + sentry-ruby-core (= 4.4.2) + sentry-ruby-core (4.4.2) + concurrent-ruby + faraday + sentry-sidekiq (4.4.0) + sentry-ruby-core (~> 4.4.0.pre.beta) settingslogic (2.0.9) sexp_processor (4.15.1) shellany (0.0.1) @@ -1619,7 +1629,9 @@ DEPENDENCIES sassc-rails (~> 2.1.0) seed-fu (~> 2.3.7) selenium-webdriver (~> 3.142) - sentry-raven (~> 3.1) + sentry-rails (~> 4.4.0) + sentry-ruby (~> 4.4.0) + sentry-sidekiq (~> 4.4.0) settingslogic (~> 2.0.9) shoulda-matchers (~> 4.0.1) sidekiq (~> 5.2.7) diff --git a/app/assets/javascripts/content_editor/extensions/code_block_highlight.js b/app/assets/javascripts/content_editor/extensions/code_block_highlight.js index ce8bd57c7e3..457864f5795 100644 --- a/app/assets/javascripts/content_editor/extensions/code_block_highlight.js +++ b/app/assets/javascripts/content_editor/extensions/code_block_highlight.js @@ -1,7 +1,7 @@ import { CodeBlockLowlight } from '@tiptap/extension-code-block-lowlight'; import { defaultMarkdownSerializer } from 'prosemirror-markdown/src/to_markdown'; -const extractLanguage = (element) => element.firstElementChild?.getAttribute('lang'); +const extractLanguage = (element) => element.getAttribute('lang'); const ExtendedCodeBlockLowlight = CodeBlockLowlight.extend({ addAttributes() { diff --git a/app/assets/javascripts/content_editor/extensions/image.js b/app/assets/javascripts/content_editor/extensions/image.js index 4f0109fd751..287216e68d5 100644 --- a/app/assets/javascripts/content_editor/extensions/image.js +++ b/app/assets/javascripts/content_editor/extensions/image.js @@ -2,8 +2,49 @@ import { Image } from '@tiptap/extension-image'; import { defaultMarkdownSerializer } from 'prosemirror-markdown/src/to_markdown'; const ExtendedImage = Image.extend({ - defaultOptions: { inline: true }, -}); + addAttributes() { + return { + ...this.parent?.(), + src: { + default: null, + /* + * GitLab Flavored Markdown provides lazy loading for rendering images. As + * as result, the src attribute of the image may contain an embedded resource + * instead of the actual image URL. The image URL is moved to the data-src + * attribute. + */ + parseHTML: (element) => { + const img = element.querySelector('img'); + + return { + src: img.dataset.src || img.getAttribute('src'), + }; + }, + }, + alt: { + default: null, + parseHTML: (element) => { + const img = element.querySelector('img'); + + return { + alt: img.getAttribute('alt'), + }; + }, + }, + }; + }, + parseHTML() { + return [ + { + priority: 100, + tag: 'a.no-attachment-icon', + }, + { + tag: 'img[src]', + }, + ]; + }, +}).configure({ inline: true }); export const tiptapExtension = ExtendedImage; export const serializer = defaultMarkdownSerializer.nodes.image; diff --git a/app/assets/stylesheets/framework/contextual_sidebar_refactoring/contextual_sidebar_variant.scss b/app/assets/stylesheets/framework/contextual_sidebar_refactoring/contextual_sidebar_variant.scss index 58438df8295..ece81fde078 100644 --- a/app/assets/stylesheets/framework/contextual_sidebar_refactoring/contextual_sidebar_variant.scss +++ b/app/assets/stylesheets/framework/contextual_sidebar_refactoring/contextual_sidebar_variant.scss @@ -1,25 +1,41 @@ +// +// VARIABLES +// + +$top-level-item-color: $purple-900; + // // TEMPORARY OVERRIDES // Needed while we serve both *_base and *_variant stylesheets -// TODO: These have to be removed during the feature flag rollout +// TODO: These have to be removed during the ':sidebar_refactor' flag rollout // &.gl-dark .nav-sidebar li.active { box-shadow: none; } &.gl-dark .nav-sidebar li a, -.toggle-sidebar-button .collapse-text, -.toggle-sidebar-button .icon-chevron-double-lg-left, -.toggle-sidebar-button .icon-chevron-double-lg-right { +&.gl-dark .toggle-sidebar-button .collapse-text, +&.gl-dark .toggle-sidebar-button .icon-chevron-double-lg-left, +&.gl-dark .toggle-sidebar-button .icon-chevron-double-lg-right, +&.gl-dark .sidebar-top-level-items .context-header a .sidebar-context-title, +&.gl-dark .nav-sidebar-inner-scroll > div.context-header a .sidebar-context-title { color: $gray-darkest; } +&.ui-indigo .nav-sidebar li.active > a { + color: $top-level-item-color; +} + +&.ui-indigo .nav-sidebar li.active .nav-icon-container svg { + fill: $top-level-item-color; +} + .nav-sidebar { box-shadow: none; li.active { background-color: transparent; - box-shadow: none; + box-shadow: none !important; // TODO: This should be updated in `theme_helper.scss` together with ':sidebar_refactor' rollout } } @@ -137,6 +153,48 @@ } } +@mixin context-header { + $avatar-box-shadow: inset 0 0 0 1px $t-gray-a-08; + + @include gl-p-2; + @include gl-mb-2; + + .avatar-container { + @include gl-font-weight-normal; + flex: none; + box-shadow: $avatar-box-shadow; + + &.rect-avatar { + @include gl-border-none; + + .avatar.s32 { + @extend .rect-avatar.s32; + color: $gray-900; + box-shadow: $avatar-box-shadow; + } + } + } + + .sidebar-context-title { + color: $top-level-item-color; + } +} + +@mixin top-level-item { + @include gl-px-4; + @include gl-py-3; + @include gl-display-flex; + @include gl-align-items-center; + @include gl-rounded-base; + @include gl-w-auto; + transition: padding $sidebar-transition-duration; + margin: $sidebar-top-item-tb-margin $sidebar-top-item-lr-margin; + + &:hover { + background-color: $indigo-900-alpha-008; + } +} + // // PAGE-LAYOUT // @@ -195,7 +253,7 @@ a { @include gl-text-decoration-none; @include gl-line-height-normal; - color: $purple-900; + color: $top-level-item-color; } li { @@ -206,19 +264,7 @@ } > a { - @include gl-mx-2; - @include gl-px-4; - @include gl-py-3; - @include gl-display-flex; - @include gl-align-items-center; - @include gl-rounded-base; - @include gl-w-auto; - transition: padding $sidebar-transition-duration; - margin-bottom: 1px; - - &:hover { - background-color: $indigo-900-alpha-008; - } + @include top-level-item; } &.active { @@ -250,11 +296,6 @@ display: none; } - svg { - height: 16px; - width: 16px; - } - @media (min-width: map-get($grid-breakpoints, md)) and (max-width: map-get($grid-breakpoints, xl) - 1px) { &:not(.sidebar-expanded-mobile) { @include collapse-contextual-sidebar; @@ -264,14 +305,28 @@ } .nav-sidebar-inner-scroll { - height: 100%; - width: 100%; - overflow: auto; + @include gl-h-full; + @include gl-w-full; + @include gl-overflow-auto; + + > div.context-header { + @include gl-mt-2; + + a { + @include top-level-item; + @include context-header; + } + } } .sidebar-top-level-items { + @include gl-mt-2; margin-bottom: 60px; + .context-header a { + @include context-header; + } + > li { .badge.badge-pill { @include gl-rounded-lg; @@ -287,8 +342,8 @@ } .badge.badge-pill { - @include gl-font-weight-normal; // TODO: update in `theme_indigo.scss` - color: $blue-700; // TODO: update in `theme_indigo.scss` + @include gl-font-weight-normal; // TODO: update in `theme_helper.scss` + color: $blue-700; // TODO: update in `theme_helper.scss` } } } @@ -325,7 +380,7 @@ .close-nav-button { @include side-panel-toggle; background-color: $gray-50; - color: $purple-900; + color: $top-level-item-color; .collapse-text, .icon-chevron-double-lg-left, diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index bfb21d7112b..164bdb19447 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -573,6 +573,9 @@ $inactive-badge-background: rgba($black, 0.08); $sidebar-toggle-height: 60px; $sidebar-toggle-width: 40px; $sidebar-milestone-toggle-bottom-margin: 10px; +$sidebar-avatar-size: 32px; +$sidebar-top-item-lr-margin: 4px; +$sidebar-top-item-tb-margin: 1px; /* * Buttons diff --git a/app/assets/stylesheets/performance_bar.scss b/app/assets/stylesheets/performance_bar.scss index 65d47099a13..f2874e67796 100644 --- a/app/assets/stylesheets/performance_bar.scss +++ b/app/assets/stylesheets/performance_bar.scss @@ -128,6 +128,6 @@ color: $black; } -.with-performance-bar .nav-sidebar { +html.with-performance-bar .nav-sidebar { top: $header-height + $performance-bar-height; } diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml index a7263ae2576..e67dc750c4f 100644 --- a/app/views/layouts/nav/sidebar/_admin.html.haml +++ b/app/views/layouts/nav/sidebar/_admin.html.haml @@ -2,8 +2,8 @@ .nav-sidebar-inner-scroll .context-header = link_to admin_root_path, title: _('Admin Overview') do - %span.avatar-container.s40.settings-avatar - = sprite_icon('admin', size: 24) + %span.avatar-container.rect-avatar.s32.settings-avatar + = sprite_icon('admin', size: 18) %span.sidebar-context-title = _('Admin Area') %ul.sidebar-top-level-items{ data: { qa_selector: 'admin_sidebar_overview_submenu_content' } } diff --git a/app/views/layouts/nav/sidebar/_context_menu_body.html.haml b/app/views/layouts/nav/sidebar/_context_menu_body.html.haml index 64b28cc892e..1f56e0fefa8 100644 --- a/app/views/layouts/nav/sidebar/_context_menu_body.html.haml +++ b/app/views/layouts/nav/sidebar/_context_menu_body.html.haml @@ -1,5 +1,5 @@ = link_to group_path(@group), title: @group.name do - %span.avatar-container.rect-avatar.s40.group-avatar - = group_icon(@group, class: "avatar s40 avatar-tile") + %span.avatar-container.rect-avatar.s32.group-avatar + = group_icon(@group, class: "avatar s32 avatar-tile") %span.sidebar-context-title = @group.name diff --git a/app/views/layouts/nav/sidebar/_profile.html.haml b/app/views/layouts/nav/sidebar/_profile.html.haml index 63b97e3133c..236283badf4 100644 --- a/app/views/layouts/nav/sidebar/_profile.html.haml +++ b/app/views/layouts/nav/sidebar/_profile.html.haml @@ -2,8 +2,8 @@ .nav-sidebar-inner-scroll .context-header = link_to profile_path, title: _('Profile Settings') do - %span.avatar-container.s40.settings-avatar - = image_tag avatar_icon_for_user(current_user, 40), class: "avatar s40 avatar-tile js-sidebar-user-avatar", alt: current_user.name, data: { testid: 'sidebar-user-avatar' } + %span.avatar-container.s32.settings-avatar + = image_tag avatar_icon_for_user(current_user, 32), class: "avatar s32 avatar-tile js-sidebar-user-avatar", alt: current_user.name, data: { testid: 'sidebar-user-avatar' } %span.sidebar-context-title= _('User Settings') %ul.sidebar-top-level-items = nav_link(path: 'profiles#show', html_options: {class: 'home'}) do diff --git a/app/views/shared/nav/_scope_menu_body.html.haml b/app/views/shared/nav/_scope_menu_body.html.haml index 4e08bee4ee4..64b503bb327 100644 --- a/app/views/shared/nav/_scope_menu_body.html.haml +++ b/app/views/shared/nav/_scope_menu_body.html.haml @@ -1,5 +1,5 @@ = link_to scope_menu.link, **scope_menu.container_html_options do - %span.avatar-container.rect-avatar.s40.project-avatar - = source_icon(scope_menu.container, alt: scope_menu.title, class: 'avatar s40 avatar-tile', width: 40, height: 40) + %span.avatar-container.rect-avatar.s32.project-avatar + = source_icon(scope_menu.container, alt: scope_menu.title, class: 'avatar s32 avatar-tile', width: 32, height: 32) %span.sidebar-context-title = scope_menu.title diff --git a/config/feature_flags/development/postgres_hll_batch_counting.yml b/config/feature_flags/development/postgres_hll_batch_counting.yml deleted file mode 100644 index 87d3c7816a1..00000000000 --- a/config/feature_flags/development/postgres_hll_batch_counting.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: postgres_hll_batch_counting -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48233 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/285485 -milestone: '13.7' -type: development -group: group::product analytics -default_enabled: false diff --git a/config/initializers/mailer_retries.rb b/config/initializers/mailer_retries.rb new file mode 100644 index 00000000000..64fb0ffaa55 --- /dev/null +++ b/config/initializers/mailer_retries.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +class ActiveJob::QueueAdapters::SidekiqAdapter + # With Sidekiq 6, we can do something like: + # class ActionMailer::MailDeliveryJob + # sidekiq_options retry: 3 + # end + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/329430 + raise "Update this monkey patch: #{__FILE__}" unless Sidekiq::VERSION == '5.2.9' + + def enqueue(job) #:nodoc: + # Sidekiq::Client does not support symbols as keys + job.provider_job_id = Sidekiq::Client.push \ + "class" => JobWrapper, + "wrapped" => job.class.to_s, + "queue" => job.queue_name, + "args" => [job.serialize], + "retry" => retry_for(job) + end + + def enqueue_at(job, timestamp) #:nodoc: + job.provider_job_id = Sidekiq::Client.push \ + "class" => JobWrapper, + "wrapped" => job.class.to_s, + "queue" => job.queue_name, + "args" => [job.serialize], + "at" => timestamp, + "retry" => retry_for(job) + end + + private + + def retry_for(job) + if job.queue_name == 'mailers' + 3 + else + true + end + end +end diff --git a/doc/administration/operations/sidekiq_memory_killer.md b/doc/administration/operations/sidekiq_memory_killer.md index c7f00d05213..d3019e2c580 100644 --- a/doc/administration/operations/sidekiq_memory_killer.md +++ b/doc/administration/operations/sidekiq_memory_killer.md @@ -19,7 +19,7 @@ _only_ for Omnibus packages. The reason for this is that the MemoryKiller relies on runit to restart Sidekiq after a memory-induced shutdown and GitLab installations from source do not all use runit or an equivalent. -With the default settings, the MemoryKiller will cause a Sidekiq restart no +With the default settings, the MemoryKiller causes a Sidekiq restart no more often than once every 15 minutes, with the restart causing about one minute of delay for incoming background jobs. @@ -48,13 +48,13 @@ The MemoryKiller is controlled using environment variables. `SIDEKIQ_MEMORY_KILLER_MAX_RSS` defines the Sidekiq process allowed RSS. In _legacy_ mode, if the Sidekiq process exceeds the allowed RSS then an irreversible - delayed graceful restart will be triggered. The restart of Sidekiq will happen + delayed graceful restart is triggered. The restart of Sidekiq happens after `SIDEKIQ_MEMORY_KILLER_GRACE_TIME` seconds. In _daemon_ mode, if the Sidekiq process exceeds the allowed RSS for longer than - `SIDEKIQ_MEMORY_KILLER_GRACE_TIME` the graceful restart will be triggered. If the + `SIDEKIQ_MEMORY_KILLER_GRACE_TIME` the graceful restart is triggered. If the Sidekiq process go below the allowed RSS within `SIDEKIQ_MEMORY_KILLER_GRACE_TIME`, - the restart will be aborted. + the restart is aborted. The default value for Omnibus packages is set [in the Omnibus GitLab @@ -71,13 +71,13 @@ The MemoryKiller is controlled using environment variables. The usage of this variable is described as part of `SIDEKIQ_MEMORY_KILLER_MAX_RSS`. - `SIDEKIQ_MEMORY_KILLER_SHUTDOWN_WAIT`: defaults to 30 seconds. This defines the - maximum time allowed for all Sidekiq jobs to finish. No new jobs will be accepted - during that time, and the process will exit as soon as all jobs finish. + maximum time allowed for all Sidekiq jobs to finish. No new jobs are accepted + during that time, and the process exits as soon as all jobs finish. - If jobs do not finish during that time, the MemoryKiller will interrupt all currently + If jobs do not finish during that time, the MemoryKiller interrupts all currently running jobs by sending `SIGTERM` to the Sidekiq process. If the process hard shutdown/restart is not performed by Sidekiq, - the Sidekiq process will be forcefully terminated after + the Sidekiq process is forcefully terminated after `Sidekiq.options[:timeout] + 2` seconds. An external supervision mechanism (e.g. runit) must restart Sidekiq afterwards. diff --git a/lib/gitlab/error_tracking.rb b/lib/gitlab/error_tracking.rb index 38ac5d9af74..635e84d799f 100644 --- a/lib/gitlab/error_tracking.rb +++ b/lib/gitlab/error_tracking.rb @@ -19,22 +19,21 @@ module Gitlab PROCESSORS = [ ::Gitlab::ErrorTracking::Processor::SidekiqProcessor, ::Gitlab::ErrorTracking::Processor::GrpcErrorProcessor, - ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor + ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor, + # IMPORTANT: this processor must stay at the bottom, right before + # sending the event to Sentry. + ::Gitlab::ErrorTracking::Processor::SanitizerProcessor ].freeze class << self def configure - Raven.configure do |config| + Sentry.init do |config| config.dsn = sentry_dsn config.release = Gitlab.revision - config.current_environment = Gitlab.config.sentry.environment - - # Sanitize fields based on those sanitized from Rails. - config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s) - - # Sanitize authentication headers - config.sanitize_http_headers = %w[Authorization Private-Token] + config.environment = Gitlab.config.sentry.environment config.before_send = method(:before_send) + config.background_worker_threads = 0 + config.send_default_pii = true yield config if block_given? end @@ -108,8 +107,11 @@ module Gitlab def process_exception(exception, sentry: false, logging: true, extra:) context_payload = Gitlab::ErrorTracking::ContextPayloadGenerator.generate(exception, extra) - if sentry && Raven.configuration.server - Raven.capture_exception(exception, **context_payload) + # There is a possibility that this method is called before Sentry is + # configured. Since Sentry 4.0, some methods of Sentry are forwarded to + # to `nil`, hence we have to check the client as well. + if sentry && ::Sentry.get_current_client && ::Sentry.configuration.dsn + ::Sentry.capture_exception(exception, **context_payload) end if logging diff --git a/lib/gitlab/error_tracking/log_formatter.rb b/lib/gitlab/error_tracking/log_formatter.rb index d004c4e20bb..92ef4d957f3 100644 --- a/lib/gitlab/error_tracking/log_formatter.rb +++ b/lib/gitlab/error_tracking/log_formatter.rb @@ -3,7 +3,7 @@ module Gitlab module ErrorTracking class LogFormatter - # Note: all the accesses to Raven's contexts here are to keep the + # Note: all the accesses to Sentry's contexts here are to keep the # backward-compatibility to Sentry's built-in integrations. In future, # they can be removed. def generate_log(exception, context_payload) @@ -20,21 +20,27 @@ module Gitlab private def append_user_to_log!(payload, context_payload) - user_context = Raven.context.user.merge(context_payload[:user]) + return if current_scope.blank? + + user_context = current_scope.user.merge(context_payload[:user]) user_context.each do |key, value| payload["user.#{key}"] = value end end def append_tags_to_log!(payload, context_payload) - tags_context = Raven.context.tags.merge(context_payload[:tags]) + return if current_scope.blank? + + tags_context = current_scope.tags.merge(context_payload[:tags]) tags_context.each do |key, value| payload["tags.#{key}"] = value end end def append_extra_to_log!(payload, context_payload) - extra = Raven.context.extra.merge(context_payload[:extra]) + return if current_scope.blank? + + extra = current_scope.extra.merge(context_payload[:extra]) extra = extra.except(:server) # The extra value for sidekiq is a hash whose keys are strings. @@ -50,6 +56,10 @@ module Gitlab payload["extra.#{key}"] = value end end + + def current_scope + Sentry.get_current_scope + end end end end diff --git a/lib/gitlab/error_tracking/processor/grpc_error_processor.rb b/lib/gitlab/error_tracking/processor/grpc_error_processor.rb index e2a9192806f..0c2f1b2be67 100644 --- a/lib/gitlab/error_tracking/processor/grpc_error_processor.rb +++ b/lib/gitlab/error_tracking/processor/grpc_error_processor.rb @@ -17,8 +17,7 @@ module Gitlab # Sentry can report multiple exceptions in an event. Sanitize # only the first one since that's what is used for grouping. def process_first_exception_value(event) - # Better in new version, will be event.exception.values - exceptions = event.instance_variable_get(:@interfaces)[:exception]&.values + exceptions = event.exception&.instance_variable_get(:@values) return unless exceptions.is_a?(Array) @@ -33,9 +32,7 @@ module Gitlab message, debug_str = split_debug_error_string(raw_message) - # Worse in new version, no setter! Have to poke at the - # instance variable - exception.value = message if message + exception.instance_variable_set(:@value, message) if message event.extra[:grpc_debug_error_string] = debug_str if debug_str end @@ -66,7 +63,7 @@ module Gitlab def valid_exception?(exception) case exception - when Raven::SingleExceptionInterface + when Sentry::SingleExceptionInterface exception&.value else false diff --git a/lib/gitlab/error_tracking/processor/sanitizer_processor.rb b/lib/gitlab/error_tracking/processor/sanitizer_processor.rb new file mode 100644 index 00000000000..32d441fcdef --- /dev/null +++ b/lib/gitlab/error_tracking/processor/sanitizer_processor.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Gitlab + module ErrorTracking + module Processor + module SanitizerProcessor + SANITIZED_HTTP_HEADERS = %w[Authorization Private-Token].freeze + SANITIZED_ATTRIBUTES = %i[user contexts extra tags].freeze + + # This processor removes sensitive fields or headers from the event + # before sending. Sentry versions above 4.0 don't support + # sanitized_fields and sanitized_http_headers anymore. The official + # document recommends using before_send instead. + # + # For more information, please visit: + # https://docs.sentry.io/platforms/ruby/guides/rails/configuration/filtering/#using-beforesend + def self.call(event) + if event.request.present? && event.request.headers.is_a?(Hash) + header_filter = ActiveSupport::ParameterFilter.new(SANITIZED_HTTP_HEADERS) + event.request.headers = header_filter.filter(event.request.headers) + end + + attribute_filter = ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters) + SANITIZED_ATTRIBUTES.each do |attribute| + event.send("#{attribute}=", attribute_filter.filter(event.send(attribute))) # rubocop:disable GitlabSecurity/PublicSend + end + + event + end + end + end + end +end diff --git a/spec/frontend/fixtures/api_markdown.rb b/spec/frontend/fixtures/api_markdown.rb index e012d922aad..1c3967b2c36 100644 --- a/spec/frontend/fixtures/api_markdown.rb +++ b/spec/frontend/fixtures/api_markdown.rb @@ -25,7 +25,7 @@ RSpec.describe API::MergeRequests, '(JavaScript fixtures)', type: :request do let(:markdown) { markdown_example.fetch(:markdown) } it "#{fixture_subdir}/#{name}.json" do - post api("/markdown"), params: { text: markdown } + post api("/markdown"), params: { text: markdown, gfm: true } expect(response).to be_successful end diff --git a/spec/initializers/mailer_retries_spec.rb b/spec/initializers/mailer_retries_spec.rb new file mode 100644 index 00000000000..59b8056aa0c --- /dev/null +++ b/spec/initializers/mailer_retries_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Mailer retries' do + # We need to ensure that this runs through Sidekiq to take + # advantage of the middleware. There is a Rails bug that means we + # have to do some extra steps to make this happen: + # https://github.com/rails/rails/issues/37270#issuecomment-553927324 + around do |example| + descendants = ActiveJob::Base.descendants + [ActiveJob::Base] + descendants.each(&:disable_test_adapter) + ActiveJob::Base.queue_adapter = :sidekiq + + example.run + + descendants.each { |a| a.queue_adapter = :test } + end + + it 'sets retries for mailers to 3' do + DeviseMailer.user_admin_approval(create(:user)).deliver_later + + expect(Sidekiq::Queues['mailers'].first).to include('retry' => 3) + end +end diff --git a/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb b/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb index 0e72dd7ec5e..dffa7e6522e 100644 --- a/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb +++ b/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'fast_spec_helper' +require 'spec_helper' require 'rspec-parameterized' RSpec.describe Gitlab::ErrorTracking::ContextPayloadGenerator do diff --git a/spec/lib/gitlab/error_tracking/log_formatter_spec.rb b/spec/lib/gitlab/error_tracking/log_formatter_spec.rb index 188ccd000a1..c97aebf5cd3 100644 --- a/spec/lib/gitlab/error_tracking/log_formatter_spec.rb +++ b/spec/lib/gitlab/error_tracking/log_formatter_spec.rb @@ -27,9 +27,9 @@ RSpec.describe Gitlab::ErrorTracking::LogFormatter do end before do - Raven.context.user[:user_flag] = 'flag' - Raven.context.tags[:shard] = 'catchall' - Raven.context.extra[:some_info] = 'info' + Sentry.set_user(user_flag: 'flag') + Sentry.set_tags(shard: 'catchall') + Sentry.set_extras(some_info: 'info') allow(exception).to receive(:backtrace).and_return( [ @@ -40,7 +40,7 @@ RSpec.describe Gitlab::ErrorTracking::LogFormatter do end after do - ::Raven::Context.clear! + ::Sentry.get_current_scope.clear end it 'appends error-related log fields and filters sensitive Sidekiq arguments' do diff --git a/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb index 210829056c8..8b83d073e45 100644 --- a/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb +++ b/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb @@ -4,18 +4,14 @@ require 'spec_helper' RSpec.describe Gitlab::ErrorTracking::Processor::ContextPayloadProcessor do describe '.call' do - let(:required_options) do - { - configuration: Raven.configuration, - context: Raven.context, - breadcrumbs: Raven.breadcrumbs - } - end - - let(:event) { Raven::Event.new(required_options.merge(payload)) } + let(:exception) { StandardError.new('Test exception') } + let(:event) { Sentry.get_current_client.event_from_exception(exception) } let(:result_hash) { described_class.call(event).to_hash } before do + Sentry.get_current_scope.update_from_options(**payload) + Sentry.get_current_scope.apply_to_event(event) + allow_next_instance_of(Gitlab::ErrorTracking::ContextPayloadGenerator) do |generator| allow(generator).to receive(:generate).and_return( user: { username: 'root' }, @@ -25,6 +21,10 @@ RSpec.describe Gitlab::ErrorTracking::Processor::ContextPayloadProcessor do end end + after do + Sentry.get_current_scope.clear + end + let(:payload) do { user: { ip_address: '127.0.0.1' }, diff --git a/spec/lib/gitlab/error_tracking/processor/grpc_error_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/grpc_error_processor_spec.rb index 6076e525f06..20fa5e2dacf 100644 --- a/spec/lib/gitlab/error_tracking/processor/grpc_error_processor_spec.rb +++ b/spec/lib/gitlab/error_tracking/processor/grpc_error_processor_spec.rb @@ -4,16 +4,17 @@ require 'spec_helper' RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do describe '.call' do - let(:required_options) do - { - configuration: Raven.configuration, - context: Raven.context, - breadcrumbs: Raven.breadcrumbs - } + let(:event) { Sentry.get_current_client.event_from_exception(exception) } + let(:result_hash) { described_class.call(event).to_hash } + + before do + Sentry.get_current_scope.update_from_options(**data) + Sentry.get_current_scope.apply_to_event(event) end - let(:event) { Raven::Event.from_exception(exception, required_options.merge(data)) } - let(:result_hash) { described_class.call(event).to_hash } + after do + Sentry.get_current_scope.clear + end context 'when there is no GRPC exception' do let(:exception) { RuntimeError.new } @@ -56,7 +57,7 @@ RSpec.describe Gitlab::ErrorTracking::Processor::GrpcErrorProcessor do end it 'removes the debug error string and stores it as an extra field' do - expect(result_hash).not_to include(:fingerprint) + expect(result_hash[:fingerprint]).to be_empty expect(result_hash[:exception][:values].first) .to include(type: 'GRPC::DeadlineExceeded', value: '4:Deadline Exceeded.') diff --git a/spec/lib/gitlab/error_tracking/processor/sanitizer_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/sanitizer_processor_spec.rb new file mode 100644 index 00000000000..7e7d836f1d2 --- /dev/null +++ b/spec/lib/gitlab/error_tracking/processor/sanitizer_processor_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::ErrorTracking::Processor::SanitizerProcessor do + describe '.call' do + let(:event) { Sentry.get_current_client.event_from_exception(exception) } + let(:result_hash) { described_class.call(event).to_hash } + + before do + data.each do |key, value| + event.send("#{key}=", value) + end + end + + after do + Sentry.get_current_scope.clear + end + + context 'when event attributes contains sensitive information' do + let(:exception) { RuntimeError.new } + let(:data) do + { + contexts: { + jwt: 'abcdef', + controller: 'GraphController#execute' + }, + tags: { + variables: %w[some sensitive information'], + deep_hash: { + sharedSecret: 'secret123' + } + }, + user: { + email: 'a@a.com', + password: 'nobodyknows' + }, + extra: { + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1', + my_token: '[FILTERED]', + another_token: '[FILTERED]' + } + } + end + + it 'filters sensitive attributes' do + expect_next_instance_of(ActiveSupport::ParameterFilter) do |instance| + expect(instance).to receive(:filter).exactly(4).times.and_call_original + end + + expect(result_hash).to include( + contexts: { + jwt: '[FILTERED]', + controller: 'GraphController#execute' + }, + tags: { + variables: '[FILTERED]', + deep_hash: { + sharedSecret: '[FILTERED]' + } + }, + user: { + email: 'a@a.com', + password: '[FILTERED]' + }, + extra: { + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1', + my_token: '[FILTERED]', + another_token: '[FILTERED]' + } + ) + end + end + + context 'when request headers contains sensitive information' do + let(:exception) { RuntimeError.new } + let(:data) { {} } + + before do + event.rack_env = { + 'HTTP_AUTHORIZATION' => 'Bearer 123456', + 'HTTP_PRIVATE_TOKEN' => 'abcdef', + 'HTTP_GITLAB_WORKHORSE_PROXY_START' => 123456 + } + end + + it 'filters sensitive headers' do + expect(result_hash[:request][:headers]).to include( + 'Authorization' => '[FILTERED]', + 'Private-Token' => '[FILTERED]', + 'Gitlab-Workhorse-Proxy-Start' => '123456' + ) + end + end + end +end diff --git a/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb index af5f11c9362..bf8d31822a6 100644 --- a/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb +++ b/spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb @@ -95,16 +95,18 @@ RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do end describe '.call' do - let(:required_options) do - { - configuration: Raven.configuration, - context: Raven.context, - breadcrumbs: Raven.breadcrumbs - } + let(:exception) { StandardError.new('Test exception') } + let(:event) { Sentry.get_current_client.event_from_exception(exception) } + let(:result_hash) { described_class.call(event).to_hash } + + before do + Sentry.get_current_scope.update_from_options(**wrapped_value) + Sentry.get_current_scope.apply_to_event(event) end - let(:event) { Raven::Event.new(required_options.merge(wrapped_value)) } - let(:result_hash) { described_class.call(event).to_hash } + after do + Sentry.get_current_scope.clear + end context 'when there is Sidekiq data' do let(:wrapped_value) { { extra: { sidekiq: value } } } diff --git a/spec/lib/gitlab/error_tracking_spec.rb b/spec/lib/gitlab/error_tracking_spec.rb index 7ad1f52780a..50c85f76ce8 100644 --- a/spec/lib/gitlab/error_tracking_spec.rb +++ b/spec/lib/gitlab/error_tracking_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -require 'raven/transports/dummy' +require 'sentry/transport/dummy_transport' RSpec.describe Gitlab::ErrorTracking do let(:exception) { RuntimeError.new('boom') } @@ -43,7 +43,7 @@ RSpec.describe Gitlab::ErrorTracking do } end - let(:sentry_event) { Gitlab::Json.parse(Raven.client.transport.events.last[1]) } + let(:sentry_event) { Sentry.get_current_client.transport.events.last } before do stub_sentry_settings @@ -53,7 +53,7 @@ RSpec.describe Gitlab::ErrorTracking do allow(I18n).to receive(:locale).and_return('en') described_class.configure do |config| - config.encoding = 'json' + config.transport.transport_class = Sentry::DummyTransport end end @@ -63,6 +63,10 @@ RSpec.describe Gitlab::ErrorTracking do end end + after do + ::Sentry.get_current_scope.clear + end + describe '.track_and_raise_for_dev_exception' do context 'when exceptions for dev should be raised' do before do @@ -70,7 +74,7 @@ RSpec.describe Gitlab::ErrorTracking do end it 'raises the exception' do - expect(Raven).to receive(:capture_exception).with(exception, sentry_payload) + expect(Sentry).to receive(:capture_exception).with(exception, sentry_payload) expect do described_class.track_and_raise_for_dev_exception( @@ -88,7 +92,7 @@ RSpec.describe Gitlab::ErrorTracking do end it 'logs the exception with all attributes passed' do - expect(Raven).to receive(:capture_exception).with(exception, sentry_payload) + expect(Sentry).to receive(:capture_exception).with(exception, sentry_payload) described_class.track_and_raise_for_dev_exception( exception, @@ -111,7 +115,7 @@ RSpec.describe Gitlab::ErrorTracking do describe '.track_and_raise_exception' do it 'always raises the exception' do - expect(Raven).to receive(:capture_exception).with(exception, sentry_payload) + expect(Sentry).to receive(:capture_exception).with(exception, sentry_payload) expect do described_class.track_and_raise_for_dev_exception( @@ -139,14 +143,14 @@ RSpec.describe Gitlab::ErrorTracking do subject(:track_exception) { described_class.track_exception(exception, extra) } before do - allow(Raven).to receive(:capture_exception).and_call_original + allow(Sentry).to receive(:capture_exception).and_call_original allow(Gitlab::ErrorTracking::Logger).to receive(:error) end - it 'calls Raven.capture_exception' do + it 'calls Sentry.capture_exception' do track_exception - expect(Raven).to have_received(:capture_exception).with( + expect(Sentry).to have_received(:capture_exception).with( exception, sentry_payload ) @@ -172,25 +176,31 @@ RSpec.describe Gitlab::ErrorTracking do context 'the exception implements :sentry_extra_data' do let(:extra_info) { { event: 'explosion', size: :massive } } - let(:exception) { double(message: 'bang!', sentry_extra_data: extra_info, backtrace: caller, cause: nil) } + + before do + allow(exception).to receive(:sentry_extra_data).and_return(extra_info) + end it 'includes the extra data from the exception in the tracking information' do track_exception - expect(Raven).to have_received(:capture_exception).with( + expect(Sentry).to have_received(:capture_exception).with( exception, a_hash_including(extra: a_hash_including(extra_info)) ) end end context 'the exception implements :sentry_extra_data, which returns nil' do - let(:exception) { double(message: 'bang!', sentry_extra_data: nil, backtrace: caller, cause: nil) } let(:extra) { { issue_url: issue_url } } + before do + allow(exception).to receive(:sentry_extra_data).and_return(nil) + end + it 'just includes the other extra info' do track_exception - expect(Raven).to have_received(:capture_exception).with( + expect(Sentry).to have_received(:capture_exception).with( exception, a_hash_including(extra: a_hash_including(extra)) ) end @@ -202,7 +212,7 @@ RSpec.describe Gitlab::ErrorTracking do it 'injects the normalized sql query into extra' do track_exception - expect(sentry_event.dig('extra', 'sql')).to eq('SELECT "users".* FROM "users" WHERE "users"."id" = $2 AND "users"."foo" = $1') + expect(sentry_event.extra[:sql]).to eq('SELECT "users".* FROM "users" WHERE "users"."id" = $2 AND "users"."foo" = $1') end end @@ -212,7 +222,7 @@ RSpec.describe Gitlab::ErrorTracking do track_exception - expect(sentry_event.dig('extra', 'sql')).to eq('SELECT "users".* FROM "users" WHERE "users"."id" = $2 AND "users"."foo" = $1') + expect(sentry_event.extra[:sql]).to eq('SELECT "users".* FROM "users" WHERE "users"."id" = $2 AND "users"."foo" = $1') end end end @@ -221,27 +231,27 @@ RSpec.describe Gitlab::ErrorTracking do subject(:track_exception) { described_class.track_exception(exception, extra) } before do - allow(Raven).to receive(:capture_exception).and_call_original + allow(Sentry).to receive(:capture_exception).and_call_original allow(Gitlab::ErrorTracking::Logger).to receive(:error) end - context 'custom GitLab context when using Raven.capture_exception directly' do - subject(:raven_capture_exception) { Raven.capture_exception(exception) } + context 'custom GitLab context when using Sentry.capture_exception directly' do + subject(:track_exception) { Sentry.capture_exception(exception) } it 'merges a default set of tags into the existing tags' do - allow(Raven.context).to receive(:tags).and_return(foo: 'bar') + Sentry.set_tags(foo: 'bar') - raven_capture_exception + track_exception - expect(sentry_event['tags']).to include('correlation_id', 'feature_category', 'foo', 'locale', 'program') + expect(sentry_event.tags).to include(:correlation_id, :feature_category, :foo, :locale, :program) end it 'merges the current user information into the existing user information' do - Raven.user_context(id: -1) + Sentry.set_user(id: -1) - raven_capture_exception + track_exception - expect(sentry_event['user']).to eq('id' => -1, 'username' => user.username) + expect(sentry_event.user).to eq(id: -1, username: user.username) end end @@ -265,7 +275,7 @@ RSpec.describe Gitlab::ErrorTracking do it 'does not filter parameters when sending to Sentry' do track_exception - expect(sentry_event.dig('extra', 'sidekiq', 'args')).to eq([1, { 'id' => 2, 'name' => 'hello' }, 'some-value', 'another-value']) + expect(sentry_event.extra[:sidekiq]['args']).to eq([1, { 'id' => 2, 'name' => 'hello' }, 'some-value', 'another-value']) end end @@ -275,7 +285,7 @@ RSpec.describe Gitlab::ErrorTracking do it 'filters sensitive arguments before sending and logging' do track_exception - expect(sentry_event.dig('extra', 'sidekiq', 'args')).to eq(['[FILTERED]', 1, 2]) + expect(sentry_event.extra[:sidekiq]['args']).to eq(['[FILTERED]', 1, 2]) expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with( hash_including( 'extra.sidekiq' => { @@ -295,8 +305,8 @@ RSpec.describe Gitlab::ErrorTracking do it 'sets the GRPC debug error string in the Sentry event and adds a custom fingerprint' do track_exception - expect(sentry_event.dig('extra', 'grpc_debug_error_string')).to eq('{"hello":1}') - expect(sentry_event['fingerprint']).to eq(['GRPC::DeadlineExceeded', '4:unknown cause.']) + expect(sentry_event.extra[:grpc_debug_error_string]).to eq('{"hello":1}') + expect(sentry_event.fingerprint).to eq(['GRPC::DeadlineExceeded', '4:unknown cause.']) end end @@ -306,8 +316,8 @@ RSpec.describe Gitlab::ErrorTracking do it 'does not do any processing on the event' do track_exception - expect(sentry_event['extra']).not_to include('grpc_debug_error_string') - expect(sentry_event['fingerprint']).to eq(['GRPC::DeadlineExceeded', '4:unknown cause']) + expect(sentry_event.extra).not_to include(:grpc_debug_error_string) + expect(sentry_event.fingerprint).to eq(['GRPC::DeadlineExceeded', '4:unknown cause']) end end end diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index ce0018d6d0d..3c690a767a6 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -require 'raven/transports/dummy' +require 'sentry/transport/dummy_transport' require_relative '../../../config/initializers/sentry' RSpec.describe API::Helpers do diff --git a/spec/support/shared_examples/features/discussion_comments_shared_example.rb b/spec/support/shared_examples/features/discussion_comments_shared_example.rb index 808e0be6be2..ff2878f77b4 100644 --- a/spec/support/shared_examples/features/discussion_comments_shared_example.rb +++ b/spec/support/shared_examples/features/discussion_comments_shared_example.rb @@ -11,6 +11,8 @@ RSpec.shared_examples 'thread comments for commit and snippet' do |resource_name let(:comment) { 'My comment' } it 'clicking "Comment" will post a comment' do + wait_for_all_requests + expect(page).to have_selector toggle_selector find("#{form_selector} .note-textarea").send_keys(comment) @@ -29,6 +31,8 @@ RSpec.shared_examples 'thread comments for commit and snippet' do |resource_name find("#{form_selector} .note-textarea").send_keys(comment) find(toggle_selector).click + + wait_for_all_requests end it 'has a "Comment" item (selected by default) and "Start thread" item' do diff --git a/spec/support/shared_examples/features/variable_list_shared_examples.rb b/spec/support/shared_examples/features/variable_list_shared_examples.rb index 4b94411f009..997500415a9 100644 --- a/spec/support/shared_examples/features/variable_list_shared_examples.rb +++ b/spec/support/shared_examples/features/variable_list_shared_examples.rb @@ -283,6 +283,8 @@ RSpec.shared_examples 'variable list' do end def fill_variable(key, value, protected: false, masked: false) + wait_for_requests + page.within('#add-ci-variable') do find('[data-qa-selector="ci_variable_key_field"] input').set(key) find('[data-qa-selector="ci_variable_value_field"]').set(value) if value.present?