Protect #filter_parameters created by filter_parameter_logging [José Valim] [#196 state:resolved]

2 files changed, 39 insertions, 32 deletions

diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index e1bf005f39..ea55fe42ce 100755
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -259,12 +259,12 @@ module ActionController #:nodoc:
     DEFAULT_RENDER_STATUS_CODE = "200 OK"
 
     include StatusCodes
-    
+
     # Controller specific instance variables which will not be accessible inside views.
     @@protected_view_variables = %w(@assigns @performed_redirect @performed_render @variables_added @request_origin @url @parent_controller
                                     @action_name @before_filter_chain_aborted @action_cache_path @_session @_cookies @_headers @_params
                                     @_flash @_response)
-    
+
     # Prepends all the URL-generating helpers from AssetHelper. This makes it possible to easily move javascripts, stylesheets,
     # and images to a dedicated asset server away from the main web server. Example:
     #   ActionController::Base.asset_host = "http://assets.example.com"
@@ -325,7 +325,7 @@ module ActionController #:nodoc:
     # Controls the default charset for all renders.
     @@default_charset = "utf-8"
     cattr_accessor :default_charset
-    
+
     # The logger is used for generating information on the action run-time (including benchmarking) if available.
     # Can be set to nil for no logging. Compatible with both Ruby's own Logger and Log4r loggers.
     cattr_accessor :logger
@@ -333,7 +333,7 @@ module ActionController #:nodoc:
     # Controls the resource action separator
     @@resource_action_separator = "/"
     cattr_accessor :resource_action_separator
-    
+
     # Allow to override path names for default resources' actions
     @@resources_path_names = { :new => 'new', :edit => 'edit' }
     cattr_accessor :resources_path_names
@@ -433,7 +433,7 @@ module ActionController #:nodoc:
       end
 
       # Adds a view_path to the front of the view_paths array.
-      # If the current class has no view paths, copy them from 
+      # If the current class has no view paths, copy them from
       # the superclass.  This change will be visible for all future requests.
       #
       #   ArticleController.prepend_view_path("views/default")
@@ -444,9 +444,9 @@ module ActionController #:nodoc:
         view_paths.unshift(*path)
         ActionView::TemplateFinder.process_view_paths(path)
       end
-      
+
       # Adds a view_path to the end of the view_paths array.
-      # If the current class has no view paths, copy them from 
+      # If the current class has no view paths, copy them from
       # the superclass. This change will be visible for all future requests.
       #
       #   ArticleController.append_view_path("views/default")
@@ -457,7 +457,7 @@ module ActionController #:nodoc:
         view_paths.push(*path)
         ActionView::TemplateFinder.process_view_paths(path)
       end
-      
+
       # Replace sensitive parameter data 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.
@@ -504,6 +504,7 @@ module ActionController #:nodoc:
 
           filtered_parameters
         end
+        protected :filter_parameters
       end
 
       # Don't render layouts for templates with the given extensions.
@@ -643,12 +644,12 @@ module ActionController #:nodoc:
       end
 
       self.view_paths = []
-      
+
       # View load paths for controller.
       def view_paths
         @template.finder.view_paths
       end
-    
+
       def view_paths=(value)
         @template.finder.view_paths = value  # Mutex needed
       end
@@ -662,7 +663,7 @@ module ActionController #:nodoc:
       def prepend_view_path(path)
         @template.finder.prepend_view_path(path)  # Mutex needed
       end
-      
+
       # Adds a view_path to the end of the view_paths array.
       # This change affects the current request only.
       #
@@ -874,10 +875,10 @@ module ActionController #:nodoc:
           elsif action_name = options[:action]
             template = default_template_name(action_name.to_s)
             if options[:layout] && !template_exempt_from_layout?(template)
-              render_with_a_layout(:file => template, :status => options[:status], :use_full_path => true, :layout => true)              
+              render_with_a_layout(:file => template, :status => options[:status], :use_full_path => true, :layout => true)
             else
               render_with_no_layout(:file => template, :status => options[:status], :use_full_path => true)
-            end            
+            end
 
           elsif xml = options[:xml]
             response.content_type ||= Mime::XML
@@ -895,12 +896,12 @@ module ActionController #:nodoc:
 
             if collection = options[:collection]
               render_for_text(
-                @template.send!(:render_partial_collection, partial, collection, 
+                @template.send!(:render_partial_collection, partial, collection,
                 options[:spacer_template], options[:locals]), options[:status]
               )
             else
               render_for_text(
-                @template.send!(:render_partial, partial, 
+                @template.send!(:render_partial, partial,
                 ActionView::Base::ObjectWrapper.new(options[:object]), options[:locals]), options[:status]
               )
             end
@@ -1024,7 +1025,7 @@ module ActionController #:nodoc:
       #   redirect_to articles_url
       #   redirect_to :back
       #
-      # The redirection happens as a "302 Moved" header unless otherwise specified. 
+      # The redirection happens as a "302 Moved" header unless otherwise specified.
       #
       # Examples:
       #   redirect_to post_url(@post), :status=>:found
@@ -1035,17 +1036,17 @@ module ActionController #:nodoc:
       # When using <tt>redirect_to :back</tt>, if there is no referrer,
       # RedirectBackError will be raised. You may specify some fallback
       # behavior for this case by rescuing RedirectBackError.
-      def redirect_to(options = {}, response_status = {}) #:doc: 
+      def redirect_to(options = {}, response_status = {}) #:doc:
         raise ActionControllerError.new("Cannot redirect to nil!") if options.nil?
 
-        if options.is_a?(Hash) && options[:status] 
-          status = options.delete(:status) 
-        elsif response_status[:status] 
-          status = response_status[:status] 
-        else 
-          status = 302 
+        if options.is_a?(Hash) && options[:status]
+          status = options.delete(:status)
+        elsif response_status[:status]
+          status = response_status[:status]
+        else
+          status = 302
         end
-        
+
         case options
           when %r{^\w+://.*}
             raise DoubleRenderError if performed?
@@ -1119,7 +1120,7 @@ module ActionController #:nodoc:
           response.body = text.is_a?(Proc) ? text : text.to_s
         end
       end
-      
+
       def initialize_template_class(response)
         response.template = ActionView::Base.new(self.class.view_paths, {}, self)
         response.template.extend self.class.master_helper_module
diff --git a/actionpack/test/controller/filter_params_test.rb b/actionpack/test/controller/filter_params_test.rb
index 11adacb5e3..c9688b2063 100644
--- a/actionpack/test/controller/filter_params_test.rb
+++ b/actionpack/test/controller/filter_params_test.rb
@@ -7,14 +7,14 @@ class FilterParamTest < Test::Unit::TestCase
   def setup
     @controller = FilterParamController.new
   end
-  
+
   def test_filter_parameters
     assert FilterParamController.respond_to?(:filter_parameter_logging)
     assert !@controller.respond_to?(:filter_parameters)
-    
+
     FilterParamController.filter_parameter_logging
     assert @controller.respond_to?(:filter_parameters)
-    
+
     test_hashes = [[{},{},[]],
     [{'foo'=>nil},{'foo'=>nil},[]],
     [{'foo'=>'bar'},{'foo'=>'bar'},[]],
@@ -24,11 +24,11 @@ class FilterParamTest < Test::Unit::TestCase
     [{'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']]
-    
+
     test_hashes.each do |before_filter, after_filter, filter_words|
       FilterParamController.filter_parameter_logging(*filter_words)
-      assert_equal after_filter, @controller.filter_parameters(before_filter)
-      
+      assert_equal after_filter, @controller.send!(:filter_parameters, before_filter)
+
       filter_words.push('blah')
       FilterParamController.filter_parameter_logging(*filter_words) do |key, value|
         value.reverse! if key =~ /bargain/
@@ -37,7 +37,13 @@ class FilterParamTest < Test::Unit::TestCase
       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, @controller.filter_parameters(before_filter)
+      assert_equal after_filter, @controller.send!(:filter_parameters, before_filter)
     end
   end
+
+  def test_filter_parameters_is_protected
+    FilterParamController.filter_parameter_logging
+    assert !@controller.send!(:action_methods).include?(:filter_parameters)
+    assert (begin @controller.filter_parameters rescue true end)
+  end
 end