diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index caceba58f06c..a24a2b37d717 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -675,7 +675,7 @@ def require(key) # params.permit(person: { '0': [:email], '1': [:phone]}).to_h # # => {"person"=>{"0"=>{"email"=>"none@test.com"}, "1"=>{"phone"=>"555-6789"}}} def permit(*filters) - permit_only(*filters, &method(:unpermitted_parameters)) + permit_filters(filters, &method(:unpermitted_parameters)) end # Same as permit, but raises on unpermitted parameters by default. @@ -684,12 +684,7 @@ def permit(*filters) # If a block is given, it is called with the unpermitted keys. def permit_only(*filters, &block) block ||= method(:raise_unpermitted_parameters) - params = permit_filters(filters, &block) - - unpermitted = unpermitted_keys(params) - block.call(unpermitted) if unpermitted.any? - - params + permit_filters(filters, &block) end # `expect` is the preferred way to require and permit parameters and work @@ -819,13 +814,15 @@ def permit_only(*filters, &block) # object # => #"pumpkin"} permitted: true> # def expect(*filters) - expect_only filters, &method(:unpermitted_parameters) + expect_only(*filters, &method(:unpermitted_parameters)) end - # Same as expect, but raises on unpermitted parameters, regardless of ActionController::Parameters.action_on_unpermitted_parameters + # Same as expect, but raises on unpermitted parameters, ignoring ActionController::Parameters.action_on_unpermitted_parameters + # + # Accepts a block which will yield the unpermitted keys if any are found. def expect_only(*filters, &block) block ||= method(:raise_unpermitted_parameters) - params = permit_filters(filters, &block) + params = permit_filters(filters, explicit_arrays: true, check_unpermitted: false, &block) keys = filters.flatten.flat_map { |f| f.is_a?(Hash) ? f.keys : f } values = params.require(keys) values.size == 1 ? values.first : values @@ -848,20 +845,22 @@ def expect_only(*filters, &block) # person_params[:age] # => nil # def allow(*filters) - allow_only filters, &method(:unpermitted_parameters) + allow_only(*filters, &method(:unpermitted_parameters)) end - # Same as allow, but raises on unpermitted parameters, regardless of ActionController::Parameters.action_on_unpermitted_parameters + # Same as allow, but raises on unpermitted parameters, ignoring ActionController::Parameters.action_on_unpermitted_parameters + # + # Accepts a block which will yield the unpermitted keys if any are found. def allow_only(*filters, &block) block ||= method(:raise_unpermitted_parameters) - params = permit_filters(filters, &block) + params = permit_filters(filters, explicit_arrays: true, check_unpermitted: false, &block) values = filters.flatten.flat_map do |filter| case filter when Symbol, String params.fetch(filter, nil) when Hash filter.map do |key, value| - params.fetch(key) { value == EMPTY_ARRAY ? [] : self.class.new.permit! } + params.fetch(key) { value == EMPTY_ARRAY || array_filter?(value) ? [] : self.class.new.permit! } end end end @@ -1205,6 +1204,39 @@ def each_nested_attribute hash end + # Filters params given the set of filters. + # + # explicit_arrays: true switches to explicit array param filtering. + # This is a new requirement for permit to distinguish between: + # { comment: [:text] } + # { comments: [[:text]] } + # With explicit_arrays: false, the first filters would allow + # comment to return either a hash or array of hashs. + # With explicit_arrays: true, the first way would only allow a hash + # with key :text while the second way would allow only an array of hashes + # with key :text. + # check_unpermitted: true calls the passed block when unpermitted keys are found. + # It is intentionally not passed to hash_filter, defaulting to true for nested keys. + def permit_filters(filters, explicit_arrays: false, check_unpermitted: true, &block) + params = self.class.new + + filters.flatten.each do |filter| + case filter + when Symbol, String + permitted_scalar_filter(params, filter) + when Hash + hash_filter(params, filter, explicit_arrays:, &block) + end + end + + if check_unpermitted + unpermitted = unpermitted_keys(params) + block.call(unpermitted) if unpermitted.any? + end + + params.permit! + end + private def new_instance_with_inherited_permitted_status(hash) self.class.new(hash, @logging_context).tap do |new_instance| @@ -1294,6 +1326,28 @@ def specify_numeric_keys?(filter) end end + # When an array is expected, you must specify an array explicitly + # using the following format: + # + # params.expect(comments: [[:flavor]]) + # + # Which will match only the following array formats: + # + # { pies: [{ flavor: "rhubarb" }, { flavor: "apple" }] } + # { pies: { "0" => { flavor: "key lime" }, "1" => { flavor: "mince" } } } + # + # Previously, arrays were specified the same way as hashes: + # + # params.expect(pies: [:flavor]) + # + # which would also allow matching with a hash (or vice versa): + # + # { pies: { flavor: "cherry" } } + # + def array_filter?(filter) + filter.is_a?(Array) && filter.size == 1 && filter.first.is_a?(Array) + end + def each_element(object, filter, &block) case object when Array @@ -1360,22 +1414,6 @@ def permitted_scalar?(value) PERMITTED_SCALAR_TYPES.any? { |type| value.is_a?(type) } end - # Filters params given the set of filters. - def permit_filters(filters, &block) - params = self.class.new - - filters.flatten.each do |filter| - case filter - when Symbol, String - permitted_scalar_filter(params, filter) - when Hash - hash_filter(params, filter, &block) - end - end - - params.permit! - end - # Adds existing keys to the params if their values are scalar. # # For example: @@ -1413,7 +1451,7 @@ def non_scalar?(value) EMPTY_ARRAY = [] # :nodoc: EMPTY_HASH = {} # :nodoc: - def hash_filter(params, filter, &block) + def hash_filter(params, filter, explicit_arrays: false, &block) filter = filter.with_indifferent_access # Slicing filters out non-declared keys. @@ -1431,15 +1469,40 @@ def hash_filter(params, filter, &block) if value.is_a?(Parameters) params[key] = permit_any_in_parameters(value) end + elsif array_filter?(filter[key]) + # Declaration { comments: [[:text]] } + params[key] = each_array_element(value, filter[key].first) do |element| + element.permit_filters(Array.wrap(filter[key].first), explicit_arrays:, &block) + end + elsif explicit_arrays + if value.is_a?(Parameters) + # Treat params as a hash even if they could be interpretted as an array. + params[key] = value.permit_filters(Array.wrap(filter[key]), explicit_arrays:, &block) + end elsif non_scalar?(value) - # Declaration { user: :name } or { user: [:name, :age, { address: ... }] }. + # Declaration { user: { address: ... } } or { user: [:name, :age, { address: ... }] }. + # Allow arrays with old syntax { comments: [:text] } params[key] = each_element(value, filter[key]) do |element| - element.permit_only(filter[key], &block) + element.permit_filters(Array.wrap(filter[key]), explicit_arrays:, &block) end end end end + # Called with the params being filtered (object) and a nested array (filter) + def each_array_element(object, filter, &block) + case object + when Array + object.grep(Parameters).filter_map(&block) + when Parameters + if object.nested_attributes? && !specify_numeric_keys?(filter) + object.each_nested_attribute(&block) + else + [] # Array is expected but params are not an array. + end + end + end + def permit_any_in_parameters(params) self.class.new.tap do |sanitized| params.each do |key, value| diff --git a/actionpack/test/controller/parameters/parameters_allow_test.rb b/actionpack/test/controller/parameters/parameters_allow_test.rb index 71e1954c42fd..dfc669a93295 100644 --- a/actionpack/test/controller/parameters/parameters_allow_test.rb +++ b/actionpack/test/controller/parameters/parameters_allow_test.rb @@ -38,19 +38,44 @@ class ParametersAllowTest < ActiveSupport::TestCase end test "keys to arrays: returns permitted params in hash key order" do - name, addresses = @params[:person].allow(name: [:first, :last], addresses: [:city]) + name, addresses = @params[:person].allow(name: [:first, :last], addresses: [[:city, :state]]) assert_equal({ "first" => "David", "last" => "Heinemeier Hansson" }, name.to_h) - assert_equal({ "city" => "Chicago" }, addresses.first.to_h) + assert_equal({ "city" => "Chicago", "state" => "Illinois" }, addresses.first.to_h) end - test "key and hash: returns permitted params" do - params = ActionController::Parameters.new(name: "Martin", age: 40, pies: [{ type: "dessert", flavor: "pumpkin"}]) - name, age, pies = params.allow(:name, :age, pies: [:type, :flavor]) + test "key to array of keys: returns nil when params is an array" do + params = ActionController::Parameters.new(name: "Martin", pies: [{ flavor: "pumpkin" }]) + pies = params.allow(pies: [:flavor]) - assert_equal "Martin", name - assert_equal 40, age - assert_equal({ "type" => "dessert", "flavor" => "pumpkin" }, pies.first.to_h) + assert_equal({}, pies.to_h) + end + + test "key to array of keys: returns nil when params is nested attributes array" do + params = ActionController::Parameters.new(name: "Martin", pies: { "0" => { flavor: "pumpkin" } }) + pies = params.allow(pies: [:flavor]) + + assert_equal({}, pies.to_h) + end + + test "key to nested array: returns permitted array" do + params = ActionController::Parameters.new(name: "Martin", pies: [{ flavor: "pumpkin" }, { flavor: "chicken pot" }]) + pies = params.allow(pies: [[:flavor]]) + + assert_equal({ "flavor" => "pumpkin" }, pies[0].to_h) + assert_equal({ "flavor" => "chicken pot" }, pies[1].to_h) + end + + test "key to nested array: returns array when params is a hash" do + params = ActionController::Parameters.new(name: "Martin", pies: { flavor: "pumpkin" }) + + assert_equal [], params.allow(pies: [[:flavor]]) + end + + test "key to nested array: returns empty array when params empty array" do + params = ActionController::Parameters.new(name: "Martin", pies: []) + + assert_equal [], params.allow(pies: [[:flavor]]) end test "key to mixed array: returns permitted params" do diff --git a/actionpack/test/controller/parameters/parameters_expect_test.rb b/actionpack/test/controller/parameters/parameters_expect_test.rb index 7cf59d8147ad..063a83f97955 100644 --- a/actionpack/test/controller/parameters/parameters_expect_test.rb +++ b/actionpack/test/controller/parameters/parameters_expect_test.rb @@ -38,19 +38,34 @@ class ParametersExpectTest < ActiveSupport::TestCase end test "keys to arrays: returns permitted params in hash key order" do - name, addresses = @params[:person].expect(name: [:first, :last], addresses: [:city]) + name, addresses = @params[:person].expect(name: [:first, :last], addresses: [[:city]]) assert_equal({ "first" => "David", "last" => "Heinemeier Hansson" }, name.to_h) assert_equal({ "city" => "Chicago" }, addresses.first.to_h) end - test "key and hash: returns permitted params" do - params = ActionController::Parameters.new(name: "Martin", age: 40, pies: [{ type: "dessert", flavor: "pumpkin"}]) - name, age, pies = params.expect(:name, :age, pies: [:type, :flavor]) + test "key to array of keys: raises when params is an array" do + params = ActionController::Parameters.new(name: "Martin", pies: [{ flavor: "pumpkin" }]) - assert_equal "Martin", name - assert_equal 40, age - assert_equal({ "type" => "dessert", "flavor" => "pumpkin" }, pies.first.to_h) + assert_raises(ActionController::ParameterMissing) do + params.expect(pies: [:flavor]) + end + end + + test "key to nested array: returns permitted array" do + params = ActionController::Parameters.new(name: "Martin", pies: [{ flavor: "pumpkin" }, { flavor: "chicken pot" }]) + pies = params.expect(pies: [[:flavor]]) + + assert_equal({ "flavor" => "pumpkin" }, pies[0].to_h) + assert_equal({ "flavor" => "chicken pot" }, pies[1].to_h) + end + + test "key to nested array: raises when params is a hash" do + params = ActionController::Parameters.new(name: "Martin", pies: { flavor: "pumpkin" }) + + assert_raises(ActionController::ParameterMissing) do + params.expect(pies: [[:flavor]]) + end end test "key to mixed array: returns permitted params" do