Tidy up new filter_parameters implementation.

10 files changed, 147 insertions, 120 deletions

diff --git a/actionpack/lib/action_controller.rb b/actionpack/lib/action_controller.rb
index fa4a253ec1..33e107d216 100644
--- a/actionpack/lib/action_controller.rb
+++ b/actionpack/lib/action_controller.rb
@@ -16,7 +16,6 @@ module ActionController
     autoload :ConditionalGet
     autoload :Configuration
     autoload :Cookies
-    autoload :FilterParameterLogging
     autoload :Flash
     autoload :Head
     autoload :Helpers
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index 21a811c004..cf66e99fa5 100644
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -27,7 +27,6 @@ module ActionController
     include ActionController::Compatibility
 
     include ActionController::Cookies
-    include ActionController::FilterParameterLogging
     include ActionController::Flash
     include ActionController::Verification
     include ActionController::RequestForgeryProtection
@@ -74,6 +73,11 @@ module ActionController
       @subclasses ||= []
     end
 
+    # This method has been moved to ActionDispatch::Request.filter_parameters
+    def self.filter_parameter_logging(*)
+      ActiveSupport::Deprecation.warn("Setting filter_parameter_logging in ActionController is deprecated and has no longer effect, please set 'config.filter_parameters' in config/application.rb instead", caller)
+    end
+
     def _normalize_options(action=nil, options={}, &blk)
       case action
       when NilClass
diff --git a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb b/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
deleted file mode 100644
index b59f6df244..0000000000
--- a/actionpack/lib/action_controller/metal/filter_parameter_logging.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-module ActionController
-  module FilterParameterLogging
-    extend ActiveSupport::Concern
-
-    module ClassMethods
-      # This method has been moved to ActionDispatch::Http::ParametersFilter.filter_parameters
-      def filter_parameter_logging(*filter_words, &block)
-        ActiveSupport::Deprecation.warn("Setting filter_parameter_logging in ActionController is deprecated, please set 'config.filter_parameters' in application.rb or environments/[environment_name].rb instead.", caller)
-        ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words, &block)
-      end
-    end
-  end
-end
diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb
index 0f22bf96cf..7f9a7c068b 100644
--- a/actionpack/lib/action_controller/metal/instrumentation.rb
+++ b/actionpack/lib/action_controller/metal/instrumentation.rb
@@ -10,7 +10,6 @@ module ActionController
     extend ActiveSupport::Concern
 
     include AbstractController::Logger
-    include ActionController::FilterParameterLogging
 
     attr_internal :view_runtime
 
diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb
index e2d64fcd53..42ff9ca2e4 100644
--- a/actionpack/lib/action_dispatch.rb
+++ b/actionpack/lib/action_dispatch.rb
@@ -63,7 +63,7 @@ module ActionDispatch
     autoload :Headers
     autoload :MimeNegotiation
     autoload :Parameters
-    autoload :ParametersFilter
+    autoload :FilterParameters
     autoload :Upload
     autoload :UploadedFile, 'action_dispatch/http/upload'
     autoload :URL
diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb
new file mode 100644
index 0000000000..0f4afb01d9
--- /dev/null
+++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb
@@ -0,0 +1,94 @@
+require 'active_support/core_ext/hash/keys'
+
+module ActionDispatch
+  module Http
+    module FilterParameters
+      extend ActiveSupport::Concern
+
+      INTERNAL_PARAMS = %w(controller action format _method only_path)
+
+      module ClassMethods
+        # 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::Request.filter_parameters :password
+        #   => replaces the value to all keys matching /password/i with "[FILTERED]"
+        #
+        #   ActionDispatch::Request.filter_parameters :foo, "bar"
+        #   => replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
+        #
+        #   ActionDispatch::Request.filter_parameters do |k,v|
+        #     v.reverse! if k =~ /secret/i
+        #   end
+        #   => reverses the value to all keys matching /secret/i
+        #
+        #   ActionDispatch::Request.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 filter_parameters(*filter_words, &block)
+          raise "You must filter at least one word" if filter_words.empty?
+
+          parameter_filter = Regexp.new(filter_words.join('|'), true)
+
+          define_method(:process_parameter_filter) do |original_params|
+            filtered_params = {}
+
+            original_params.each do |key, value|
+              if key =~ parameter_filter
+                value = '[FILTERED]'
+              elsif value.is_a?(Hash)
+                value = process_parameter_filter(value)
+              elsif value.is_a?(Array)
+                value = value.map { |i| process_parameter_filter(i) }
+              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 :process_parameter_filter
+        end
+      end
+
+      # Return a hash of parameters with all sensitive data replaced.
+      def filtered_parameters
+        @filtered_parameters ||= process_parameter_filter(parameters)
+      end
+      alias :fitered_params :filtered_parameters
+
+      # Return a hash of request.env with all sensitive data replaced.
+      # TODO Josh should white list env to remove stuff like rack.input and rack.errors
+      def filtered_env
+        filtered_env = @env.dup
+        filtered_env.each do |key, value|
+          if (key =~ /RAW_POST_DATA/i)
+            filtered_env[key] = '[FILTERED]'
+          elsif value.is_a?(Hash)
+            filtered_env[key] = process_parameter_filter(value)
+          end
+        end
+        filtered_env
+      end
+
+    protected
+
+      def process_parameter_filter(original_parameters)
+        original_parameters.except(*INTERNAL_PARAMS)
+      end
+    end
+  end
+end
\ No newline at end of file
diff --git a/actionpack/lib/action_dispatch/http/parameters_filter.rb b/actionpack/lib/action_dispatch/http/parameters_filter.rb
deleted file mode 100644
index bec5e7427d..0000000000
--- a/actionpack/lib/action_dispatch/http/parameters_filter.rb
+++ /dev/null
@@ -1,88 +0,0 @@
-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 7c3a228149..7a17023ed2 100755
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -11,7 +11,7 @@ module ActionDispatch
     include ActionDispatch::Http::Cache::Request
     include ActionDispatch::Http::MimeNegotiation
     include ActionDispatch::Http::Parameters
-    include ActionDispatch::Http::ParametersFilter
+    include ActionDispatch::Http::FilterParameters
     include ActionDispatch::Http::Upload
     include ActionDispatch::Http::URL
 
diff --git a/actionpack/test/controller/subscriber_test.rb b/actionpack/test/controller/subscriber_test.rb
index 950eecaf6f..52434426f7 100644
--- a/actionpack/test/controller/subscriber_test.rb
+++ b/actionpack/test/controller/subscriber_test.rb
@@ -99,7 +99,8 @@ module ActionControllerSubscriberTest
   end
 
   def test_process_action_with_filter_parameters
-    Another::SubscribersController.filter_parameter_logging(:lifo, :amount)
+    ActionDispatch::Request.class_eval "alias :safe_process_parameter_filter :process_parameter_filter"
+    ActionDispatch::Request.filter_parameters(:lifo, :amount)
 
     get :show, :lifo => 'Pratik', :amount => '420', :step => '1'
     wait
@@ -108,6 +109,8 @@ module ActionControllerSubscriberTest
     assert_match /"amount"=>"\[FILTERED\]"/, params
     assert_match /"lifo"=>"\[FILTERED\]"/, params
     assert_match /"step"=>"1"/, params
+  ensure
+    ActionDispatch::Request.class_eval "alias :process_parameter_filter :safe_process_parameter_filter"
   end
 
   def test_redirect_to
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index 0a6180959e..c5a75f5d21 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -453,16 +453,19 @@ 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 })
-    
+
+  class FilterRequest < ActionDispatch::Request
+  end
+
+  test "filter_parameters raises error without arguments" do
     assert_raises RuntimeError do
-      ActionDispatch::Http::ParametersFilter.filter_parameters
+      FilterRequest.filter_parameters
     end
-    
+  end
+  
+  test "process parameter filter" do
+    request = FilterRequest.new({})
+
     test_hashes = [
     [{'foo'=>'bar'},{'foo'=>'bar'},%w'food'],
     [{'foo'=>'bar'},{'foo'=>'[FILTERED]'},%w'foo'],
@@ -473,21 +476,47 @@ class RequestTest < ActiveSupport::TestCase
     [{'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)
+      FilterRequest.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|
+
+      FilterRequest.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]'}}}
+      after_filter['barg']  = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}}
 
-      assert_equal after_filter, request.__send__(:process_parameter_filter, before_filter)
+      assert_equal after_filter, request.send(:process_parameter_filter, before_filter)
     end
   end
 
+  test "filtered_parameters returns params filtered" do
+    FilterRequest.filter_parameters(:lifo, :amount)
+
+    request = FilterRequest.new('action_dispatch.request.parameters' =>
+      { 'lifo' => 'Pratik', 'amount' => '420', 'step' => '1' })
+
+    params = request.filtered_parameters
+    assert_equal "[FILTERED]", params["lifo"]
+    assert_equal "[FILTERED]", params["amount"]
+    assert_equal "1", params["step"]
+  end
+
+  test "filtered_env filters env as a whole" do
+    FilterRequest.filter_parameters(:lifo, :amount)
+
+    request = FilterRequest.new('action_dispatch.request.parameters' =>
+      { 'amount' => '420', 'step' => '1' }, "RAW_POST_DATA" => "yada yada")
+
+    request = FilterRequest.new(request.filtered_env)
+
+    assert_equal "[FILTERED]", request.raw_post
+    assert_equal "[FILTERED]", request.params["amount"]
+    assert_equal "1", request.params["step"]    
+  end
+
 protected
 
   def stub_request(env={})