diff options
| -rw-r--r-- | actionpack/lib/action_controller/metal/strong_parameters.rb | 115 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing.rb | 9 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/route_set.rb | 6 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/url_for.rb | 13 | ||||
| -rw-r--r-- | actionpack/test/controller/parameters/parameters_permit_test.rb | 65 | ||||
| -rw-r--r-- | actionpack/test/controller/redirect_test.rb | 4 | ||||
| -rw-r--r-- | actionpack/test/controller/required_params_test.rb | 24 | ||||
| -rw-r--r-- | actionpack/test/controller/url_for_test.rb | 2 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing/custom_url_helpers_test.rb | 8 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing_test.rb | 2 | ||||
| -rw-r--r-- | actionview/lib/action_view/helpers/url_helper.rb | 5 | ||||
| -rw-r--r-- | actionview/test/template/url_helper_test.rb | 6 |
12 files changed, 168 insertions, 91 deletions
diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index baf15570d5..7864f9decd 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -43,6 +43,18 @@ module ActionController end end + # Raised when a Parameters instance is not marked as permitted and + # an operation to transform it to hash is called. + # + # params = ActionController::Parameters.new(a: "123", b: "456") + # params.to_h + # # => ActionController::UnfilteredParameters: unable to convert unpermitted parameters to hash + class UnfilteredParameters < ArgumentError + def initialize # :nodoc: + super("unable to convert unpermitted parameters to hash") + end + end + # == Action Controller \Parameters # # Allows you to choose which attributes should be whitelisted for mass updating @@ -53,9 +65,9 @@ module ActionController # # params = ActionController::Parameters.new({ # person: { - # name: 'Francesco', + # name: "Francesco", # age: 22, - # role: 'admin' + # role: "admin" # } # }) # @@ -103,7 +115,7 @@ module ActionController # You can fetch values of <tt>ActionController::Parameters</tt> using either # <tt>:key</tt> or <tt>"key"</tt>. # - # params = ActionController::Parameters.new(key: 'value') + # params = ActionController::Parameters.new(key: "value") # params[:key] # => "value" # params["key"] # => "value" class Parameters @@ -203,13 +215,13 @@ module ActionController # class Person < ActiveRecord::Base # end # - # params = ActionController::Parameters.new(name: 'Francesco') + # params = ActionController::Parameters.new(name: "Francesco") # params.permitted? # => false # Person.new(params) # => ActiveModel::ForbiddenAttributesError # # ActionController::Parameters.permit_all_parameters = true # - # params = ActionController::Parameters.new(name: 'Francesco') + # params = ActionController::Parameters.new(name: "Francesco") # params.permitted? # => true # Person.new(params) # => #<Person id: nil, name: "Francesco"> def initialize(parameters = {}) @@ -228,13 +240,14 @@ module ActionController end # Returns a safe <tt>ActiveSupport::HashWithIndifferentAccess</tt> - # representation of this parameter with all unpermitted keys removed. + # representation of the parameters with all unpermitted keys removed. # # params = ActionController::Parameters.new({ - # name: 'Senjougahara Hitagi', - # oddity: 'Heavy stone crab' + # name: "Senjougahara Hitagi", + # oddity: "Heavy stone crab" # }) - # params.to_h # => {} + # params.to_h + # # => ActionController::UnfilteredParameters: unable to convert unfiltered parameters to hash # # safe_params = params.permit(:name) # safe_params.to_h # => {"name"=>"Senjougahara Hitagi"} @@ -242,17 +255,61 @@ module ActionController if permitted? convert_parameters_to_hashes(@parameters, :to_h) else - slice(*self.class.always_permitted_parameters).permit!.to_h + raise UnfilteredParameters end end + # Returns a safe <tt>Hash</tt> representation of the parameters + # with all unpermitted keys removed. + # + # params = ActionController::Parameters.new({ + # name: "Senjougahara Hitagi", + # oddity: "Heavy stone crab" + # }) + # params.to_hash + # # => ActionController::UnfilteredParameters: unable to convert unfiltered parameters to hash + # + # safe_params = params.permit(:name) + # safe_params.to_hash # => {"name"=>"Senjougahara Hitagi"} + def to_hash + to_h.to_hash + end + + # Returns a string representation of the receiver suitable for use as a URL + # query string: + # + # params = ActionController::Parameters.new({ + # name: "David", + # nationality: "Danish" + # }) + # params.to_query + # # => "name=David&nationality=Danish" + # + # An optional namespace can be passed to enclose key names: + # + # params = ActionController::Parameters.new({ + # name: "David", + # nationality: "Danish" + # }) + # params.to_query("user") + # # => "user%5Bname%5D=David&user%5Bnationality%5D=Danish" + # + # The string pairs "key=value" that conform the query string + # are sorted lexicographically in ascending order. + # + # This method is also aliased as +to_param+. + def to_query(*args) + to_h.to_query(*args) + end + alias_method :to_param, :to_query + # Returns an unsafe, unfiltered - # <tt>ActiveSupport::HashWithIndifferentAccess</tt> representation of this - # parameter. + # <tt>ActiveSupport::HashWithIndifferentAccess</tt> representation of the + # parameters. # # params = ActionController::Parameters.new({ - # name: 'Senjougahara Hitagi', - # oddity: 'Heavy stone crab' + # name: "Senjougahara Hitagi", + # oddity: "Heavy stone crab" # }) # params.to_unsafe_h # # => {"name"=>"Senjougahara Hitagi", "oddity" => "Heavy stone crab"} @@ -297,7 +354,7 @@ module ActionController # class Person < ActiveRecord::Base # end # - # params = ActionController::Parameters.new(name: 'Francesco') + # params = ActionController::Parameters.new(name: "Francesco") # params.permitted? # => false # Person.new(params) # => ActiveModel::ForbiddenAttributesError # params.permit! @@ -319,7 +376,7 @@ module ActionController # When passed a single key, if it exists and its associated value is # either present or the singleton +false+, returns said value: # - # ActionController::Parameters.new(person: { name: 'Francesco' }).require(:person) + # ActionController::Parameters.new(person: { name: "Francesco" }).require(:person) # # => <ActionController::Parameters {"name"=>"Francesco"} permitted: false> # # Otherwise raises <tt>ActionController::ParameterMissing</tt>: @@ -352,7 +409,7 @@ module ActionController # Technically this method can be used to fetch terminal values: # # # CAREFUL - # params = ActionController::Parameters.new(person: { name: 'Finn' }) + # params = ActionController::Parameters.new(person: { name: "Finn" }) # name = params.require(:person).require(:name) # CAREFUL # # but take into account that at some point those ones have to be permitted: @@ -382,7 +439,7 @@ module ActionController # for the object to +true+. This is useful for limiting which attributes # should be allowed for mass updating. # - # params = ActionController::Parameters.new(user: { name: 'Francesco', age: 22, role: 'admin' }) + # params = ActionController::Parameters.new(user: { name: "Francesco", age: 22, role: "admin" }) # permitted = params.require(:user).permit(:name, :age) # permitted.permitted? # => true # permitted.has_key?(:name) # => true @@ -402,7 +459,7 @@ module ActionController # You may declare that the parameter should be an array of permitted scalars # by mapping it to an empty array: # - # params = ActionController::Parameters.new(tags: ['rails', 'parameters']) + # params = ActionController::Parameters.new(tags: ["rails", "parameters"]) # params.permit(tags: []) # # Sometimes it is not possible or convenient to declare the valid keys of @@ -418,11 +475,11 @@ module ActionController # # params = ActionController::Parameters.new({ # person: { - # name: 'Francesco', + # name: "Francesco", # age: 22, # pets: [{ - # name: 'Purplish', - # category: 'dogs' + # name: "Purplish", + # category: "dogs" # }] # } # }) @@ -441,8 +498,8 @@ module ActionController # params = ActionController::Parameters.new({ # person: { # contact: { - # email: 'none@test.com', - # phone: '555-1234' + # email: "none@test.com", + # phone: "555-1234" # } # } # }) @@ -475,7 +532,7 @@ module ActionController # Returns a parameter for the given +key+. If not found, # returns +nil+. # - # params = ActionController::Parameters.new(person: { name: 'Francesco' }) + # params = ActionController::Parameters.new(person: { name: "Francesco" }) # params[:person] # => <ActionController::Parameters {"name"=>"Francesco"} permitted: false> # params[:none] # => nil def [](key) @@ -494,11 +551,11 @@ module ActionController # if more arguments are given, then that will be returned; if a block # is given, then that will be run and its result returned. # - # params = ActionController::Parameters.new(person: { name: 'Francesco' }) + # params = ActionController::Parameters.new(person: { name: "Francesco" }) # params.fetch(:person) # => <ActionController::Parameters {"name"=>"Francesco"} permitted: false> # params.fetch(:none) # => ActionController::ParameterMissing: param is missing or the value is empty: none - # params.fetch(:none, 'Francesco') # => "Francesco" - # params.fetch(:none) { 'Francesco' } # => "Francesco" + # params.fetch(:none, "Francesco") # => "Francesco" + # params.fetch(:none) { "Francesco" } # => "Francesco" def fetch(key, *args) convert_value_to_parameters( @parameters.fetch(key) { @@ -715,8 +772,6 @@ module ActionController end end - undef_method :to_param - # Returns duplicate of object including all parameters. def deep_dup self.class.new(@parameters.deep_dup).tap do |duplicate| diff --git a/actionpack/lib/action_dispatch/routing.rb b/actionpack/lib/action_dispatch/routing.rb index 60d4789a63..87dd1eba38 100644 --- a/actionpack/lib/action_dispatch/routing.rb +++ b/actionpack/lib/action_dispatch/routing.rb @@ -254,14 +254,5 @@ module ActionDispatch SEPARATORS = %w( / . ? ) #:nodoc: HTTP_METHODS = [:get, :head, :post, :patch, :put, :delete, :options] #:nodoc: - - #:stopdoc: - INSECURE_URL_PARAMETERS_MESSAGE = <<-MSG.squish - Attempting to generate a URL from non-sanitized request parameters! - - An attacker can inject malicious data into the generated URL, such as - changing the host. Whitelist and sanitize passed parameters to be secure. - MSG - #:startdoc: end end diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 129e90037e..e1f9fc9ecc 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -318,11 +318,7 @@ module ActionDispatch when Hash args.pop when ActionController::Parameters - if last.permitted? - args.pop.to_h - else - raise ArgumentError, ActionDispatch::Routing::INSECURE_URL_PARAMETERS_MESSAGE - end + args.pop.to_h end helper.call self, args, options end diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb index 008216cc80..a9bdefa775 100644 --- a/actionpack/lib/action_dispatch/routing/url_for.rb +++ b/actionpack/lib/action_dispatch/routing/url_for.rb @@ -171,17 +171,10 @@ module ActionDispatch case options when nil _routes.url_for(url_options.symbolize_keys) - when Hash + when Hash, ActionController::Parameters route_name = options.delete :use_route - _routes.url_for(options.symbolize_keys.reverse_merge!(url_options), - route_name) - when ActionController::Parameters - unless options.permitted? - raise ArgumentError.new(ActionDispatch::Routing::INSECURE_URL_PARAMETERS_MESSAGE) - end - route_name = options.delete :use_route - _routes.url_for(options.to_h.symbolize_keys. - reverse_merge!(url_options), route_name) + merged_url_options = options.to_h.symbolize_keys.reverse_merge!(url_options) + _routes.url_for(merged_url_options, route_name) when String options when Symbol diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 3e067314d6..daef9ad892 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -377,17 +377,17 @@ 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 @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,39 +395,64 @@ 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 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" - } - ) + 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_equal({ "controller" => "users", "action" => "create" }, params.to_h) + 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 - 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 + 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 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/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 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 diff --git a/actionview/lib/action_view/helpers/url_helper.rb b/actionview/lib/action_view/helpers/url_helper.rb index 304db38060..70fc57c35f 100644 --- a/actionview/lib/action_view/helpers/url_helper.rb +++ b/actionview/lib/action_view/helpers/url_helper.rb @@ -621,11 +621,6 @@ module ActionView # # => [{name: 'country[name]', value: 'Denmark'}] def to_form_params(attribute, namespace = nil) attribute = if attribute.respond_to?(:permitted?) - unless attribute.permitted? - raise ArgumentError, "Attempting to generate a button from non-sanitized request parameters!" \ - " Whitelist and sanitize passed parameters to be secure." - end - attribute.to_h else attribute diff --git a/actionview/test/template/url_helper_test.rb b/actionview/test/template/url_helper_test.rb index 09454b32cc..a6444a1686 100644 --- a/actionview/test/template/url_helper_test.rb +++ b/actionview/test/template/url_helper_test.rb @@ -231,7 +231,11 @@ class UrlHelperTest < ActiveSupport::TestCase end def to_h - { foo: :bar, baz: "quux" } + if permitted? + { foo: :bar, baz: "quux" } + else + raise ArgumentError + end end end |