Fix URL scrubbing and change to a functional object

After one scrubbing refactor we starting not passing the scrubing
configuration to the URL scrubber. This is fixed with this PR.

Also, since params scrubbers was designed as a functional object, URL
scrubber is not also a functional object without state. Just to maintain
the style for them.

Some tests have been added to the scrubbers and the request data
extractor module

lib/rollbar/request_data_extractor.rb

@@ -23,20 +23,15 @@ def extract_request_data_from_rack(env)
       rack_req = ::Rack::Request.new(env)
 
       sensitive_params = sensitive_params_list(env)
-      request_params = rollbar_filtered_params(sensitive_params, rollbar_request_params(env))
-      get_params = rollbar_filtered_params(sensitive_params, rollbar_get_params(rack_req))
-      post_params = rollbar_filtered_params(sensitive_params, rollbar_post_params(rack_req))
-      raw_body_params = rollbar_filtered_params(sensitive_params, mergeable_raw_body_params(rack_req))
-      cookies = rollbar_filtered_params(sensitive_params, rollbar_request_cookies(rack_req))
-      session = rollbar_filtered_params(sensitive_params, rollbar_request_session(rack_req))
-      route_params = rollbar_filtered_params(sensitive_params, rollbar_route_params(env))
-
-      url_scrubber = Rollbar::Scrubbers::URL.new(:scrub_fields => sensitive_params,
-                                                 :scrub_user => Rollbar.configuration.scrub_user,
-                                                 :scrub_password => Rollbar.configuration.scrub_password,
-                                                 :randomize_scrub_length => Rollbar.configuration.randomize_scrub_length)
-      url = url_scrubber.call(rollbar_url(env))
-
+      request_params = scrub_params(rollbar_request_params(env), sensitive_params)
+      get_params = scrub_params(rollbar_get_params(rack_req), sensitive_params)
+      post_params = scrub_params(rollbar_post_params(rack_req), sensitive_params)
+      raw_body_params = scrub_params(mergeable_raw_body_params(rack_req), sensitive_params)
+      cookies = scrub_params(rollbar_request_cookies(rack_req), sensitive_params)
+      session = scrub_params(rollbar_request_session(rack_req), sensitive_params)
+      route_params = scrub_params(rollbar_route_params(env), sensitive_params)
+
+      url = scrub_url(rollbar_url(env), sensitive_params)
       params = request_params.merge(get_params).merge(post_params).merge(raw_body_params)
 
       data = {
@@ -57,6 +52,27 @@ def extract_request_data_from_rack(env)
       data
     end
 
+    def scrub_url(url, sensitive_params)
+      options = {
+        :url => url,
+        :scrub_fields => Array(Rollbar.configuration.scrub_fields) + sensitive_params,
+        :scrub_user => Rollbar.configuration.scrub_user,
+        :scrub_password => Rollbar.configuration.scrub_password,
+        :randomize_scrub_length => Rollbar.configuration.randomize_scrub_length
+      }
+
+      Rollbar::Scrubbers::URL.call(options)
+    end
+
+    def scrub_params(params, sensitive_params)
+      options = {
+        :params => params,
+        :config => Rollbar.configuration.scrub_fields,
+        :extra_fields => sensitive_params
+      }
+      Rollbar::Scrubbers::Params.call(options)
+    end
+
     private
 
     def mergeable_raw_body_params(rack_req)
@@ -180,10 +196,6 @@ def rollbar_request_cookies(rack_req)
       {}
     end
 
-    def rollbar_filtered_params(sensitive_params, params)
-      Rollbar::Scrubbers::Params.call(params, sensitive_params)
-    end
-
     def sensitive_params_list(env)
       Array(env['action_dispatch.parameter_filter'])
     end

lib/rollbar/scrubbers/params.rb

@@ -15,10 +15,12 @@ def self.call(*args)
         new.call(*args)
       end
 
-      def call(params, extra_fields = [])
+      def call(options = {})
+        params = options[:params]
         return {} unless params
 
-        config = Rollbar.configuration.scrub_fields
+        config = options[:config]
+        extra_fields = options[:extra_fields]
 
         scrub(params, build_scrub_options(config, extra_fields))
       end

lib/rollbar/scrubbers/url.rb

@@ -6,34 +6,36 @@
 module Rollbar
   module Scrubbers
     class URL
-      attr_reader :regex
-      attr_reader :scrub_user
-      attr_reader :scrub_password
-      attr_reader :randomize_scrub_length
-
-      def initialize(options = {})
-        @regex = build_regex(options[:scrub_fields])
-        @scrub_user = options[:scrub_user]
-        @scrub_password = options[:scrub_password]
-        @randomize_scrub_length = options.fetch(:randomize_scrub_length, true)
+      def self.call(*args)
+        new.call(*args)
       end
 
-      def call(url)
+      def call(options = {})
+        url = options[:url]
         return url unless Rollbar::LanguageSupport.can_scrub_url?
 
+        filter(url,
+               build_regex(options[:scrub_fields]),
+               options[:scrub_user],
+               options[:scrub_password],
+               options.fetch(:randomize_scrub_length, true))
+      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)
         uri = URI.parse(url)
 
-        uri.user = filter_user(uri.user)
-        uri.password = filter_password(uri.password)
-        uri.query = filter_query(uri.query)
+        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.to_s
-      rescue
-        url
       end
 
-      private
-
       # 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)
@@ -42,20 +44,20 @@ def build_regex(fields)
         Regexp.new("^#{fields_or}$")
       end
 
-      def filter_user(user)
-        scrub_user && user ? filtered_value(user) : user
+      def filter_user(user, scrub_user, randomize_scrub_length)
+        scrub_user && user ? filtered_value(user, randomize_scrub_length) : user
       end
 
-      def filter_password(password)
-        scrub_password && password ? filtered_value(password) : password
+      def filter_password(password, scrub_password, randomize_scrub_length)
+        scrub_password && password ? filtered_value(password, randomize_scrub_length) : password
       end
 
-      def filter_query(query)
+      def filter_query(query, regex, randomize_scrub_length)
         return query unless query
 
         params = decode_www_form(query)
 
-        encoded_query = encode_www_form(filter_query_params(params))
+        encoded_query = encode_www_form(filter_query_params(params, regex, randomize_scrub_length))
 
         # We want this to rebuild array params like foo[]=1&foo[]=2
         CGI.unescape(encoded_query)
@@ -69,17 +71,17 @@ def encode_www_form(params)
         URI.encode_www_form(params)
       end
 
-      def filter_query_params(params)
+      def filter_query_params(params, regex, randomize_scrub_length)
         params.map do |key, value|
-          [key, filter_key?(key) ? filtered_value(value) : value]
+          [key, filter_key?(key, regex) ? filtered_value(value, randomize_scrub_length) : value]
         end
       end
 
-      def filter_key?(key)
+      def filter_key?(key, regex)
         !!(key =~ regex)
       end
 
-      def filtered_value(value)
+      def filtered_value(value, randomize_scrub_length)
         if randomize_scrub_length
           random_filtered_value
         else

spec/rollbar/request_data_extractor_spec.rb

@@ -14,18 +14,64 @@ class ExtractorDummy
     Rack::MockRequest.env_for('/', 'HTTP_HOST' => 'localhost:81', 'HTTP_X_FORWARDED_HOST' => 'example.org:9292')
   end
 
-  describe '#extract_request_data_from_rack' do
-    let(:scrubber) { double }
+  describe '#scrub_url' do
+    let(:url) { 'http://this-is-the-url.com/foobar?param1=value1' }
+    let(:sensitive_params) { [:param1, :param2] }
+    let(:scrub_fields) { [:password, :secret] }
 
-    it 'returns a Hash object' do
-      scrubber_config = {
-        :scrub_fields => kind_of(Array),
-        :scrub_user => Rollbar.configuration.scrub_user,
-        :scrub_password => Rollbar.configuration.scrub_password,
-        :randomize_scrub_length => Rollbar.configuration.randomize_scrub_length
+    before do
+      allow(Rollbar.configuration).to receive(:scrub_fields).and_return(scrub_fields)
+      allow(Rollbar.configuration).to receive(:scrub_user).and_return(true)
+      allow(Rollbar.configuration).to receive(:scrub_password).and_return(true)
+      allow(Rollbar.configuration).to receive(:randomize_secret_length).and_return(true)
+    end
+
+    it 'calls the scrubber with the correct options' do
+      expected_options = {
+        :url => url,
+        :scrub_fields => [:password, :secret, :param1, :param2],
+        :scrub_user => true,
+        :scrub_password => true,
+        :randomize_scrub_length => true
+      }
+
+      expect(Rollbar::Scrubbers::URL).to receive(:call).with(expected_options)
+
+      subject.scrub_url(url, sensitive_params)
+    end
+  end
+
+  describe '#scrub_params' do
+    let(:params) do
+      {
+        :param1 => 'value1',
+        :param2 => 'value2'
+      }
+    end
+    let(:sensitive_params) { [:param1, :param2] }
+    let(:scrub_fields) { [:password, :secret] }
+
+    before do
+      allow(Rollbar.configuration).to receive(:scrub_fields).and_return(scrub_fields)
+    end
+
+    it 'calls the scrubber with the correct options' do
+      expected_options = {
+        :params => params,
+        :config => scrub_fields,
+        :extra_fields => sensitive_params
       }
-      expect(Rollbar::Scrubbers::URL).to receive(:new).with(scrubber_config).and_return(scrubber)
-      expect(scrubber).to receive(:call).with(kind_of(String))
+
+      expect(Rollbar::Scrubbers::Params).to receive(:call).with(expected_options)
+
+      subject.scrub_params(params, sensitive_params)
+    end
+  end
+
+  describe '#extract_request_data_from_rack' do
+    it 'returns a Hash object' do
+      expect(Rollbar::Scrubbers::URL).to receive(:call).with(kind_of(Hash)).and_call_original
+      expect(Rollbar::Scrubbers::Params).to receive(:call).with(kind_of(Hash)).and_call_original.exactly(7)
 
       result = subject.extract_request_data_from_rack(env)
 

spec/rollbar/scrubbers/params_spec.rb

@@ -15,8 +15,11 @@
   end
 
   describe '#call' do
-    before do
-      allow(Rollbar.configuration).to receive(:scrub_fields).and_return(scrub_config)
+    let(:options) do
+      {
+        :params => params,
+        :config => scrub_config
+      }
     end
 
     context 'with scrub fields configured' do
@@ -43,7 +46,7 @@
         end
 
         it 'scrubs the required parameters' do
-          expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
         end
       end
 
@@ -70,7 +73,7 @@
         end
 
         it 'scrubs the required parameters' do
-          expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
         end
       end
 
@@ -97,7 +100,7 @@
         end
 
         it 'scrubs the required parameters' do
-          expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
         end
       end
 
@@ -129,7 +132,7 @@
         after { tempfile.close }
 
         it 'scrubs the required parameters' do
-          expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
         end
       end
 
@@ -169,7 +172,7 @@
         end
 
         it 'scrubs the required parameters' do
-          expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
         end
 
         context 'if getting the attachment values fails' do
@@ -204,7 +207,7 @@
           end
 
           it 'scrubs the required parameters' do
-            expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+            expect(subject.call(options)).to be_eql_hash_with_regexes(result)
           end
         end
       end
@@ -218,7 +221,7 @@
         end
 
         it 'scrubs the required parameters' do
-          expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+          expect(subject.call(options)).to be_eql_hash_with_regexes(result)
         end
       end
     end
@@ -249,7 +252,7 @@
       end
 
       it 'scrubs the required parameters' do
-        expect(subject.call(params)).to be_eql_hash_with_regexes(result)
+        expect(subject.call(options)).to be_eql_hash_with_regexes(result)
       end
     end
   end