Add latest changes from gitlab-org/security/gitlab@14-7-stable-ee

12 files changed, 321 insertions, 38 deletions

diff --git a/lib/gitlab/error_tracking.rb b/lib/gitlab/error_tracking.rb
index 6a637306225..fd0c407543d 100644
--- a/lib/gitlab/error_tracking.rb
+++ b/lib/gitlab/error_tracking.rb
@@ -19,7 +19,8 @@ module Gitlab
     PROCESSORS = [
       ::Gitlab::ErrorTracking::Processor::SidekiqProcessor,
       ::Gitlab::ErrorTracking::Processor::GrpcErrorProcessor,
-      ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor
+      ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor,
+      ::Gitlab::ErrorTracking::Processor::SanitizeErrorMessageProcessor
     ].freeze
 
     class << self
diff --git a/lib/gitlab/error_tracking/processor/concerns/processes_exceptions.rb b/lib/gitlab/error_tracking/processor/concerns/processes_exceptions.rb
new file mode 100644
index 00000000000..c2345395f5f
--- /dev/null
+++ b/lib/gitlab/error_tracking/processor/concerns/processes_exceptions.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module ErrorTracking
+    module Processor
+      module Concerns
+        module ProcessesExceptions
+          private
+
+          def extract_exceptions_from(event)
+            exceptions = event.instance_variable_get(:@interfaces)[:exception]&.values
+
+            Array.wrap(exceptions)
+          end
+
+          def valid_exception?(exception)
+            case exception
+            when Raven::SingleExceptionInterface
+              exception&.value.present?
+            else
+              false
+            end
+          end
+        end
+      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 e835deeea2c..3064d18bb93 100644
--- a/lib/gitlab/error_tracking/processor/grpc_error_processor.rb
+++ b/lib/gitlab/error_tracking/processor/grpc_error_processor.rb
@@ -4,6 +4,8 @@ module Gitlab
   module ErrorTracking
     module Processor
       module GrpcErrorProcessor
+        extend Gitlab::ErrorTracking::Processor::Concerns::ProcessesExceptions
+
         DEBUG_ERROR_STRING_REGEX = RE2('(.*) debug_error_string:(.*)')
 
         class << self
@@ -18,10 +20,7 @@ module Gitlab
           # 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
-
-            return unless exceptions.is_a?(Array)
-
+            exceptions = extract_exceptions_from(event)
             exception = exceptions.first
 
             return unless valid_exception?(exception)
@@ -68,15 +67,6 @@ module Gitlab
 
             [match[1], match[2]]
           end
-
-          def valid_exception?(exception)
-            case exception
-            when Raven::SingleExceptionInterface
-              exception&.value
-            else
-              false
-            end
-          end
         end
       end
     end
diff --git a/lib/gitlab/error_tracking/processor/sanitize_error_message_processor.rb b/lib/gitlab/error_tracking/processor/sanitize_error_message_processor.rb
new file mode 100644
index 00000000000..d53269aade1
--- /dev/null
+++ b/lib/gitlab/error_tracking/processor/sanitize_error_message_processor.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module ErrorTracking
+    module Processor
+      module SanitizeErrorMessageProcessor
+        extend Gitlab::ErrorTracking::Processor::Concerns::ProcessesExceptions
+
+        class << self
+          def call(event)
+            exceptions = extract_exceptions_from(event)
+
+            exceptions.each do |exception|
+              next unless valid_exception?(exception)
+
+              exception.value = Gitlab::Sanitizers::ExceptionMessage.clean(exception.type, exception.value)
+            end
+
+            event
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/exception_log_formatter.rb b/lib/gitlab/exception_log_formatter.rb
index 315574fed31..ce802b562f0 100644
--- a/lib/gitlab/exception_log_formatter.rb
+++ b/lib/gitlab/exception_log_formatter.rb
@@ -10,7 +10,7 @@ module Gitlab
         # Use periods to flatten the fields.
         payload.merge!(
           'exception.class' => exception.class.name,
-          'exception.message' => exception.message
+          'exception.message' => sanitize_message(exception)
         )
 
         if exception.backtrace
@@ -38,6 +38,10 @@ module Gitlab
       rescue PgQuery::ParseError
         sql
       end
+
+      def sanitize_message(exception)
+        Gitlab::Sanitizers::ExceptionMessage.clean(exception.class.name, exception.message)
+      end
     end
   end
 end
diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb
index dd7ec361dd8..d3b1bb6a57d 100644
--- a/lib/gitlab/import_export/members_mapper.rb
+++ b/lib/gitlab/import_export/members_mapper.rb
@@ -19,9 +19,8 @@ module Gitlab
             @exported_members.inject(missing_keys_tracking_hash) do |hash, member|
               if member['user']
                 old_user_id = member['user']['id']
-                old_user_email = member.dig('user', 'public_email') || member.dig('user', 'email')
-                existing_user = User.find_by(find_user_query(old_user_email)) if old_user_email
-                hash[old_user_id] = existing_user.id if existing_user && add_team_member(member, existing_user)
+                existing_user_id = existing_users_email_map[get_email(member)]
+                hash[old_user_id] = existing_user_id if existing_user_id && add_team_member(member, existing_user_id)
               else
                 add_team_member(member)
               end
@@ -72,11 +71,45 @@ module Gitlab
         member&.user == @user && member.access_level >= highest_access_level
       end
 
-      def add_team_member(member, existing_user = nil)
-        return true if existing_user && @importable.members.exists?(user_id: existing_user.id)
+      # Returns {email => user_id} hash where user_id is an ID at current instance
+      def existing_users_email_map
+        @existing_users_email_map ||= begin
+          emails = @exported_members.map { |member| get_email(member) }
+
+          User.by_user_email(emails).pluck(:email, :id).to_h
+        end
+      end
+
+      # Returns {user_id => email} hash where user_id is an ID at source "old" instance
+      def exported_members_email_map
+        @exported_members_email_map ||= begin
+          result = {}
+          @exported_members.each do |member|
+            email = get_email(member)
+
+            next unless email
+
+            result[member.dig('user', 'id')] = email
+          end
+
+          result
+        end
+      end
+
+      def get_email(member_data)
+        return unless member_data['user']
+
+        member_data.dig('user', 'public_email') || member_data.dig('user', 'email')
+      end
+
+      def add_team_member(member, existing_user_id = nil)
+        return true if existing_user_id && @importable.members.exists?(user_id: existing_user_id)
 
-        member['user'] = existing_user
         member_hash = member_hash(member)
+        if existing_user_id
+          member_hash.delete('user')
+          member_hash['user_id'] = existing_user_id
+        end
 
         member = relation_class.create(member_hash)
 
@@ -92,11 +125,19 @@ module Gitlab
       end
 
       def member_hash(member)
-        parsed_hash(member).merge(
+        result = parsed_hash(member).merge(
           'source_id' => @importable.id,
           'importing' => true,
           'access_level' => [member['access_level'], highest_access_level].min
         ).except('user_id')
+
+        if result['created_by_id']
+          created_by_email = exported_members_email_map[result['created_by_id']]
+
+          result['created_by_id'] = existing_users_email_map[created_by_email]
+        end
+
+        result
       end
 
       def parsed_hash(member)
@@ -104,14 +145,6 @@ module Gitlab
                                                      relation_class: relation_class)
       end
 
-      def find_user_query(email)
-        user_arel[:email].eq(email)
-      end
-
-      def user_arel
-        @user_arel ||= User.arel_table
-      end
-
       def relation_class
         case @importable
         when ::Project
@@ -143,7 +176,7 @@ module Gitlab
 
       def base_log_params(member_hash)
         {
-          user_id: member_hash['user']&.id,
+          user_id: member_hash['user_id'],
           access_level: member_hash['access_level'],
           importable_type: @importable.class.to_s,
           importable_id: @importable.id,
diff --git a/lib/gitlab/sanitizers/exception_message.rb b/lib/gitlab/sanitizers/exception_message.rb
new file mode 100644
index 00000000000..11c91093d88
--- /dev/null
+++ b/lib/gitlab/sanitizers/exception_message.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Sanitizers
+    module ExceptionMessage
+      FILTERED_STRING = '[FILTERED]'
+      EXCEPTION_NAMES = %w(URI::InvalidURIError Addressable::URI::InvalidURIError).freeze
+      MESSAGE_REGEX = %r{(\A[^:]+:\s).*\Z}.freeze
+
+      class << self
+        def clean(exception_name, message)
+          return message unless exception_name.in?(EXCEPTION_NAMES)
+
+          message.sub(MESSAGE_REGEX, '\1' + FILTERED_STRING)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/error_tracking/processor/sanitize_error_message_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/sanitize_error_message_processor_spec.rb
new file mode 100644
index 00000000000..b4173617a14
--- /dev/null
+++ b/spec/lib/gitlab/error_tracking/processor/sanitize_error_message_processor_spec.rb
@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::ErrorTracking::Processor::SanitizeErrorMessageProcessor, :sentry do
+  describe '.call' do
+    let(:exception) { StandardError.new('raw error') }
+    let(:event) { Raven::Event.from_exception(exception, raven_required_options) }
+    let(:result_hash) { described_class.call(event).to_hash }
+    let(:raven_required_options) do
+      {
+        configuration: Raven.configuration,
+        context: Raven.context,
+        breadcrumbs: Raven.breadcrumbs
+      }
+    end
+
+    it 'cleans the exception message' do
+      expect(Gitlab::Sanitizers::ExceptionMessage).to receive(:clean).with('StandardError', 'raw error').and_return('cleaned')
+
+      expect(result_hash[:exception][:values].first).to include(
+        type: 'StandardError',
+        value: 'cleaned'
+      )
+    end
+
+    context 'when event is invalid' do
+      let(:event) { instance_double('Raven::Event', to_hash: { invalid: true }) }
+
+      it 'does nothing' do
+        extracted_exception = instance_double('Raven::SingleExceptionInterface', value: nil)
+        allow(described_class).to receive(:extract_exceptions_from).and_return([extracted_exception])
+
+        expect(Gitlab::Sanitizers::ExceptionMessage).not_to receive(:clean)
+        expect(result_hash).to eq(invalid: true)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/error_tracking_spec.rb b/spec/lib/gitlab/error_tracking_spec.rb
index a5d44963f4b..284b773ef24 100644
--- a/spec/lib/gitlab/error_tracking_spec.rb
+++ b/spec/lib/gitlab/error_tracking_spec.rb
@@ -301,5 +301,48 @@ RSpec.describe Gitlab::ErrorTracking do
         end
       end
     end
+
+    context 'when processing invalid URI exceptions' do
+      let(:invalid_uri) { 'http://foo:bar' }
+      let(:sentry_exception_values) { sentry_event['exception']['values'] }
+
+      context 'when the error is a URI::InvalidURIError' do
+        let(:exception) do
+          URI.parse(invalid_uri)
+        rescue URI::InvalidURIError => error
+          error
+        end
+
+        it 'filters the URI from the error message' do
+          track_exception
+
+          expect(sentry_exception_values).to include(
+            hash_including(
+              'type' => 'URI::InvalidURIError',
+              'value' => 'bad URI(is not URI?): [FILTERED]'
+            )
+          )
+        end
+      end
+
+      context 'when the error is a Addressable::URI::InvalidURIError' do
+        let(:exception) do
+          Addressable::URI.parse(invalid_uri)
+        rescue Addressable::URI::InvalidURIError => error
+          error
+        end
+
+        it 'filters the URI from the error message' do
+          track_exception
+
+          expect(sentry_exception_values).to include(
+            hash_including(
+              'type' => 'Addressable::URI::InvalidURIError',
+              'value' => 'Invalid port number: [FILTERED]'
+            )
+          )
+        end
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/exception_log_formatter_spec.rb b/spec/lib/gitlab/exception_log_formatter_spec.rb
index beeeeb2b64c..7dda56f0bf5 100644
--- a/spec/lib/gitlab/exception_log_formatter_spec.rb
+++ b/spec/lib/gitlab/exception_log_formatter_spec.rb
@@ -22,6 +22,14 @@ RSpec.describe Gitlab::ExceptionLogFormatter do
       expect(payload['exception.sql']).to be_nil
     end
 
+    it 'cleans the exception message' do
+      expect(Gitlab::Sanitizers::ExceptionMessage).to receive(:clean).with('RuntimeError', 'bad request').and_return('cleaned')
+
+      described_class.format!(exception, payload)
+
+      expect(payload['exception.message']).to eq('cleaned')
+    end
+
     context 'when exception is ActiveRecord::StatementInvalid' do
       let(:exception) { ActiveRecord::StatementInvalid.new(sql: 'SELECT "users".* FROM "users" WHERE "users"."id" = 1 AND "users"."foo" = $1') }
 
diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb
index 8b9ca90a280..1085cde767d 100644
--- a/spec/lib/gitlab/import_export/members_mapper_spec.rb
+++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb
@@ -17,7 +17,7 @@ RSpec.describe Gitlab::ImportExport::MembersMapper do
            "notification_level" => 3,
            "created_at" => "2016-03-11T10:21:44.822Z",
            "updated_at" => "2016-03-11T10:21:44.822Z",
-           "created_by_id" => nil,
+           "created_by_id" => 1,
            "invite_email" => nil,
            "invite_token" => nil,
            "invite_accepted_at" => nil,
@@ -38,10 +38,24 @@ RSpec.describe Gitlab::ImportExport::MembersMapper do
            "notification_level" => 3,
            "created_at" => "2016-03-11T10:21:44.822Z",
            "updated_at" => "2016-03-11T10:21:44.822Z",
-           "created_by_id" => 1,
+           "created_by_id" => 2,
            "invite_email" => 'invite@test.com',
            "invite_token" => 'token',
            "invite_accepted_at" => nil
+         },
+         {
+           "id" => 3,
+           "access_level" => 40,
+           "source_id" => 14,
+           "source_type" => source_type,
+           "user_id" => nil,
+           "notification_level" => 3,
+           "created_at" => "2016-03-11T10:21:44.822Z",
+           "updated_at" => "2016-03-11T10:21:44.822Z",
+           "created_by_id" => nil,
+           "invite_email" => 'invite2@test.com',
+           "invite_token" => 'token',
+           "invite_accepted_at" => nil
          }]
       end
 
@@ -68,12 +82,37 @@ RSpec.describe Gitlab::ImportExport::MembersMapper do
         expect(member_class.find_by_invite_email('invite@test.com')).not_to be_nil
       end
 
-      it 'removes old user_id from member_hash to avoid conflict with user key' do
+      it 'maps created_by_id to user on new instance' do
+        expect(member_class)
+          .to receive(:create)
+            .once
+            .with(hash_including('user_id' => user2.id, 'created_by_id' => nil))
+            .and_call_original
+        expect(member_class)
+          .to receive(:create)
+            .once
+            .with(hash_including('invite_email' => 'invite@test.com', 'created_by_id' => nil))
+            .and_call_original
+        expect(member_class)
+          .to receive(:create)
+            .once
+            .with(hash_including('invite_email' => 'invite2@test.com', 'created_by_id' => nil))
+            .and_call_original
+
+        members_mapper.map
+      end
+
+      it 'replaced user_id with user_id from new instance' do
+        expect(member_class)
+          .to receive(:create)
+            .once
+            .with(hash_including('user_id' => user2.id))
+            .and_call_original
         expect(member_class)
           .to receive(:create)
-                .twice
-                .with(hash_excluding('user_id'))
-                .and_call_original
+            .twice
+            .with(hash_excluding('user_id'))
+            .and_call_original
 
         members_mapper.map
       end
@@ -99,7 +138,7 @@ RSpec.describe Gitlab::ImportExport::MembersMapper do
           end
 
           expect(logger).to receive(:info).with(hash_including(expected_log_params.call(user2.id))).once
-          expect(logger).to receive(:info).with(hash_including(expected_log_params.call(nil))).once
+          expect(logger).to receive(:info).with(hash_including(expected_log_params.call(nil))).twice
 
           members_mapper.map
         end
diff --git a/spec/lib/gitlab/sanitizers/exception_message_spec.rb b/spec/lib/gitlab/sanitizers/exception_message_spec.rb
new file mode 100644
index 00000000000..8b54b353235
--- /dev/null
+++ b/spec/lib/gitlab/sanitizers/exception_message_spec.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+RSpec.describe Gitlab::Sanitizers::ExceptionMessage do
+  describe '.clean' do
+    let(:exception_name) { exception.class.name }
+    let(:exception_message) { exception.message }
+
+    subject { described_class.clean(exception_name, exception_message) }
+
+    context 'when error is a URI::InvalidURIError' do
+      let(:exception) do
+        URI.parse('http://foo:bar')
+      rescue URI::InvalidURIError => error
+        error
+      end
+
+      it { is_expected.to eq('bad URI(is not URI?): [FILTERED]') }
+    end
+
+    context 'when error is an Addressable::URI::InvalidURIError' do
+      using RSpec::Parameterized::TableSyntax
+
+      let(:exception) do
+        Addressable::URI.parse(uri)
+      rescue Addressable::URI::InvalidURIError => error
+        error
+      end
+
+      where(:uri, :result) do
+        'http://foo:bar' | 'Invalid port number: [FILTERED]'
+        'http://foo:%eb' | 'Invalid encoding in port'
+        'ht%0atp://foo'  | 'Invalid scheme format: [FILTERED]'
+        'http:'          | 'Absolute URI missing hierarchical segment: [FILTERED]'
+        '::http'         | 'Cannot assemble URI string with ambiguous path: [FILTERED]'
+        'http://foo bar' | 'Invalid character in host: [FILTERED]'
+      end
+
+      with_them do
+        it { is_expected.to eq(result) }
+      end
+    end
+
+    context 'with any other exception' do
+      let(:exception) { StandardError.new('Error message: http://foo@bar:baz@ex:ample.com') }
+
+      it 'is not invoked and does nothing' do
+        is_expected.to eq('Error message: http://foo@bar:baz@ex:ample.com')
+      end
+    end
+  end
+end