From 31fddf2ace29518399f15f718ff408737e0031a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Thu, 21 Jan 2010 11:39:57 +0100 Subject: [PATCH] Tidy up new filter_parameters implementation. --- actionpack/lib/action_controller.rb | 1 - actionpack/lib/action_controller/base.rb | 6 +- .../metal/filter_parameter_logging.rb | 13 --- .../metal/instrumentation.rb | 1 - actionpack/lib/action_dispatch.rb | 2 +- .../action_dispatch/http/filter_parameters.rb | 94 +++++++++++++++++++ .../action_dispatch/http/parameters_filter.rb | 88 ----------------- .../lib/action_dispatch/http/request.rb | 2 +- actionpack/test/controller/subscriber_test.rb | 5 +- actionpack/test/dispatch/request_test.rb | 55 ++++++++--- .../app/controllers/application_controller.rb | 1 - .../rails/app/templates/config/application.rb | 5 +- railties/lib/rails/configuration.rb | 2 +- 13 files changed, 150 insertions(+), 125 deletions(-) delete mode 100644 actionpack/lib/action_controller/metal/filter_parameter_logging.rb create mode 100644 actionpack/lib/action_dispatch/http/filter_parameters.rb delete mode 100644 actionpack/lib/action_dispatch/http/parameters_filter.rb 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 @@ class Base < Metal include ActionController::Compatibility include ActionController::Cookies - include ActionController::FilterParameterLogging include ActionController::Flash include ActionController::Verification include ActionController::RequestForgeryProtection @@ -74,6 +73,11 @@ def self.subclasses @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 Instrumentation 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 Http 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 @@ class Request < Rack::Request 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 @@ def test_process_action_with_view_runtime 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 @@ def test_process_action_with_filter_parameters 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 @@ def teardown 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 @@ def teardown [{'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={}) diff --git a/railties/lib/generators/rails/app/templates/app/controllers/application_controller.rb b/railties/lib/generators/rails/app/templates/app/controllers/application_controller.rb index 2cdf4eae54..e8065d9505 100644 --- a/railties/lib/generators/rails/app/templates/app/controllers/application_controller.rb +++ b/railties/lib/generators/rails/app/templates/app/controllers/application_controller.rb @@ -1,4 +1,3 @@ class ApplicationController < ActionController::Base protect_from_forgery - filter_parameter_logging :password end diff --git a/railties/lib/generators/rails/app/templates/config/application.rb b/railties/lib/generators/rails/app/templates/config/application.rb index 8a7f024a4d..ecebbc6146 100644 --- a/railties/lib/generators/rails/app/templates/config/application.rb +++ b/railties/lib/generators/rails/app/templates/config/application.rb @@ -30,9 +30,8 @@ class Application < Rails::Application # g.template_engine :erb # g.test_framework :test_unit, :fixture => true # end - + # Configure sensitive parameters which will be filtered from the log file. - # Check the documentation for ActionDispatch::Http::ParametersFilter for more information. - # config.filter_parameters :password + config.filter_parameters :password end end diff --git a/railties/lib/rails/configuration.rb b/railties/lib/rails/configuration.rb index ae4f4007e7..a9586b6e9e 100644 --- a/railties/lib/rails/configuration.rb +++ b/railties/lib/rails/configuration.rb @@ -254,7 +254,7 @@ def i18n end def filter_parameters(*filter_words, &block) - ActionDispatch::Http::ParametersFilter.filter_parameters(*filter_words, &block) + ActionDispatch::Request.filter_parameters(*filter_words, &block) end def environment_path