Skip to content
Browse files

Protect #filter_parameters created by filter_parameter_logging [José …

…Valim] [#196 state:resolved]
  • Loading branch information...
1 parent 49846f8 commit 8d37bd08eeac6d0e94f76ac6640e288bf64595b2 @josh josh committed
Showing with 39 additions and 32 deletions.
  1. +26 −25 actionpack/lib/action_controller/base.rb
  2. +13 −7 actionpack/test/controller/filter_params_test.rb
View
51 actionpack/lib/action_controller/base.rb
@@ -259,12 +259,12 @@ class Base
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 @@ class Base
# 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 @@ class Base
# 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 @@ def view_paths=(value)
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 @@ def prepend_view_path(path)
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 @@ def append_view_path(path)
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 @@ def filter_parameter_logging(*filter_words, &block)
filtered_parameters
end
+ protected :filter_parameters
end

Why was this done? It’s reasonable for other code to want to call this method, as does ExceptionNotification.

Ah, ok, read bug. Worked around by making the method public again in my base controller. ExceptionNotifiable needs to be rewritten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
# Don't render layouts for templates with the given extensions.
@@ -643,12 +644,12 @@ def session_enabled?
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 @@ def view_paths=(value)
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 @@ def render(options = nil, extra_options = {}, &block) #:doc:
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 @@ def render(options = nil, extra_options = {}, &block) #:doc:
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 @@ def default_url_options(options = nil)
# 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 @@ def default_url_options(options = nil)
# 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 @@ def render_for_text(text = nil, status = nil, append_response = false) #: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
View
20 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 @@ def test_filter_parameters
[{'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 @@ def test_filter_parameters
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

0 comments on commit 8d37bd0

Please sign in to comment.
Something went wrong with that request. Please try again.