From 1396b05e5a36859a9730e7a4a56abba02c41c0d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 20:02:51 -0400 Subject: Test the correct object --- actionpack/test/controller/parameters/parameters_permit_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 3e067314d6..e5bb553855 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -396,7 +396,7 @@ class ParametersPermitTest < ActiveSupport::TestCase params = ActionController::Parameters.new(crab: "Senjougahara Hitagi") assert params.to_h.is_a? ActiveSupport::HashWithIndifferentAccess - assert_not @params.to_h.is_a? ActionController::Parameters + assert_not params.to_h.is_a? ActionController::Parameters assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_h) ensure ActionController::Parameters.permit_all_parameters = false -- cgit v1.2.3 From fd88ccc905549c61e0e4525fcb68b91d20b9afe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 21:45:39 -0400 Subject: Raise exception when calling to_h in a unfiltered Parameters Before we returned either an empty hash or only the always permitted parameters (:controller and :action by default). The previous behavior was dangerous because in order to get the attributes users usually fallback to use to_unsafe_h that could potentially introduce security issues. The to_unsafe_h API is also not good since Parameters is a object that quacks like a Hash but not in all cases since to_h would return an empty hash and users were forced to check if to_unsafe_h is defined or if the instance is a ActionController::Parameters in order to work with it. This end up coupling a lot of libraries and parts of the application with something that is from the controller layer. --- .../controller/parameters/parameters_permit_test.rb | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index e5bb553855..2616b040d1 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -377,10 +377,10 @@ class ParametersPermitTest < ActiveSupport::TestCase assert_equal "32", @params[:person].permit([ :age ])[:age] end - test "to_h returns empty hash on unpermitted params" do - assert @params.to_h.is_a? ActiveSupport::HashWithIndifferentAccess - assert_not @params.to_h.is_a? ActionController::Parameters - assert @params.to_h.empty? + test "to_h raises UnfilteredParameters on unfiltered params" do + assert_raises(ActionController::UnfilteredParameters) do + @params.to_h + end end test "to_h returns converted hash on permitted params" do @@ -403,17 +403,6 @@ class ParametersPermitTest < ActiveSupport::TestCase end end - test "to_h returns always permitted parameter on unpermitted params" do - params = ActionController::Parameters.new( - controller: "users", - action: "create", - user: { - name: "Sengoku Nadeko" - } - ) - - assert_equal({ "controller" => "users", "action" => "create" }, params.to_h) - end test "to_unsafe_h returns unfiltered params" do assert @params.to_unsafe_h.is_a? ActiveSupport::HashWithIndifferentAccess -- cgit v1.2.3 From 9f4c2632ef28b9622ffa0eca5d02beea8ec809c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 21:56:55 -0400 Subject: Add ActionController::Parameters#to_hash to implict conversion Now methods that implicit convert objects to a hash will be able to work without requiring the users to change their implementation. This method will return a Hash instead of a HashWithIndefirentAccess to mimic the same implementation of HashWithIndefirentAccess#to_hash. --- .../parameters/parameters_permit_test.rb | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 2616b040d1..12555c3136 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -403,6 +403,32 @@ class ParametersPermitTest < ActiveSupport::TestCase end end + test "to_hash raises UnfilteredParameters on unfiltered params" do + assert_raises(ActionController::UnfilteredParameters) do + @params.to_hash + end + end + + test "to_hash returns converted hash on permitted params" do + @params.permit! + + assert_instance_of Hash, @params.to_hash + assert_not_kind_of ActionController::Parameters, @params.to_hash + end + + test "to_hash returns converted hash when .permit_all_parameters is set" do + begin + ActionController::Parameters.permit_all_parameters = true + params = ActionController::Parameters.new(crab: "Senjougahara Hitagi") + + assert_instance_of Hash, params.to_hash + assert_not_kind_of ActionController::Parameters, params.to_hash + assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_hash) + assert_equal({ "crab" => "Senjougahara Hitagi" }, params) + ensure + ActionController::Parameters.permit_all_parameters = false + end + end test "to_unsafe_h returns unfiltered params" do assert @params.to_unsafe_h.is_a? ActiveSupport::HashWithIndifferentAccess -- cgit v1.2.3 From e13e72cce4e08484aaa03b1e62fc0c70d0a7e6f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 22:00:14 -0400 Subject: Add test to make sure that to_unsafe_h don't mutate the target --- .../test/controller/parameters/parameters_permit_test.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 12555c3136..6d41d4fc64 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -443,6 +443,16 @@ class ParametersPermitTest < ActiveSupport::TestCase assert_equal expected, params.to_unsafe_h end + test "to_unsafe_h does not mutate the parameters" do + params = ActionController::Parameters.new("f" => { "language_facet" => ["Tibetan"] }) + params[:f] + + params.to_unsafe_h + + assert_not_predicate params, :permitted? + assert_not_predicate params[:f], :permitted? + end + test "to_h only deep dups Ruby collections" do company = Class.new do attr_reader :dupped -- cgit v1.2.3 From af878151dbf93fae647ec682d96c0caaeb9a81f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 22:03:30 -0400 Subject: Use the right assetions to better error messages --- .../test/controller/parameters/parameters_permit_test.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 6d41d4fc64..daef9ad892 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -386,8 +386,8 @@ class ParametersPermitTest < ActiveSupport::TestCase test "to_h returns converted hash on permitted params" do @params.permit! - assert @params.to_h.is_a? ActiveSupport::HashWithIndifferentAccess - assert_not @params.to_h.is_a? ActionController::Parameters + assert_instance_of ActiveSupport::HashWithIndifferentAccess, @params.to_h + assert_not_kind_of ActionController::Parameters, @params.to_h end test "to_h returns converted hash when .permit_all_parameters is set" do @@ -395,8 +395,8 @@ class ParametersPermitTest < ActiveSupport::TestCase ActionController::Parameters.permit_all_parameters = true params = ActionController::Parameters.new(crab: "Senjougahara Hitagi") - assert params.to_h.is_a? ActiveSupport::HashWithIndifferentAccess - assert_not params.to_h.is_a? ActionController::Parameters + assert_instance_of ActiveSupport::HashWithIndifferentAccess, params.to_h + assert_not_kind_of ActionController::Parameters, params.to_h assert_equal({ "crab" => "Senjougahara Hitagi" }, params.to_h) ensure ActionController::Parameters.permit_all_parameters = false @@ -431,15 +431,15 @@ class ParametersPermitTest < ActiveSupport::TestCase end test "to_unsafe_h returns unfiltered params" do - assert @params.to_unsafe_h.is_a? ActiveSupport::HashWithIndifferentAccess - assert_not @params.to_unsafe_h.is_a? ActionController::Parameters + assert_instance_of ActiveSupport::HashWithIndifferentAccess, @params.to_unsafe_h + assert_not_kind_of ActionController::Parameters, @params.to_unsafe_h end test "to_unsafe_h returns unfiltered params even after accessing few keys" do params = ActionController::Parameters.new("f" => { "language_facet" => ["Tibetan"] }) expected = { "f" => { "language_facet" => ["Tibetan"] } } - assert params["f"].is_a? ActionController::Parameters + assert_instance_of ActionController::Parameters, params["f"] assert_equal expected, params.to_unsafe_h end -- cgit v1.2.3 From 29333ddb69e69d0fa99a66bf5fab333e8c5611aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 22:21:14 -0400 Subject: Implement ActionController::Parameters#to_query and #to_param Previously it was raising an error because it may be unsafe to use those methods in a unpermitted parameter. Now we delegate to to_h that already raise an error when the Parameters instance is not permitted. This also fix a bug when using `#to_query` in a hash that contains a `ActionController::Parameters` instance and was returning the name of the class in the string. --- actionpack/test/controller/required_params_test.rb | 24 +++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/required_params_test.rb b/actionpack/test/controller/required_params_test.rb index dd07c2486b..46bb374b3f 100644 --- a/actionpack/test/controller/required_params_test.rb +++ b/actionpack/test/controller/required_params_test.rb @@ -72,9 +72,27 @@ class ParametersRequireTest < ActiveSupport::TestCase assert params.value?("cinco") end - test "to_query is not supported" do - assert_raises(NoMethodError) do - ActionController::Parameters.new(foo: "bar").to_param + test "to_param works like in a Hash" do + params = ActionController::Parameters.new(nested: { key: "value" }).permit! + assert_equal({ nested: { key: "value" } }.to_param, params.to_param) + + params = { root: ActionController::Parameters.new(nested: { key: "value" }).permit! } + assert_equal({ root: { nested: { key: "value" } } }.to_param, params.to_param) + + assert_raise(ActionController::UnfilteredParameters) do + ActionController::Parameters.new(nested: { key: "value" }).to_param + end + end + + test "to_query works like in a Hash" do + params = ActionController::Parameters.new(nested: { key: "value" }).permit! + assert_equal({ nested: { key: "value" } }.to_query, params.to_query) + + params = { root: ActionController::Parameters.new(nested: { key: "value" }).permit! } + assert_equal({ root: { nested: { key: "value" } } }.to_query, params.to_query) + + assert_raise(ActionController::UnfilteredParameters) do + ActionController::Parameters.new(nested: { key: "value" }).to_query end end end -- cgit v1.2.3 From 93034ad7fea7e00562103a7cd0acfab19bbfadf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 17 Apr 2017 18:55:21 -0400 Subject: Reuse the Parameters#to_h check in the routing helpers Since this protection is now in Parameters we can use it instead of reimplementing again. --- actionpack/test/controller/redirect_test.rb | 4 ++-- actionpack/test/controller/url_for_test.rb | 2 +- actionpack/test/dispatch/routing/custom_url_helpers_test.rb | 8 ++++---- actionpack/test/dispatch/routing_test.rb | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) (limited to 'actionpack/test') diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb index f06a1f4d23..5b16af78c4 100644 --- a/actionpack/test/controller/redirect_test.rb +++ b/actionpack/test/controller/redirect_test.rb @@ -285,10 +285,10 @@ class RedirectTest < ActionController::TestCase end def test_redirect_to_params - error = assert_raise(ArgumentError) do + error = assert_raise(ActionController::UnfilteredParameters) do get :redirect_to_params end - assert_equal ActionDispatch::Routing::INSECURE_URL_PARAMETERS_MESSAGE, error.message + assert_equal "unable to convert unpermitted parameters to hash", error.message end def test_redirect_to_with_block diff --git a/actionpack/test/controller/url_for_test.rb b/actionpack/test/controller/url_for_test.rb index 862dcf01c3..2afe67ed91 100644 --- a/actionpack/test/controller/url_for_test.rb +++ b/actionpack/test/controller/url_for_test.rb @@ -386,7 +386,7 @@ module AbstractController def test_url_action_controller_parameters add_host! - assert_raise(ArgumentError) do + assert_raise(ActionController::UnfilteredParameters) do W.new.url_for(ActionController::Parameters.new(controller: "c", action: "a", protocol: "javascript", f: "%0Aeval(name)")) end end diff --git a/actionpack/test/dispatch/routing/custom_url_helpers_test.rb b/actionpack/test/dispatch/routing/custom_url_helpers_test.rb index cb5ca5888b..338992dda5 100644 --- a/actionpack/test/dispatch/routing/custom_url_helpers_test.rb +++ b/actionpack/test/dispatch/routing/custom_url_helpers_test.rb @@ -165,8 +165,8 @@ class TestCustomUrlHelpers < ActionDispatch::IntegrationTest assert_equal "/", params_path(@safe_params) assert_equal "/", Routes.url_helpers.params_path(@safe_params) - assert_raises(ArgumentError) { params_path(@unsafe_params) } - assert_raises(ArgumentError) { Routes.url_helpers.params_path(@unsafe_params) } + assert_raises(ActionController::UnfilteredParameters) { params_path(@unsafe_params) } + assert_raises(ActionController::UnfilteredParameters) { Routes.url_helpers.params_path(@unsafe_params) } assert_equal "/basket", symbol_path assert_equal "/basket", Routes.url_helpers.symbol_path @@ -208,8 +208,8 @@ class TestCustomUrlHelpers < ActionDispatch::IntegrationTest assert_equal "http://www.example.com/", params_url(@safe_params) assert_equal "http://www.example.com/", Routes.url_helpers.params_url(@safe_params) - assert_raises(ArgumentError) { params_url(@unsafe_params) } - assert_raises(ArgumentError) { Routes.url_helpers.params_url(@unsafe_params) } + assert_raises(ActionController::UnfilteredParameters) { params_url(@unsafe_params) } + assert_raises(ActionController::UnfilteredParameters) { Routes.url_helpers.params_url(@unsafe_params) } assert_equal "http://www.example.com/basket", symbol_url assert_equal "http://www.example.com/basket", Routes.url_helpers.symbol_url diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index fdc47743fa..d64917e0d3 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3633,7 +3633,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest end params = ActionController::Parameters.new(id: "1") - assert_raises ArgumentError do + assert_raises ActionController::UnfilteredParameters do root_path(params) end end -- cgit v1.2.3