Merge pull request #795 from rollbar/scrub-all-whitelist

Make scrub_all work correctly (including with whitelist)

Author: ArturMoczulski

lib/rollbar/scrubbers/params.rb

@@ -6,7 +6,8 @@ module Scrubbers
     # This class contains the logic to scrub the received parameters. It will
     # scrub the parameters matching Rollbar.configuration.scrub_fields Array.
     # Also, if that configuration option is set to :scrub_all, it will scrub all
-    # received parameters
+    # received parameters. It will not scrub anything that is in the scrub_whitelist
+    # configuration array even if :scrub_all is true.
     class Params
       SKIPPED_CLASSES = [::Tempfile]
       ATTACHMENT_CLASSES = %w(ActionDispatch::Http::UploadedFile Rack::Multipart::UploadedFile).freeze
@@ -33,37 +34,46 @@ def build_scrub_options(config, extra_fields, whitelist)
         ary_config = Array(config)
 
         {
-          :fields_regex => build_fields_regex(ary_config, extra_fields, whitelist),
-          :scrub_all => ary_config.include?(SCRUB_ALL)
+          :fields_regex => build_fields_regex(ary_config, extra_fields),
+          :scrub_all => ary_config.include?(SCRUB_ALL),
+          :whitelist => build_whitelist_regex(whitelist)
         }
       end
 
-      def build_fields_regex(config, extra_fields, whitelist)
+      def build_fields_regex(config, extra_fields)
         fields = config.find_all { |f| f.is_a?(String) || f.is_a?(Symbol) }
         fields += Array(extra_fields)
 
         return unless fields.any?
 
-        Regexp.new(fields.reject { |f| whitelist.include? f }.map { |val| Regexp.escape(val.to_s).to_s }.join('|'), true)
+        Regexp.new(fields.map { |val| Regexp.escape(val.to_s).to_s }.join('|'), true)
+      end
+
+      def build_whitelist_regex(whitelist)
+        fields = whitelist.find_all { |f| f.is_a?(String) || f.is_a?(Symbol) }
+        return unless fields.any?
+        Regexp.new(fields.map { |val| /\A#{Regexp.escape(val.to_s)}\z/ }.join('|'))
       end
 
       def scrub(params, options)
         fields_regex = options[:fields_regex]
         scrub_all = options[:scrub_all]
+        whitelist_regex = options[:whitelist]
 
         return scrub_array(params, options) if params.is_a?(Array)
 
         params.to_hash.inject({}) do |result, (key, value)|
-          if fields_regex === Rollbar::Encoding.encode(key).to_s
+          encoded_key = Rollbar::Encoding.encode(key).to_s
+          if (fields_regex === encoded_key) && !(whitelist_regex === encoded_key)
             result[key] = scrub_value(value)
           elsif value.is_a?(Hash)
             result[key] = scrub(value, options)
+          elsif scrub_all && !(whitelist_regex === encoded_key)
+            result[key] = scrub_value(value)
           elsif value.is_a?(Array)
             result[key] = scrub_array(value, options)
           elsif skip_value?(value)
             result[key] = "Skipped value of class '#{value.class.name}'"
-          elsif scrub_all
-            result[key] = scrub_value(value)
           else
             result[key] = rollbar_filtered_param_value(value)
           end

lib/rollbar/scrubbers/url.rb

@@ -6,6 +6,8 @@
 module Rollbar
   module Scrubbers
     class URL
+      SCRUB_ALL = :scrub_all
+
       def self.call(*args)
         new.call(*args)
       end
@@ -15,31 +17,39 @@ def call(options = {})
         return url unless Rollbar::LanguageSupport.can_scrub_url?
 
         filter(url,
-               build_regex(options[:scrub_fields], options[:whitelist] || []),
+               build_regex(options[:scrub_fields]),
                options[:scrub_user],
                options[:scrub_password],
-               options.fetch(:randomize_scrub_length, true))
+               options.fetch(:randomize_scrub_length, true),
+               options[:scrub_fields].include?(SCRUB_ALL),
+               build_whitelist_regex(options[:whitelist] || []))
       rescue => e
         Rollbar.logger.error("[Rollbar] There was an error scrubbing the url: #{e}, options: #{options.inspect}")
         url
       end
 
       private
 
-      def filter(url, regex, scrub_user, scrub_password, randomize_scrub_length)
+      def build_whitelist_regex(whitelist)
+        fields = whitelist.find_all { |f| f.is_a?(String) || f.is_a?(Symbol) }
+        return unless fields.any?
+        Regexp.new(fields.map { |val| /\A#{Regexp.escape(val.to_s)}\z/ }.join('|'))
+      end
+
+      def filter(url, regex, scrub_user, scrub_password, randomize_scrub_length, scrub_all, whitelist)
         uri = URI.parse(url)
 
         uri.user = filter_user(uri.user, scrub_user, randomize_scrub_length)
         uri.password = filter_password(uri.password, scrub_password, randomize_scrub_length)
-        uri.query = filter_query(uri.query, regex, randomize_scrub_length)
+        uri.query = filter_query(uri.query, regex, randomize_scrub_length, scrub_all, whitelist)
 
         uri.to_s
       end
 
       # Builds a regex to match with any of the received fields.
       # The built regex will also match array params like 'user_ids[]'.
-      def build_regex(fields, whitelist)
-        fields_or = fields.reject { |f| whitelist.include? f }.map { |field| "#{field}(\\[\\])?" }.join('|')
+      def build_regex(fields)
+        fields_or = fields.map { |field| "#{field}(\\[\\])?" }.join('|')
 
         Regexp.new("^#{fields_or}$")
       end
@@ -52,12 +62,12 @@ def filter_password(password, scrub_password, randomize_scrub_length)
         scrub_password && password ? filtered_value(password, randomize_scrub_length) : password
       end
 
-      def filter_query(query, regex, randomize_scrub_length)
+      def filter_query(query, regex, randomize_scrub_length, scrub_all, whitelist)
         return query unless query
 
         params = decode_www_form(query)
 
-        encoded_query = encode_www_form(filter_query_params(params, regex, randomize_scrub_length))
+        encoded_query = encode_www_form(filter_query_params(params, regex, randomize_scrub_length, scrub_all, whitelist))
 
         # We want this to rebuild array params like foo[]=1&foo[]=2
         URI.escape(CGI.unescape(encoded_query))
@@ -71,14 +81,14 @@ def encode_www_form(params)
         URI.encode_www_form(params)
       end
 
-      def filter_query_params(params, regex, randomize_scrub_length)
+      def filter_query_params(params, regex, randomize_scrub_length, scrub_all, whitelist)
         params.map do |key, value|
-          [key, filter_key?(key, regex) ? filtered_value(value, randomize_scrub_length) : value]
+          [key, filter_key?(key, regex, scrub_all, whitelist) ? filtered_value(value, randomize_scrub_length) : value]
         end
       end
 
-      def filter_key?(key, regex)
-        !!(key =~ regex)
+      def filter_key?(key, regex, scrub_all, whitelist)
+        !(whitelist === key) && (scrub_all || regex === key)
       end
 
       def filtered_value(value, randomize_scrub_length)

spec/rollbar/scrubbers/params_spec.rb

@@ -278,31 +278,140 @@
     context 'with :scrub_all option' do
       let(:scrub_config) { :scrub_all }
 
-      let(:params) do
-        {
-          :foo => 'bar',
-          :password => 'the-password',
-          :bar => 'foo',
-          :extra => {
-            :foo => 'more-foo',
-            :bar => 'more-bar'
+      context 'with simple hash' do
+        let(:params) do
+          {
+            :foo => 'bar',
+            :password => 'the-password',
+            :bar => 'foo',
+            :extra => {
+              :foo => 'more-foo',
+              :bar => 'more-bar'
+            }
           }
-        }
+        end
+        let(:result) do
+          {
+            :foo => /\*+/,
+            :password => /\*+/,
+            :bar => /\*+/,
+            :extra => {
+              :foo => /\*+/,
+              :bar => /\*+/
+            }
+          }
+        end
+
+        it 'scrubs the required parameters' do
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
+        end
       end
-      let(:result) do
-        {
-          :foo => /\*+/,
-          :password => /\*+/,
-          :bar => /\*+/,
-          :extra => {
+
+      context 'with nested arrays' do
+        let(:params) do
+          {
+            :foo => 'bar',
+            :password => 'the-password',
+            :bar => 'foo',
+            :extra => [
+              'hello world',
+              {
+                :foo => 'more-foo',
+                :bar => 'more-bar'
+              }
+            ]
+          }
+        end
+        let(:result) do
+          {
             :foo => /\*+/,
-            :bar => /\*+/
+            :password => /\*+/,
+            :bar => /\*+/,
+            :extra => /\*+/,
           }
-        }
+        end
+
+        it 'scrubs the required parameters' do
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
+        end
       end
 
-      it 'scrubs the required parameters' do
-        expect(subject.call(options)).to be_eql_hash_with_regexes(result)
+      context 'and with :whitelist option' do
+        let (:whitelist) { [:foo, :buzz] }
+
+        context 'with simple hash' do
+          let(:params) do
+            {
+              :foo => 'bar',
+              :password => 'the-password',
+              :bar => 'foo',
+              :extra => {
+                :foo => 'more-foo',
+                :bar => 'more-bar'
+              }
+            }
+          end
+          let(:result) do
+            {
+              :foo => 'bar',
+              :password => /\*+/,
+              :bar => /\*+/,
+              :extra => {
+                :foo => 'more-foo',
+                :bar => /\*+/
+              }
+            }
+          end
+
+          it 'scrubs the required parameters' do
+            expect(subject.call(options)).to be_eql_hash_with_regexes(result)
+          end
+        end
+
+        context 'with nested arrays' do
+          let(:params) do
+            {
+              :foo => 'bar',
+              :password => 'the-password',
+              :bar => 'foo',
+              :extra => [
+                'hello world',
+                {
+                  :foo => 'more-foo',
+                  :bar => 'more-bar'
+                }
+              ],
+              :buzz => [
+                'fizzbuzz',
+                {
+                  :a => 42,
+                  :foo => 'another-foo',
+                  :b => 'this should be scrubbed'
+                }
+              ]
+            }
+          end
+          let(:result) do
+            {
+              :foo => 'bar',
+              :password => /\*+/,
+              :bar => /\*+/,
+              :extra => /\*+/,
+              :buzz => [
+                'fizzbuzz',
+                {
+                  :a => /\*+/,
+                  :foo => 'another-foo',
+                  :b => /\*+/
+                }
+              ]
+            }
+          end
+
+          it 'scrubs the required parameters' do
+            expect(subject.call(options)).to be_eql_hash_with_regexes(result)
+          end
+        end
       end
     end
 
@@ -330,7 +439,7 @@
               :foo => 'bar',
               :secret => /\*+/,
               :password => 'the-password',
-              :password_confirmation => 'the-password'
+              :password_confirmation => /\*+/
             }
           ]
         end
@@ -354,7 +463,7 @@
             :foo => 'bar',
             :secret => /\*+/,
             :password => 'the-password',
-            :password_confirmation => 'the-password'
+            :password_confirmation => /\*+/
           }
         end
 
@@ -388,7 +497,7 @@
             :extra => {
               :secret => /\*+/,
               :password => 'the-password',
-              :password_confirmation => 'the-password'
+              :password_confirmation => /\*+/
             },
             :other => {
               :param => /\*+/,
@@ -427,7 +536,7 @@
             :extra => [{
               :secret => /\*+/,
               :password => 'the-password',
-              :password_confirmation => 'the-password'
+              :password_confirmation => /\*+/
             }],
             :other => [{
               :param => /\*+/,
@@ -460,7 +569,7 @@
             :extra => [{
               :secret => /\*+/,
               :password => 'the-password',
-              :password_confirmation => 'the-password',
+              :password_confirmation => /\*+/,
               :skipped => "Skipped value of class 'Tempfile'"
             }]
           }