Merge branch 'master' into nested_has_many_through

5 files changed, 114 insertions, 19 deletions

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/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 1733c8032a..f67708722b 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)"
@@ -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
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
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 = {})