Filter Sidekiq arguments in Sentry

Filter out sensitive Sidekiq arguments from being sent to Sentry. By
default, only numeric arguments are sent. Overrides are possible for
individual worker classes if they have safe non-numeric arguments.

lib/gitlab/error_tracking.rb

@@ -31,6 +31,8 @@ module Gitlab
           config.sanitize_http_headers = %w[Authorization Private-Token]
           config.tags = { program: Gitlab.process_name }
           config.before_send = method(:before_send)
+
+          yield config if block_given?
         end
       end
 

lib/gitlab/error_tracking/processor/sidekiq_processor.rb

 # frozen_string_literal: true
 
+require 'set'
+
 module Gitlab
   module ErrorTracking
     module Processor
       class SidekiqProcessor < ::Raven::Processor
+        FILTERED_STRING = '[FILTERED]'
+        PERMITTED_ARGUMENTS = Hash.new(Set.new).merge(
+          'PostReceive' => [0, 1, 2, 3],
+          'SystemHookPushWorker' => [1],
+          'WebHookWorker' => [2]
+        ).transform_values!(&:to_set).freeze
+
+        def self.filter_arguments(args, klass)
+          args.lazy.with_index.map do |arg, i|
+            case arg
+            when Numeric
+              arg
+            else
+              if PERMITTED_ARGUMENTS[klass].include?(i)
+                arg
+              else
+                FILTERED_STRING
+              end
+            end
+          end
+        end
+
         def process(value, key = nil)
           sidekiq = value.dig(:extra, :sidekiq)
 
           return value unless sidekiq
 
-          sidekiq = sidekiq.dup
+          sidekiq = sidekiq.deep_dup
           sidekiq.delete(:jobstr)
 
+          # 'args' in this hash => from Gitlab::ErrorTracking.track_*
+          # 'args' in :job => from default error handler
+          job_holder = sidekiq.key?('args') ? sidekiq : sidekiq[:job]
+
+          if job_holder['args']
+            job_holder['args'] = self.class.filter_arguments(job_holder['args'], job_holder['class']).to_a
+          end
+
           value[:extra][:sidekiq] = sidekiq
 
           value

spec/lib/gitlab/error_tracking/processor/sidekiq_processor_spec.rb

@@ -6,8 +6,76 @@ require 'rspec-parameterized'
 require 'raven'
 
 RSpec.describe Gitlab::ErrorTracking::Processor::SidekiqProcessor do
+  describe '.filter_arguments' do
+    it 'returns a lazy enumerator' do
+      filtered = described_class.filter_arguments([1, 'string'], 'TestWorker')
+
+      expect(filtered).to be_a(Enumerator::Lazy)
+      expect(filtered.to_a).to eq([1, described_class::FILTERED_STRING])
+    end
+
+    context 'arguments filtering' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:klass, :expected) do
+        'UnknownWorker' | [1, described_class::FILTERED_STRING, described_class::FILTERED_STRING, described_class::FILTERED_STRING]
+        'NoPermittedArguments' | [1, described_class::FILTERED_STRING, described_class::FILTERED_STRING, described_class::FILTERED_STRING]
+        'OnePermittedArgument' | [1, 'string', described_class::FILTERED_STRING, described_class::FILTERED_STRING]
+        'AllPermittedArguments' | [1, 'string', [1, 2], { a: 1 }]
+      end
+
+      with_them do
+        before do
+          permitted_arguments = Hash.new(Set.new).merge(
+            'NoPermittedArguments' => [],
+            'OnePermittedArgument' => [1],
+            'AllPermittedArguments' => [0, 1, 2, 3]
+          ).transform_values!(&:to_set)
+
+          stub_const("#{described_class}::PERMITTED_ARGUMENTS", permitted_arguments)
+        end
+
+        it do
+          expect(described_class.filter_arguments([1, 'string', [1, 2], { a: 1 }], klass).to_a)
+            .to eq(expected)
+        end
+      end
+    end
+  end
+
   describe '#process' do
     context 'when there is Sidekiq data' do
+      shared_examples 'Sidekiq arguments' do |args_in_job_hash: true|
+        let(:path) { [:extra, :sidekiq, args_in_job_hash ? :job : nil, 'args'].compact }
+        let(:args) { [1, 'string', { a: 1 }, [1, 2]] }
+
+        it 'only allows numeric arguments for an unknown worker' do
+          value = { 'args' => args, 'class' => 'UnknownWorker' }
+
+          value = { job: value } if args_in_job_hash
+
+          expect(subject.process(extra_sidekiq(value)).dig(*path))
+            .to eq([1, described_class::FILTERED_STRING, described_class::FILTERED_STRING, described_class::FILTERED_STRING])
+        end
+
+        it 'allows all argument types for a permitted worker' do
+          value = { 'args' => args, 'class' => 'PostReceive' }
+
+          value = { job: value } if args_in_job_hash
+
+          expect(subject.process(extra_sidekiq(value)).dig(*path))
+            .to eq(args)
+        end
+      end
+
+      context 'when processing via the default error handler' do
+        include_examples 'Sidekiq arguments', args_in_job_hash: true
+      end
+
+      context 'when processing via Gitlab::ErrorTracking' do
+        include_examples 'Sidekiq arguments', args_in_job_hash: false
+      end
+
       it 'removes a jobstr field if present' do
         value = {
           job: { 'args' => [1] },

spec/lib/gitlab/error_tracking_spec.rb

@@ -2,6 +2,8 @@
 
 require 'spec_helper'
 
+require 'raven/transports/dummy'
+
 describe Gitlab::ErrorTracking do
   let(:exception) { RuntimeError.new('boom') }
   let(:issue_url) { 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' }
@@ -22,7 +24,9 @@ describe Gitlab::ErrorTracking do
     allow(described_class).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn)
     allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid')
 
-    described_class.configure
+    described_class.configure do |config|
+      config.encoding = 'json'
+    end
   end
 
   describe '.with_context' do
@@ -181,14 +185,24 @@ describe Gitlab::ErrorTracking do
     end
 
     context 'with sidekiq args' do
-      let(:extra) { { sidekiq: { 'args' => [1, { 'id' => 2, 'name' => 'hello' }, 'some-value', 'another-value'] } } }
-
       it 'ensures extra.sidekiq.args is a string' do
+        extra = { sidekiq: { 'args' => [1, { 'id' => 2, 'name' => 'hello' }, 'some-value', 'another-value'] } }
+
         expect(Gitlab::ErrorTracking::Logger).to receive(:error).with(
           hash_including({ 'extra.sidekiq' => { 'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value'] } }))
 
         described_class.track_exception(exception, extra)
       end
+
+      it 'filters sensitive arguments before sending' do
+        extra = { sidekiq: { 'class' => 'UnknownWorker', 'args' => ['sensitive string', 1, 2] } }
+
+        described_class.track_exception(exception, extra)
+
+        sentry_event = Gitlab::Json.parse(Raven.client.transport.events.last[1])
+
+        expect(sentry_event.dig('extra', 'sidekiq', 'args')).to eq(['[FILTERED]', 1, 2])
+      end
     end
   end
 end