Merge pull request #325 from rollbar/scrubbing-length-behavior

Make scrubbing length configurable

gemfiles/rails30.gemfile

@@ -14,4 +14,7 @@ gem 'rubinius-developer_tools', :platform => :rbx
 gem 'rails', '3.0.20'
 gem 'hitimes', '< 1.2.2'
 
+gem 'celluloid', '< 0.17.0' if RUBY_VERSION == '1.9.2'
+
+
 gemspec :path => '../'

lib/rollbar/configuration.rb

@@ -35,6 +35,7 @@ class Configuration
     attr_accessor :scrub_fields
     attr_accessor :scrub_user
     attr_accessor :scrub_password
+    attr_accessor :randomize_scrub_length
     attr_accessor :uncaught_exception_level
     attr_accessor :scrub_headers
     attr_accessor :use_async
@@ -82,6 +83,7 @@ def initialize
                        :confirm_password, :password_confirmation, :secret_token]
       @scrub_user = true
       @scrub_password = true
+      @randomize_scrub_length = true
       @uncaught_exception_level = 'error'
       @scrub_headers = ['Authorization']
       @safely = false

lib/rollbar/request_data_extractor.rb

@@ -32,7 +32,8 @@ def extract_request_data_from_rack(env)
 
       url_scrubber = Rollbar::Scrubbers::URL.new(:scrub_fields => sensitive_params,
                                                  :scrub_user => Rollbar.configuration.scrub_user,
-                                                 :scrub_password => Rollbar.configuration.scrub_password)
+                                                 :scrub_password => Rollbar.configuration.scrub_password,
+                                                 :randomize_scrub_length => Rollbar.configuration.randomize_scrub_length)
       url = url_scrubber.call(rollbar_url(env))
 
       params = request_params.merge(get_params).merge(post_params).merge(raw_body_params)
@@ -55,6 +56,14 @@ def extract_request_data_from_rack(env)
       data
     end
 
+    def rollbar_scrubbed(value)
+      if Rollbar.configuration.randomize_scrub_length
+        random_filtered_value
+      else
+        '*' * (value.length rescue 8)
+      end
+    end
+
     private
 
     def mergeable_raw_body_params(rack_req)
@@ -219,8 +228,8 @@ def sensitive_headers_list
       Rollbar.configuration.scrub_headers || []
     end
 
-    def rollbar_scrubbed(value)
-      '*' * (value.length rescue 8)
+    def random_filtered_value
+      '*' * (rand(5) + 3)
     end
 
     def skip_value?(value)

lib/rollbar/scrubbers/url.rb

@@ -9,11 +9,13 @@ 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)
       end
 
       def call(url)
@@ -39,11 +41,11 @@ def build_regex(fields)
       end
 
       def filter_user(user)
-        scrub_user && user ? filtered_value : user
+        scrub_user && user ? filtered_value(user) : user
       end
 
       def filter_password(password)
-        scrub_password && password ? filtered_value : password
+        scrub_password && password ? filtered_value(password) : password
       end
 
       def filter_query(query)
@@ -67,15 +69,23 @@ def encode_www_form(params)
 
       def filter_query_params(params)
         params.map do |key, value|
-          [key, filter_key?(key) ? filtered_value : value]
+          [key, filter_key?(key) ? filtered_value(value) : value]
         end
       end
 
       def filter_key?(key)
         !!(key =~ regex)
       end
 
-      def filtered_value
+      def filtered_value(value)
+        if randomize_scrub_length
+          random_filtered_value
+        else
+          '*' * (value.length rescue 8)
+        end
+      end
+
+      def random_filtered_value
         '*' * (rand(5) + 3)
       end
     end

spec/controllers/home_controller_spec.rb

@@ -67,10 +67,9 @@
         }
 
         filtered = controller.send( :rollbar_headers, headers )
-        filtered.should == {
-          "Authorization" => "*********",
-          "User-Agent" => "spec"
-        }
+
+        expect(filtered['Authorization']).to match(/\**/)
+        expect(filtered['User-Agent']).to be_eql('spec')
       end
 
       it 'should filter custom headers' do
@@ -85,11 +84,9 @@
         }
 
         filtered = controller.send( :rollbar_headers, headers )
-        filtered.should == {
-          "Auth" => "**********",
-          "Token" => "***********",
-          "Content-Type" => "text/html"
-        }
+        expect(filtered['Auth']).to match(/\**/)
+        expect(filtered['Token']).to match(/\**/)
+        expect(filtered['Content-Type']).to be_eql('text/html')
       end
 
     end
@@ -188,11 +185,11 @@
 
       filtered = Rollbar.last_report[:request][:params]
 
-      filtered["passwd"].should == "******"
-      filtered["password"].should == "******"
-      filtered["secret"].should == "******"
-      filtered["notpass"].should == "visible"
-      filtered["secret_token"].should == "*" * 32
+      expect(filtered["passwd"]).to match(/\**/)
+      expect(filtered["password"]).to match(/\**/)
+      expect(filtered["secret"]).to match(/\**/)
+      expect(filtered["notpass"]).to match(/\**/)
+      expect(filtered["secret_token"]).to match(/\**/)
     end
 
     it "should scrub custom scrub_fields" do
@@ -214,9 +211,9 @@
       filtered["passwd"].should == "visible"
       # config.filter_parameters is set to [:password] in
       # spec/dummyapp/config/application.rb
-      filtered["password"].should == "*******"
-      filtered["secret"].should == "******"
-      filtered["notpass"].should == "******"
+      expect(filtered["password"]).to match(/\**/)
+      expect(filtered["secret"]).to match(/\**/)
+      expect(filtered["notpass"]).to match(/\**/)
     end
   end
 

spec/rollbar/request_data_extractor_spec.rb

@@ -21,7 +21,8 @@ class ExtractorDummy
       scrubber_config = {
         :scrub_fields => kind_of(Array),
         :scrub_user => Rollbar.configuration.scrub_user,
-        :scrub_password => Rollbar.configuration.scrub_password
+        :scrub_password => Rollbar.configuration.scrub_password,
+        :randomize_scrub_length => Rollbar.configuration.randomize_scrub_length
       }
       expect(Rollbar::Scrubbers::URL).to receive(:new).with(scrubber_config).and_return(scrubber)
       expect(scrubber).to receive(:call).with(kind_of(String))
@@ -31,4 +32,30 @@ class ExtractorDummy
       expect(result).to be_kind_of(Hash)
     end
   end
+
+  describe '#rollbar_scrubbed_value' do
+    context 'with random scrub length' do
+      before do
+        allow(Rollbar.configuration).to receive(:randomize_scrub_length).and_return(true)
+      end
+
+      let(:value) { 'herecomesaverylongvalue' }
+
+      it 'randomizes the scrubbed string' do
+        expect(subject.rollbar_scrubbed(value)).to match(/\*{3,8}/)
+      end
+    end
+
+    context 'with no-random scrub length' do
+      before do
+        allow(Rollbar.configuration).to receive(:randomize_scrub_length).and_return(false)
+      end
+
+      let(:value) { 'herecomesaverylongvalue' }
+
+      it 'randomizes the scrubbed string' do
+        expect(subject.rollbar_scrubbed(value)).to match(/\*{#{value.length}}/)
+      end
+    end
+  end
 end

spec/rollbar/scrubbers/url_spec.rb

@@ -6,7 +6,8 @@
   let(:options) do
     { :scrub_fields => [:password, :secret],
       :scrub_user => false,
-      :scrub_password => false
+      :scrub_password => false,
+      :randomize_scrub_length => true
     }
   end
 
@@ -79,6 +80,24 @@
           end
         end
       end
+
+      context 'with no-random scrub length' do
+        let(:options) do
+          { :scrub_fields => [:password, :secret],
+            :scrub_user => false,
+            :scrub_password => false,
+            :randomize_scrub_length => false
+          }
+        end
+        let(:password) { 'longpasswordishere' }
+        let(:url) { "http://foo.com/some-interesting-path?foo=bar&password=#{password}#fragment" }
+
+        it 'scrubs with same length than the scrubbed param' do
+          expected_url = /http:\/\/foo.com\/some-interesting-path\?foo=bar&password=\*{#{password.length}}#fragment/
+
+          expect(subject.call(url)).to match(expected_url)
+        end
+      end
     end
   end
 end