From cbec22ce5783c9530b3e2735a8e13eeeca00e0d2 Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Sun, 20 Jan 2013 17:59:53 +0100 Subject: strong parameters filters permitted scalars --- actionpack/CHANGELOG.md | 17 +++ .../action_controller/metal/strong_parameters.rb | 114 +++++++++++++++---- .../parameters/nested_parameters_test.rb | 56 ++++++--- .../parameters/parameters_permit_test.rb | 125 ++++++++++++++++++++- 4 files changed, 273 insertions(+), 39 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 2b6425caa5..7fc8b85850 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,5 +1,22 @@ ## Rails 4.0.0 (unreleased) ## +* Given + + params.permit(:name) + + `:name` passes if it is a key of `params` whose value is a permitted scalar. + + Similarly, given + + params.permit(tags: []) + + `:tags` passes if it is a key of `params` whose value is an array of + permitted scalars. + + Permitted scalars filtering happens at any level of nesting. + + *Xavier Noria* + * `BestStandardsSupport` no longer duplicates `X-UA-Compatible` values on each request to prevent header size from blowing up. diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 2c96e03f55..02d9c0b4fb 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -184,6 +184,20 @@ module ActionController # permitted.has_key?(:age) # => true # permitted.has_key?(:role) # => false # + # Only permitted scalars pass the filter. For example, given + # + # params.permit(:name) + # + # +:name+ passes it is a key of +params+ whose associated value is of type + # +String+, +Symbol+, +NilClass+, +Numeric+, +TrueClass+, +FalseClass+, + # +Date+, +Time+, +DateTime+, +StringIO+, or +IO+. Otherwise, the key +:name+ + # is filtered out. + # + # You may declare that the parameter should be an array of permitted scalars + # by mapping it to an empty array: + # + # params.permit(:tags => []) + # # You can also use +permit+ on nested parameters, like: # # params = ActionController::Parameters.new({ @@ -230,29 +244,10 @@ module ActionController filters.flatten.each do |filter| case filter - when Symbol, String then - if has_key?(filter) - _value = self[filter] - params[filter] = _value unless Hash === _value - end - keys.grep(/\A#{Regexp.escape(filter)}\(\d+[if]?\)\z/) { |key| params[key] = self[key] } + when Symbol, String + permitted_scalar_filter(params, filter) when Hash then - filter = filter.with_indifferent_access - - self.slice(*filter.keys).each do |key, values| - return unless values - - key = key.to_sym - - params[key] = each_element(values) do |value| - # filters are a Hash, so we expect value to be a Hash too - next if filter.is_a?(Hash) && !value.is_a?(Hash) - - value = self.class.new(value) if !value.respond_to?(:permit) - - value.permit(*Array.wrap(filter[key])) - end - end + hash_filter(params, filter) end end @@ -352,6 +347,81 @@ module ActionController def unpermitted_keys(params) self.keys - params.keys - NEVER_UNPERMITTED_PARAMS end + + # + # --- Filtering ---------------------------------------------------------- + # + + # This is a white list of permitted scalar types that includes the ones + # supported in XML and JSON requests. + # + # This list is in particular used to filter ordinary requests, String goes + # as first element to quickly short-circuit the common case. + # + # If you modify this collection please update the API of +permit+ above. + PERMITTED_SCALAR_TYPES = [ + String, + Symbol, + NilClass, + Numeric, + TrueClass, + FalseClass, + Date, + Time, + # DateTimes are Dates, we document the type but avoid the redundant check. + StringIO, + IO, + ] + + def permitted_scalar?(value) + PERMITTED_SCALAR_TYPES.any? {|type| value.is_a?(type)} + end + + def permitted_scalar_filter(params, key) + if has_key?(key) && permitted_scalar?(self[key]) + params[key] = self[key] + end + + keys.grep(/\A#{Regexp.escape(key)}\(\d+[if]?\)\z/).each do |key| + if permitted_scalar?(self[key]) + params[key] = self[key] + end + end + end + + def array_of_permitted_scalars?(value) + if value.is_a?(Array) + value.all? {|element| permitted_scalar?(element)} + end + end + + def array_of_permitted_scalars_filter(params, key) + if has_key?(key) && array_of_permitted_scalars?(self[key]) + params[key] = self[key] + end + end + + def hash_filter(params, filter) + filter = filter.with_indifferent_access + + # Slicing filters out non-declared keys. + slice(*filter.keys).each do |key, value| + return unless value + + if filter[key] == [] + # Declaration {:comment_ids => []}. + array_of_permitted_scalars_filter(params, key) + else + # Declaration {:user => :name} or {:user => [:name, :age, {:adress => ...}]}. + params[key] = each_element(value) do |element| + if element.is_a?(Hash) + element = self.class.new(element) unless element.respond_to?(:permit) + element.permit(*Array.wrap(filter[key])) + end + end + end + end + end end # == Strong \Parameters diff --git a/actionpack/test/controller/parameters/nested_parameters_test.rb b/actionpack/test/controller/parameters/nested_parameters_test.rb index 6df849c4e2..8aec159499 100644 --- a/actionpack/test/controller/parameters/nested_parameters_test.rb +++ b/actionpack/test/controller/parameters/nested_parameters_test.rb @@ -2,6 +2,10 @@ require 'abstract_unit' require 'action_controller/metal/strong_parameters' class NestedParametersTest < ActiveSupport::TestCase + def assert_filtered_out(params, key) + assert !params.has_key?(key), "key #{key.inspect} has not been filtered out" + end + test "permitted nested parameters" do params = ActionController::Parameters.new({ book: { @@ -11,6 +15,8 @@ class NestedParametersTest < ActiveSupport::TestCase born: "1564-04-26" }, { name: "Christopher Marlowe" + }, { + :name => %w(malicious injected names) }], details: { pages: 200, @@ -30,10 +36,12 @@ class NestedParametersTest < ActiveSupport::TestCase assert_equal "William Shakespeare", permitted[:book][:authors][0][:name] assert_equal "Christopher Marlowe", permitted[:book][:authors][1][:name] assert_equal 200, permitted[:book][:details][:pages] - assert_nil permitted[:book][:id] - assert_nil permitted[:book][:details][:genre] - assert_nil permitted[:book][:authors][0][:born] - assert_nil permitted[:magazine] + + assert_filtered_out permitted, :magazine + assert_filtered_out permitted[:book], :id + assert_filtered_out permitted[:book][:details], :genre + assert_filtered_out permitted[:book][:authors][0], :born + assert_filtered_out permitted[:book][:authors][2], :name end test "permitted nested parameters with a string or a symbol as a key" do @@ -68,7 +76,7 @@ class NestedParametersTest < ActiveSupport::TestCase } }) - permitted = params.permit :book => :genres + permitted = params.permit :book => {:genres => []} assert_equal ["Tragedy"], permitted[:book][:genres] end @@ -124,19 +132,41 @@ class NestedParametersTest < ActiveSupport::TestCase test "fields_for-style nested params" do params = ActionController::Parameters.new({ - book: { - authors_attributes: { - :'0' => { name: 'William Shakespeare', age_of_death: '52' }, - :'-1' => { name: 'Unattributed Assistant' } + :book => { + :authors_attributes => { + :'0' => { :name => 'William Shakespeare', :age_of_death => '52' }, + :'1' => { :name => 'Unattributed Assistant' }, + :'2' => { :name => %w(injected names)} } } }) - permitted = params.permit book: { authors_attributes: [ :name ] } + permitted = params.permit :book => { :authors_attributes => [ :name ] } assert_not_nil permitted[:book][:authors_attributes]['0'] - assert_not_nil permitted[:book][:authors_attributes]['-1'] - assert_nil permitted[:book][:authors_attributes]['0'][:age_of_death] + assert_not_nil permitted[:book][:authors_attributes]['1'] + assert_empty permitted[:book][:authors_attributes]['2'] assert_equal 'William Shakespeare', permitted[:book][:authors_attributes]['0'][:name] - assert_equal 'Unattributed Assistant', permitted[:book][:authors_attributes]['-1'][:name] + assert_equal 'Unattributed Assistant', permitted[:book][:authors_attributes]['1'][:name] + + assert_filtered_out permitted[:book][:authors_attributes]['0'], :age_of_death + end + + test "fields_for-style nested params with negative numbers" do + params = ActionController::Parameters.new({ + :book => { + :authors_attributes => { + :'-1' => { :name => 'William Shakespeare', :age_of_death => '52' }, + :'-2' => { :name => 'Unattributed Assistant' } + } + } + }) + permitted = params.permit :book => { :authors_attributes => [:name] } + + assert_not_nil permitted[:book][:authors_attributes]['-1'] + assert_not_nil permitted[:book][:authors_attributes]['-2'] + assert_equal 'William Shakespeare', permitted[:book][:authors_attributes]['-1'][:name] + assert_equal 'Unattributed Assistant', permitted[:book][:authors_attributes]['-2'][:name] + + assert_filtered_out permitted[:book][:authors_attributes]['-1'], :age_of_death end end diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 7cc71fe6dc..6270ea4a9f 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -2,10 +2,131 @@ require 'abstract_unit' require 'action_controller/metal/strong_parameters' class ParametersPermitTest < ActiveSupport::TestCase + def assert_filtered_out(params, key) + assert !params.has_key?(key), "key #{key.inspect} has not been filtered out" + end + setup do @params = ActionController::Parameters.new({ person: { age: "32", name: { first: "David", last: "Heinemeier Hansson" } }}) + + @struct_fields = [] + %w(0 1 12).each do |number| + ['', 'i', 'f'].each do |suffix| + @struct_fields << "sf(#{number}#{suffix})" + end + end + end + + test 'if nothing is permitted, the hash becomes empty' do + params = ActionController::Parameters.new(:id => '1234') + permitted = params.permit + permitted.permitted? + permitted.empty? + end + + test 'key: permitted scalar values' do + values = ['a', :a, nil] + values += [0, 1.0, 2**128, BigDecimal.new(1)] + values += [true, false] + values += [Date.today, Time.now, DateTime.now] + values += [StringIO.new] + + values.each do |value| + params = ActionController::Parameters.new(:id => value) + permitted = params.permit(:id) + assert_equal value, permitted[:id] + + @struct_fields.each do |sf| + params = ActionController::Parameters.new(sf => value) + permitted = params.permit(:sf) + assert_equal value, permitted[sf] + end + end + end + + test 'key: unknown keys are filtered out' do + params = ActionController::Parameters.new(:id => '1234', :injected => 'injected') + permitted = params.permit(:id) + assert_equal '1234', permitted[:id] + assert_filtered_out permitted, :injected + end + + test 'key: arrays are filtered out' do + [[], [1], ['1']].each do |array| + params = ActionController::Parameters.new(:id => array) + permitted = params.permit(:id) + assert_filtered_out permitted, :id + + @struct_fields.each do |sf| + params = ActionController::Parameters.new(sf => array) + permitted = params.permit(:sf) + assert_filtered_out permitted, sf + end + end + end + + test 'key: hashes are filtered out' do + [{}, {:foo => 1}, {:foo => 'bar'}].each do |hash| + params = ActionController::Parameters.new(:id => hash) + permitted = params.permit(:id) + assert_filtered_out permitted, :id + + @struct_fields.each do |sf| + params = ActionController::Parameters.new(sf => hash) + permitted = params.permit(:sf) + assert_filtered_out permitted, sf + end + end + end + + test 'key: non-permitted scalar values are filtered out' do + params = ActionController::Parameters.new(:id => Object.new) + permitted = params.permit(:id) + assert_filtered_out permitted, :id + + @struct_fields.each do |sf| + params = ActionController::Parameters.new(sf => Object.new) + permitted = params.permit(:sf) + assert_filtered_out permitted, sf + end + end + + test 'key: it is not assigned if not present in params' do + params = ActionController::Parameters.new(:name => 'Joe') + permitted = params.permit(:id) + assert !permitted.has_key?(:id) + end + + test 'key to empty array: empty arrays pass' do + params = ActionController::Parameters.new(:id => []) + permitted = params.permit(:id => []) + assert_equal [], permitted[:id] + end + + test 'key to empty array: arrays of permitted scalars pass' do + [['foo'], [1], ['foo', 'bar'], [1, 2, 3]].each do |array| + params = ActionController::Parameters.new(:id => array) + permitted = params.permit(:id => []) + assert_equal array, permitted[:id] + end + end + + test 'key to empty array: permitted scalar values do not pass' do + ['foo', 1].each do |permitted_scalar| + params = ActionController::Parameters.new(:id => permitted_scalar) + permitted = params.permit(:id => []) + assert_filtered_out permitted, :id + end + end + + test 'key to empty array: arrays of non-permitted scalar do not pass' do + [[Object.new], [[]], [[1]], [{}], [{:id => '1'}]].each do |non_permitted_scalar| + params = ActionController::Parameters.new(:id => non_permitted_scalar) + permitted = params.permit(:id => []) + assert_filtered_out permitted, :id + end end test "fetch raises ParameterMissing exception" do @@ -73,10 +194,6 @@ class ParametersPermitTest < ActiveSupport::TestCase assert_equal "Jonas", @params[:person][:family][:brother] end - test "permitting parameters that are not there should not include the keys" do - assert !@params.permit(:person, :funky).has_key?(:funky) - end - test "permit state is kept on a dup" do @params.permit! assert_equal @params.permitted?, @params.dup.permitted? -- cgit v1.2.3