Skip to content
Browse files

strong parameters filters permitted scalars

  • Loading branch information...
1 parent 996aba3 commit cbec22ce5783c9530b3e2735a8e13eeeca00e0d2 @fxn fxn committed Jan 20, 2013
View
17 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.
View
114 actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -184,6 +184,20 @@ def require(key)
# 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 @@ def permit(*filters)
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 @@ def unpermitted_parameters!(params)
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] == []
@carlosantoniodasilva
Ruby on Rails member

Perhaps it'd be better to check if it's an Array and it's empty, to avoid a new [] for each item? Or maybe creating the empty item outside the block, only once, to check for it?

@fxn
Ruby on Rails member
fxn added a note Jan 21, 2013

Yeah, we could use a constant as we do for permitted types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ # 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
View
56 actionpack/test/controller/parameters/nested_parameters_test.rb
@@ -2,6 +2,10 @@
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
View
125 actionpack/test/controller/parameters/parameters_permit_test.rb
@@ -2,10 +2,131 @@
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?

4 comments on commit cbec22c

@carlosantoniodasilva
Ruby on Rails member

@fxn I think that it is now impossible for me to say that the same value can be either a String or an Array of Strings after these changes, correct?

@fxn
Ruby on Rails member
fxn commented on cbec22c Jan 22, 2013

Yes, it must be one or the other. Do you have a use case where you'd need that?

@carlosantoniodasilva
Ruby on Rails member

@fxn yeah =(.. Questions that can receive a different answer, from text fields or check boxes, depending on their types.

@fxn
Ruby on Rails member
fxn commented on cbec22c Jan 22, 2013

@carlosantoniodasilva yeah... saw the conversation in Basecamp.

Please sign in to comment.
Something went wrong with that request. Please try again.