diff options
| -rw-r--r-- | actionpack/lib/action_controller/base.rb | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/railties/subscriber.rb | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/http/filter_parameters.rb | 128 | ||||
| -rw-r--r-- | actionpack/test/controller/base_test.rb | 21 | ||||
| -rw-r--r-- | actionpack/test/controller/subscriber_test.rb | 5 | ||||
| -rw-r--r-- | actionpack/test/dispatch/request_test.rb | 39 | ||||
| -rw-r--r-- | railties/lib/generators/rails/app/templates/config/application.rb | 2 | ||||
| -rw-r--r-- | railties/lib/rails/application.rb | 1 | ||||
| -rw-r--r-- | railties/lib/rails/configuration.rb | 7 | ||||
| -rw-r--r-- | railties/test/application/configuration_test.rb | 4 |
10 files changed, 115 insertions, 104 deletions
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index cf66e99fa5..a66aafd80e 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -74,8 +74,12 @@ module ActionController end # This method has been moved to ActionDispatch::Request.filter_parameters - def self.filter_parameter_logging(*) + def self.filter_parameter_logging(*args, &block) 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) + filter = Rails.application.config.filter_parameters + filter.concat(args) + filter << block if block + filter end def _normalize_options(action=nil, options={}, &blk) diff --git a/actionpack/lib/action_controller/railties/subscriber.rb b/actionpack/lib/action_controller/railties/subscriber.rb index d257d6ac2c..1f0e6bf51a 100644 --- a/actionpack/lib/action_controller/railties/subscriber.rb +++ b/actionpack/lib/action_controller/railties/subscriber.rb @@ -1,10 +1,14 @@ module ActionController module Railties class Subscriber < Rails::Subscriber + INTERNAL_PARAMS = %w(controller action format _method only_path) + def start_processing(event) payload = event.payload + params = payload[:params].except(*INTERNAL_PARAMS) + info " Processing by #{payload[:controller]}##{payload[:action]} as #{payload[:formats].first.to_s.upcase}" - info " Parameters: #{payload[:params].inspect}" unless payload[:params].blank? + info " Parameters: #{params.inspect}" unless params.empty? end def process_action(event) diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb index 0f4afb01d9..1958e1668d 100644 --- a/actionpack/lib/action_dispatch/http/filter_parameters.rb +++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb @@ -1,69 +1,30 @@ +require 'active_support/core_ext/object/blank' require 'active_support/core_ext/hash/keys' module ActionDispatch module Http + # Allows you to specify sensitive parameters which will be replaced from + # the request log by looking 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: + # + # env["action_dispatch.parameter_filter"] = [:password] + # => replaces the value to all keys matching /password/i with "[FILTERED]" + # + # env["action_dispatch.parameter_filter"] = [:foo, "bar"] + # => replaces the value to all keys matching /foo|bar/i with "[FILTERED]" + # + # env["action_dispatch.parameter_filter"] = lambda do |k,v| + # v.reverse! if k =~ /secret/i + # end + # => reverses the value to all keys matching /secret/i + # 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) @@ -71,7 +32,6 @@ module ActionDispatch 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| @@ -86,8 +46,52 @@ module ActionDispatch protected - def process_parameter_filter(original_parameters) - original_parameters.except(*INTERNAL_PARAMS) + def compile_parameter_filter #:nodoc: + strings, regexps, blocks = [], [], [] + + Array(@env["action_dispatch.parameter_filter"]).each do |item| + case item + when NilClass + when Proc + blocks << item + when Regexp + regexps << item + else + strings << item.to_s + end + end + + regexps << Regexp.new(strings.join('|'), true) unless strings.empty? + [regexps, blocks] + end + + def filtering_parameters? #:nodoc: + @env["action_dispatch.parameter_filter"].present? + end + + def process_parameter_filter(original_params) #:nodoc: + return original_params.dup unless filtering_parameters? + + filtered_params = {} + regexps, blocks = compile_parameter_filter + + original_params.each do |key, value| + if regexps.find { |r| key =~ r } + 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 blocks.present? + key = key.dup + value = value.dup if value.duplicable? + blocks.each { |b| b.call(key, value) } + end + + filtered_params[key] = value + end + + filtered_params end end end diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb index 1510a6a7e0..4fcfbacf4e 100644 --- a/actionpack/test/controller/base_test.rb +++ b/actionpack/test/controller/base_test.rb @@ -2,6 +2,9 @@ require 'abstract_unit' require 'logger' require 'pp' # require 'pp' early to prevent hidden_methods from not picking up the pretty-print methods until too late +module Rails +end + # Provide some controller to run the tests on. module Submodule class ContainedEmptyController < ActionController::Base @@ -63,7 +66,7 @@ class DefaultUrlOptionsController < ActionController::Base end end -class ControllerClassTests < Test::Unit::TestCase +class ControllerClassTests < ActiveSupport::TestCase def test_controller_path assert_equal 'empty', EmptyController.controller_path assert_equal EmptyController.controller_path, EmptyController.new.controller_path @@ -74,7 +77,21 @@ class ControllerClassTests < Test::Unit::TestCase def test_controller_name assert_equal 'empty', EmptyController.controller_name assert_equal 'contained_empty', Submodule::ContainedEmptyController.controller_name - end + end + + def test_filter_parameter_logging + parameters = [] + config = mock(:config => mock(:filter_parameters => parameters)) + Rails.expects(:application).returns(config) + + assert_deprecated do + Class.new(ActionController::Base) do + filter_parameter_logging :password + end + end + + assert_equal [:password], parameters + end end class ControllerInstanceTests < Test::Unit::TestCase diff --git a/actionpack/test/controller/subscriber_test.rb b/actionpack/test/controller/subscriber_test.rb index bd0c83413c..152a0d0c04 100644 --- a/actionpack/test/controller/subscriber_test.rb +++ b/actionpack/test/controller/subscriber_test.rb @@ -97,8 +97,7 @@ class ACSubscriberTest < ActionController::TestCase end def test_process_action_with_filter_parameters - ActionDispatch::Request.class_eval "alias :safe_process_parameter_filter :process_parameter_filter" - ActionDispatch::Request.filter_parameters(:lifo, :amount) + @request.env["action_dispatch.parameter_filter"] = [:lifo, :amount] get :show, :lifo => 'Pratik', :amount => '420', :step => '1' wait @@ -107,8 +106,6 @@ class ACSubscriberTest < ActionController::TestCase 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 c5a75f5d21..2b5c19361a 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -454,18 +454,7 @@ class RequestTest < ActiveSupport::TestCase assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV]) end - class FilterRequest < ActionDispatch::Request - end - - test "filter_parameters raises error without arguments" do - assert_raises RuntimeError do - 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,18 +462,18 @@ class RequestTest < ActiveSupport::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'], - [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, %w(foo)]] + [{'baz'=>[{'foo'=>'baz'}]}, {'baz'=>[{'foo'=>'[FILTERED]'}]}, [/foo/]]] test_hashes.each do |before_filter, after_filter, filter_words| - FilterRequest.filter_parameters(*filter_words) + request = stub_request('action_dispatch.parameter_filter' => filter_words) assert_equal after_filter, request.send(:process_parameter_filter, before_filter) - filter_words.push('blah') - - FilterRequest.filter_parameters(*filter_words) do |key, value| + filter_words << 'blah' + filter_words << lambda { |key, value| value.reverse! if key =~ /bargain/ - end + } + request = stub_request('action_dispatch.parameter_filter' => filter_words) before_filter['barg'] = {'bargain'=>'gain', 'blah'=>'bar', 'bar'=>{'bargain'=>{'blah'=>'foo'}}} after_filter['barg'] = {'bargain'=>'niag', 'blah'=>'[FILTERED]', 'bar'=>{'bargain'=>{'blah'=>'[FILTERED]'}}} @@ -493,10 +482,9 @@ class RequestTest < ActiveSupport::TestCase 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' }) + request = stub_request('action_dispatch.request.parameters' => + { 'lifo' => 'Pratik', 'amount' => '420', 'step' => '1' }, + 'action_dispatch.parameter_filter' => [:lifo, :amount]) params = request.filtered_parameters assert_equal "[FILTERED]", params["lifo"] @@ -505,12 +493,11 @@ class RequestTest < ActiveSupport::TestCase 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 = stub_request('action_dispatch.request.parameters' => + { 'amount' => '420', 'step' => '1' }, "RAW_POST_DATA" => "yada yada", + 'action_dispatch.parameter_filter' => [:lifo, :amount]) - request = FilterRequest.new(request.filtered_env) + request = stub_request(request.filtered_env) assert_equal "[FILTERED]", request.raw_post assert_equal "[FILTERED]", request.params["amount"] diff --git a/railties/lib/generators/rails/app/templates/config/application.rb b/railties/lib/generators/rails/app/templates/config/application.rb index ecebbc6146..78a355d2f4 100644 --- a/railties/lib/generators/rails/app/templates/config/application.rb +++ b/railties/lib/generators/rails/app/templates/config/application.rb @@ -32,6 +32,6 @@ module <%= app_const_base %> # end # Configure sensitive parameters which will be filtered from the log file. - config.filter_parameters :password + config.filter_parameters << :password end end diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 743681359c..5c9892c630 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -110,6 +110,7 @@ module Rails end def call(env) + env["action_dispatch.parameter_filter"] = config.filter_parameters app.call(env) end diff --git a/railties/lib/rails/configuration.rb b/railties/lib/rails/configuration.rb index c7331f79c5..7f1783a6b9 100644 --- a/railties/lib/rails/configuration.rb +++ b/railties/lib/rails/configuration.rb @@ -69,7 +69,7 @@ module Rails class Configuration < Railtie::Configuration attr_accessor :after_initialize_blocks, :cache_classes, :colorize_logging, - :consider_all_requests_local, :dependency_loading, + :consider_all_requests_local, :dependency_loading, :filter_parameters, :load_once_paths, :logger, :metals, :plugins, :preload_frameworks, :reload_plugins, :serve_static_assets, :time_zone, :whiny_nils @@ -83,6 +83,7 @@ module Rails super @load_once_paths = [] @after_initialize_blocks = [] + @filter_parameters = [] @dependency_loading = true @serve_static_assets = true end @@ -252,10 +253,6 @@ module Rails i18n end end - - def filter_parameters(*filter_words, &block) - ActionDispatch::Request.filter_parameters(*filter_words, &block) - end def environment_path "#{root}/config/environments/#{Rails.env}.rb" diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 8c08fe5acc..6968e87986 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -125,9 +125,9 @@ module ApplicationTests test "filter_parameters should be able to set via config.filter_parameters" do add_to_config <<-RUBY - config.filter_parameters :foo, 'bar' do |key, value| + config.filter_parameters += [ :foo, 'bar', lambda { |key, value| value = value.reverse if key =~ /baz/ - end + }] RUBY assert_nothing_raised do |