From 68802d0fbe9d20ef8c5f6626d4b3279bd3a42d3e Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist & Xavier Noria Date: Sat, 1 Jan 2011 23:51:05 +0700 Subject: Filter sensitive query string parameters in the log [#6244 state:committed] This provides more safety to applications that put secret information in the query string, such as API keys or SSO tokens. Signed-off-by: Xavier Noria --- actionpack/CHANGELOG | 2 ++ .../lib/action_dispatch/http/filter_parameters.rb | 21 +++++++++--- actionpack/test/dispatch/request_test.rb | 38 ++++++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG index d9fe45d897..5ab92c8cfc 100644 --- a/actionpack/CHANGELOG +++ b/actionpack/CHANGELOG @@ -1,5 +1,7 @@ *Rails 3.1.0 (unreleased)* +* Sensitive query string parameters (specified in config.filter_parameters) will now be filtered out from the request paths in the log file. [Prem Sichanugrist, fxn] + * URL parameters which return false for to_param now appear in the query string (previously they were removed) [Andrew White] * URL parameters which return nil for to_param are now removed from the query string [Andrew White] diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb index 1ab48ae04d..8dd1af7f3d 100644 --- a/actionpack/lib/action_dispatch/http/filter_parameters.rb +++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb @@ -5,10 +5,10 @@ require 'active_support/core_ext/object/duplicable' 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. + # the request log by looking in the query string of the request and all + # subhashes of the params hash to filter. If a block is given, each key and + # value of the params hash and all subhashes is passed to it, the value + # or key can be replaced using String#replace or similar method. # # Examples: # @@ -38,6 +38,11 @@ module ActionDispatch @filtered_env ||= env_filter.filter(@env) end + # Reconstructed a path with all sensitive GET parameters replaced. + def filtered_path + @filtered_path ||= query_string.empty? ? path : "#{path}?#{filtered_query_string}" + end + protected def parameter_filter @@ -52,6 +57,14 @@ module ActionDispatch @@parameter_filter_for[filters] ||= ParameterFilter.new(filters) end + KV_RE = '[^&;=]+' + PAIR_RE = %r{(#{KV_RE})=(#{KV_RE})} + def filtered_query_string + query_string.gsub(PAIR_RE) do |_| + parameter_filter.filter([[$1, $2]]).first.join("=") + end + end + end end end diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index dd5bf5ec2d..f03ae7f2b3 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -518,6 +518,44 @@ class RequestTest < ActiveSupport::TestCase assert_equal "1", request.params["step"] end + test "filtered_path returns path with filtered query string" do + %w(; &).each do |sep| + request = stub_request('QUERY_STRING' => %w(username=sikachu secret=bd4f21f api_key=b1bc3b3cd352f68d79d7).join(sep), + 'PATH_INFO' => '/authenticate', + 'action_dispatch.parameter_filter' => [:secret, :api_key]) + + path = request.filtered_path + assert_equal %w(/authenticate?username=sikachu secret=[FILTERED] api_key=[FILTERED]).join(sep), path + end + end + + test "filtered_path should not unescape a genuine '[FILTERED]' value" do + request = stub_request('QUERY_STRING' => "secret=bd4f21f&genuine=%5BFILTERED%5D", + 'PATH_INFO' => '/authenticate', + 'action_dispatch.parameter_filter' => [:secret]) + + path = request.filtered_path + assert_equal "/authenticate?secret=[FILTERED]&genuine=%5BFILTERED%5D", path + end + + test "filtered_path should preserve duplication of keys in query string" do + request = stub_request('QUERY_STRING' => "username=sikachu&secret=bd4f21f&username=fxn", + 'PATH_INFO' => '/authenticate', + 'action_dispatch.parameter_filter' => [:secret]) + + path = request.filtered_path + assert_equal "/authenticate?username=sikachu&secret=[FILTERED]&username=fxn", path + end + + test "filtered_path should ignore searchparts" do + request = stub_request('QUERY_STRING' => "secret", + 'PATH_INFO' => '/authenticate', + 'action_dispatch.parameter_filter' => [:secret]) + + path = request.filtered_path + assert_equal "/authenticate?secret", path + end + protected def stub_request(env = {}) -- cgit v1.2.3 From 89c5b9aee7d7db95cec9e5a934c3761872ab107e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 11 Mar 2011 18:06:10 -0800 Subject: do not automatically add format to routes that end in a slash --- actionpack/lib/action_dispatch/routing/mapper.rb | 2 +- .../test/action_dispatch/routing/mapper_test.rb | 51 ++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 actionpack/test/action_dispatch/routing/mapper_test.rb (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 1733c8032a..756a21dcac 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -107,7 +107,7 @@ module ActionDispatch if @options[:format] == false @options.delete(:format) path - elsif path.include?(":format") + elsif path.include?(":format") || path.end_with?('/') path else "#{path}(.:format)" diff --git a/actionpack/test/action_dispatch/routing/mapper_test.rb b/actionpack/test/action_dispatch/routing/mapper_test.rb new file mode 100644 index 0000000000..9966234f1b --- /dev/null +++ b/actionpack/test/action_dispatch/routing/mapper_test.rb @@ -0,0 +1,51 @@ +require 'abstract_unit' + +module ActionDispatch + module Routing + class MapperTest < ActiveSupport::TestCase + class FakeSet + attr_reader :routes + + def initialize + @routes = [] + end + + def resources_path_names + {} + end + + def request_class + ActionDispatch::Request + end + + def add_route(*args) + routes << args + end + + def conditions + routes.map { |x| x[1] } + end + end + + def test_initialize + Mapper.new FakeSet.new + end + + def test_map_slash + fakeset = FakeSet.new + mapper = Mapper.new fakeset + mapper.match '/', :to => 'posts#index', :as => :main + assert_equal '/', fakeset.conditions.first[:path_info] + end + + def test_map_more_slashes + fakeset = FakeSet.new + mapper = Mapper.new fakeset + + # FIXME: is this a desired behavior? + mapper.match '/one/two/', :to => 'posts#index', :as => :main + assert_equal '/one/two(.:format)', fakeset.conditions.first[:path_info] + end + end + end +end -- cgit v1.2.3 From acd4bfb53764cfea0feaa367cfc8d00dbf9a87c3 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 11 Mar 2011 18:16:47 -0800 Subject: Just define methods directly on the class rather than use the module indirection. clever-- --- actionpack/lib/action_dispatch/routing/mapper.rb | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 756a21dcac..f67708722b 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -243,10 +243,6 @@ module ActionDispatch end module Base - def initialize(set) #:nodoc: - @set = set - end - # You can specify what Rails should route "/" to with the root method: # # root :to => 'pages#main' @@ -558,11 +554,6 @@ module ActionDispatch # PUT /admin/posts/1 # DELETE /admin/posts/1 module Scoping - def initialize(*args) #:nodoc: - @scope = {} - super - end - # Scopes a set of routes to the given default options. # # Take the following route definition as an example: @@ -956,11 +947,6 @@ module ActionDispatch alias :nested_scope :path end - def initialize(*args) #:nodoc: - super - @scope[:path_names] = @set.resources_path_names - end - def resources_path_names(options) @scope[:path_names].merge!(options) end @@ -1473,6 +1459,11 @@ module ActionDispatch end end + def initialize(set) #:nodoc: + @set = set + @scope = { :path_names => @set.resources_path_names } + end + include Base include HttpHelpers include Redirection -- cgit v1.2.3