Move filter_parameter_logging logic out of the controller and create ActionDispatch::ParametersFilter to handle parameter filteration instead. This will make filteration not depending on controller anymore.

Signed-off-by: José Valim <jose.valim@gmail.com>

8 files changed, 130 insertions, 57 deletions

diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
index 9e03f50759..befb4a58cc 100644
--- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
+++ b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
@@ -2,8 +2,6 @@ module ActionController
   module FilterParameterLogging
     extend ActiveSupport::Concern
 
-    INTERNAL_PARAMS = %w(controller action format _method only_path)
-
     module ClassMethods
       # Replace sensitive parameter data from the request log.
       # Filters parameters that have any of the arguments as a substring.
@@ -27,40 +25,14 @@ module ActionController
       #   => reverses the value to all keys matching /secret/i, and
       #      replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
       def filter_parameter_logging(*filter_words, &block)
-        raise "You must filter at least one word from logging" if filter_words.empty?
-
-        parameter_filter = Regexp.new(filter_words.join('|'), true)
-
-        define_method(:filter_parameters) do |original_params|
-          filtered_params = {}
-
-          original_params.each do |key, value|
-            if key =~ parameter_filter
-              value = '[FILTERED]'
-            elsif value.is_a?(Hash)
-              value = filter_parameters(value)
-            elsif value.is_a?(Array)
-              value = value.map { |item| filter_parameters(item) }
-            elsif block_given?
-              key = key.dup
-              value = value.dup if value.duplicable?
-              yield key, value
-            end
-
-            filtered_params[key] = value
-          end
-
-          filtered_params.except!(*INTERNAL_PARAMS)
-        end
-        protected :filter_parameters
+        ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words, &block)
       end
     end
-
+    
   protected
-
+    
     def filter_parameters(params)
-      params.dup.except!(*INTERNAL_PARAMS)
+      request.send(:process_parameter_filter, params)
     end
-
   end
 end
diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb
index 19c962bafa..0f22bf96cf 100644
--- a/actionpack/lib/action_controller/metal/instrumentation.rb
+++ b/actionpack/lib/action_controller/metal/instrumentation.rb
@@ -18,7 +18,7 @@ module ActionController
       raw_payload = {
         :controller => self.class.name,
         :action     => self.action_name,
-        :params     => filter_parameters(params),
+        :params     => request.filtered_parameters,
         :formats    => request.formats.map(&:to_sym)
       }
 
diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb
index 082562d921..e2d64fcd53 100644
--- a/actionpack/lib/action_dispatch.rb
+++ b/actionpack/lib/action_dispatch.rb
@@ -63,6 +63,7 @@ module ActionDispatch
     autoload :Headers
     autoload :MimeNegotiation
     autoload :Parameters
+    autoload :ParametersFilter
     autoload :Upload
     autoload :UploadedFile, 'action_dispatch/http/upload'
     autoload :URL
diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb
index 68ba3637bf..40b40ea94e 100644
--- a/actionpack/lib/action_dispatch/http/parameters.rb
+++ b/actionpack/lib/action_dispatch/http/parameters.rb
@@ -30,29 +30,6 @@ module ActionDispatch
         @env["action_dispatch.request.path_parameters"] ||= {}
       end
 
-      def filter_parameters
-        # TODO: Remove dependency on controller
-        if controller = @env['action_controller.instance']
-          controller.send(:filter_parameters, params)
-        else
-          params
-        end
-      end
-
-      def filter_env
-        if controller = @env['action_controller.instance']
-          @env.map do |key, value|
-            if (key =~ /RAW_POST_DATA/i)
-              '[FILTERED]'
-            else
-              controller.send(:filter_parameters, {key => value}).values[0]
-            end
-          end
-        else
-          env
-        end
-      end
-
     private
       # Convert nested Hashs to HashWithIndifferentAccess
       def normalize_parameters(value)
diff --git a/actionpack/lib/action_dispatch/http/parameters_filter.rb b/actionpack/lib/action_dispatch/http/parameters_filter.rb
new file mode 100644
index 0000000000..bec5e7427d
--- /dev/null
+++ b/actionpack/lib/action_dispatch/http/parameters_filter.rb
@@ -0,0 +1,88 @@
+require 'active_support/core_ext/hash/keys'
+
+module ActionDispatch
+  module Http
+    module ParametersFilter
+      INTERNAL_PARAMS = %w(controller action format _method only_path)
+      
+      @@filter_parameters = nil
+      @@filter_parameters_block = nil
+      
+      # Specify sensitive parameters which will be replaced from the request log.
+      # Filters parameters that have any of the arguments as a substring.
+      # Looks in all subhashes of the param hash for keys to filter.
+      # If a block is given, each key and value of the parameter hash and all
+      # subhashes is passed to it, the value or key
+      # can be replaced using String#replace or similar method.
+      #
+      # Examples:
+      #
+      #   ActionDispatch::Http::ParametersFilter.filter_parameters :password
+      #   => replaces the value to all keys matching /password/i with "[FILTERED]"
+      #
+      #   ActionDispatch::Http::ParametersFilter.filter_parameters :foo, "bar"
+      #   => replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
+      #
+      #   ActionDispatch::Http::ParametersFilter.filter_parameters do |k,v|
+      #     v.reverse! if k =~ /secret/i
+      #   end
+      #   => reverses the value to all keys matching /secret/i
+      #
+      #   ActionDispatch::Http::ParametersFilter.filter_parameters(:foo, "bar") do |k,v|
+      #     v.reverse! if k =~ /secret/i
+      #   end
+      #   => reverses the value to all keys matching /secret/i, and
+      #      replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
+      def self.filter_parameters(*filter_words, &block)
+        raise "You must filter at least one word" if filter_words.empty? and !block_given?
+        
+        @@filter_parameters = filter_words.empty? ? nil : Regexp.new(filter_words.join('|'), true) 
+        @@filter_parameters_block = block
+      end
+      
+      # Return a hash of parameters with all sensitive data replaced.
+      def filtered_parameters
+        @filtered_parameters ||= process_parameter_filter(parameters)
+      end
+      alias_method :fitered_params, :filtered_parameters
+      
+      # Return a hash of request.env with all sensitive data replaced.
+      def filtered_env
+        @env.merge(@env) do |key, value|
+          if (key =~ /RAW_POST_DATA/i)
+            '[FILTERED]'
+          else
+            process_parameter_filter({key => value}, false).values[0]
+          end
+        end
+      end
+      
+    protected
+      
+      def process_parameter_filter(original_parameters, validate_block = true)
+        if @@filter_parameters or @@filter_parameters_block
+          filtered_params = {}
+          
+          original_parameters.each do |key, value|
+            if key =~ @@filter_parameters
+              value = '[FILTERED]'
+            elsif value.is_a?(Hash)
+              value = process_parameter_filter(value)
+            elsif value.is_a?(Array)
+              value = value.map { |item| process_parameter_filter({key => item}, validate_block).values[0] }
+            elsif validate_block and @@filter_parameters_block
+              key = key.dup
+              value = value.dup if value.duplicable?
+              value = @@filter_parameters_block.call(key, value) || value
+            end
+
+            filtered_params[key] = value
+          end
+          filtered_params.except!(*INTERNAL_PARAMS)
+        else
+          original_parameters.except(*INTERNAL_PARAMS)
+        end
+      end
+    end
+  end
+end
\ No newline at end of file
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index 187ce7c15d..7c3a228149 100755
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -11,6 +11,7 @@ module ActionDispatch
     include ActionDispatch::Http::Cache::Request
     include ActionDispatch::Http::MimeNegotiation
     include ActionDispatch::Http::Parameters
+    include ActionDispatch::Http::ParametersFilter
     include ActionDispatch::Http::Upload
     include ActionDispatch::Http::URL
 
diff --git a/actionpack/lib/action_dispatch/middleware/notifications.rb b/actionpack/lib/action_dispatch/middleware/notifications.rb
index ce3732b740..65409f57fd 100644
--- a/actionpack/lib/action_dispatch/middleware/notifications.rb
+++ b/actionpack/lib/action_dispatch/middleware/notifications.rb
@@ -10,7 +10,7 @@ module ActionDispatch
 
     def call(env)
       request = Request.new(env)
-      payload = retrieve_payload_from_env(request.filter_env)
+      payload = retrieve_payload_from_env(request.filtered_env)
 
       ActiveSupport::Notifications.instrument("action_dispatch.before_dispatch", payload)
 
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index cb95ecea50..0a6180959e 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -453,6 +453,40 @@ class RequestTest < ActiveSupport::TestCase
     request.expects(:parameters).at_least_once.returns({})
     assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV])
   end
+  
+  test "filter_parameters" do
+    request = stub_request
+    request.stubs(:request_parameters).returns({ "foo" => 1 })
+    request.stubs(:query_parameters).returns({ "bar" => 2 })
+    
+    assert_raises RuntimeError do
+      ActionDispatch::Http::ParametersFilter.filter_parameters
+    end
+    
+    test_hashes = [
+    [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'],
+    [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'],
+    [{'foo'=>'bar', 'bar'=>'foo'},{'foo'=>'[FILTERED]', 'bar'=>'foo'},%w'foo baz'],
+    [{'foo'=>'bar', 'baz'=>'foo'},{'foo'=>'[FILTERED]', 'baz'=>'[FILTERED]'},%w'foo baz'],
+    [{'bar'=>{'foo'=>'bar','bar'=>'foo'}},{'bar'=>{'foo'=>'[FILTERED]','bar'=>'foo'}},%w'fo'],
+    [{'foo'=>{'foo'=>'bar','bar'=>'foo'}},{'foo'=>'[FILTERED]'},%w'f banana'],
+    [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, %w(foo)]]
+
+    test_hashes.each do |before_filter, after_filter, filter_words|
+      ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words)
+      assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter)
+
+      filter_words.push('blah')
+      ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words) do |key, value|
+        value.reverse! if key =~ /bargain/
+      end
+
+      before_filter['barg'] = {'bargain'=>'gain', 'blah'=>'bar', 'bar'=>{'bargain'=>{'blah'=>'foo'}}}
+      after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}}
+
+      assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter)
+    end
+  end
 
 protected