From 14a3bd520dd4bbf1247fd3e0071b59c02c115ce0 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Mon, 13 Jul 2015 16:43:21 -0400 Subject: Make AC::Parameters not inherited from Hash This is another take at #14384 as we decided to wait until `master` is targeting Rails 5.0. This commit is implementation-complete, as it guarantees that all the public methods on the hash-inherited Parameters are still working (based on test case). We can decide to follow-up later if we want to remove some methods out from Parameters. --- actionpack/CHANGELOG.md | 14 +++ .../action_controller/metal/strong_parameters.rb | 127 ++++++++++++++++----- actionpack/lib/action_dispatch/routing/url_for.rb | 4 + .../test/controller/api/params_wrapper_test.rb | 2 +- .../parameters/nested_parameters_test.rb | 2 +- .../parameters/parameters_permit_test.rb | 2 - actionpack/test/controller/test_case_test.rb | 2 +- actionpack/test/controller/webservice_test.rb | 8 +- .../dispatch/request/json_params_parsing_test.rb | 2 +- 9 files changed, 128 insertions(+), 35 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 6c6f27ce90..b1fe2af990 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,17 @@ +* `ActionController::Parameters` no longer inherits from + `HashWithIndifferentAccess` + + Inheriting from `HashWithIndifferentAccess` allowed users to call any + enumerable methods on `Parameters` object, resulting in a risk of losing the + `permitted?` status or even getting back a pure `Hash` object instead of + a `Parameters` object with proper sanitization. + + By stop inheriting from `HashWithIndifferentAccess`, we are able to make + sure that all methods that are defined in `Parameters` object will return + a proper `Parameters` object with a correct `permitted?` flag. + + *Prem Sichanugrist* + * Replaced `ActiveSupport::Concurrency::Latch` with `Concurrent::CountDownLatch` from the concurrent-ruby gem. diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 09867e2407..a93748556e 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -104,10 +104,12 @@ module ActionController # params = ActionController::Parameters.new(key: 'value') # params[:key] # => "value" # params["key"] # => "value" - class Parameters < ActiveSupport::HashWithIndifferentAccess + class Parameters cattr_accessor :permit_all_parameters, instance_accessor: false cattr_accessor :action_on_unpermitted_parameters, instance_accessor: false + delegate :keys, :key?, :has_key?, :empty?, :inspect, to: :@parameters + # By default, never raise an UnpermittedParameters exception if these # params are present. The default includes both 'controller' and 'action' # because they are added by Rails and should be of no concern. One way @@ -144,11 +146,19 @@ module ActionController # params = ActionController::Parameters.new(name: 'Francesco') # params.permitted? # => true # Person.new(params) # => # - def initialize(attributes = nil) - super(attributes) + def initialize(parameters = {}) + @parameters = parameters.with_indifferent_access @permitted = self.class.permit_all_parameters end + def ==(other_hash) + if other_hash.respond_to?(:permitted?) + super + else + @parameters == other_hash + end + end + # Returns a safe +Hash+ representation of this parameter with all # unpermitted keys removed. # @@ -162,7 +172,7 @@ module ActionController # safe_params.to_h # => {"name"=>"Senjougahara Hitagi"} def to_h if permitted? - to_hash + @parameters.to_h else slice(*self.class.always_permitted_parameters).permit!.to_h end @@ -170,20 +180,17 @@ module ActionController # Returns an unsafe, unfiltered +Hash+ representation of this parameter. def to_unsafe_h - to_hash + @parameters end alias_method :to_unsafe_hash, :to_unsafe_h # Convert all hashes in values into parameters, then yield each pair like # the same way as Hash#each_pair def each_pair(&block) - super do |key, value| - convert_hashes_to_parameters(key, value) + @parameters.each_pair do |key, value| + yield key, convert_hashes_to_parameters(key, value) end - - super end - alias_method :each, :each_pair # Attribute that keeps track of converted arrays, if any, to avoid double @@ -347,7 +354,11 @@ module ActionController # params[:person] # => {"name"=>"Francesco"} # params[:none] # => nil def [](key) - convert_hashes_to_parameters(key, super) + convert_hashes_to_parameters(key, @parameters[key]) + end + + def []=(key, value) + @parameters[key] = value end # Returns a parameter for the given +key+. If the +key+ @@ -361,8 +372,12 @@ module ActionController # params.fetch(:none) # => ActionController::ParameterMissing: param is missing or the value is empty: none # params.fetch(:none, 'Francesco') # => "Francesco" # params.fetch(:none) { 'Francesco' } # => "Francesco" - def fetch(key, *args) - convert_hashes_to_parameters(key, super, false) + def fetch(key, *args, &block) + convert_hashes_to_parameters( + key, + @parameters.fetch(key, *args, &block), + false + ) rescue KeyError raise ActionController::ParameterMissing.new(key) end @@ -375,7 +390,16 @@ module ActionController # params.slice(:a, :b) # => {"a"=>1, "b"=>2} # params.slice(:d) # => {} def slice(*keys) - new_instance_with_inherited_permitted_status(super) + new_instance_with_inherited_permitted_status(@parameters.slice(*keys)) + end + + def slice!(*keys) + @parameters.slice!(*keys) + self + end + + def except(*keys) + new_instance_with_inherited_permitted_status(@parameters.except(*keys)) end # Removes and returns the key/value pairs matching the given keys. @@ -384,7 +408,7 @@ module ActionController # params.extract!(:a, :b) # => {"a"=>1, "b"=>2} # params # => {"c"=>3} def extract!(*keys) - new_instance_with_inherited_permitted_status(super) + new_instance_with_inherited_permitted_status(@parameters.extract!(*keys)) end # Returns a new ActionController::Parameters with the results of @@ -393,36 +417,70 @@ module ActionController # params = ActionController::Parameters.new(a: 1, b: 2, c: 3) # params.transform_values { |x| x * 2 } # # => {"a"=>2, "b"=>4, "c"=>6} - def transform_values - if block_given? - new_instance_with_inherited_permitted_status(super) + def transform_values(&block) + if block + new_instance_with_inherited_permitted_status( + @parameters.transform_values(&block) + ) else - super + @parameters.transform_values end end + def transform_values!(&block) + @parameters.transform_values!(&block) + self + end + # This method is here only to make sure that the returned object has the # correct +permitted+ status. It should not matter since the parent of # this object is +HashWithIndifferentAccess+ - def transform_keys # :nodoc: - if block_given? - new_instance_with_inherited_permitted_status(super) + def transform_keys(&block) # :nodoc: + if block + new_instance_with_inherited_permitted_status( + @parameters.transform_keys(&block) + ) else - super + @parameters.transform_keys end end + def transform_keys!(&block) + @parameters.transform_keys!(&block) + self + end + # Deletes and returns a key-value pair from +Parameters+ whose key is equal # to key. If the key is not found, returns the default value. If the # optional code block is given and the key is not found, pass in the key # and return the result of block. def delete(key, &block) - convert_hashes_to_parameters(key, super, false) + convert_hashes_to_parameters(key, @parameters.delete(key), false) + end + + def select(&block) + new_instance_with_inherited_permitted_status(@parameters.select(&block)) end # Equivalent to Hash#keep_if, but returns nil if no changes were made. def select!(&block) - convert_value_to_parameters(super) + @parameters.select!(&block) + self + end + alias_method :keep_if, :select! + + def reject(&block) + new_instance_with_inherited_permitted_status(@parameters.reject(&block)) + end + + def reject!(&block) + @parameters.reject!(&block) + self + end + alias_method :delete_if, :reject! + + def values_at(*keys) + convert_value_to_parameters(@parameters.values_at(*keys)) end # Returns an exact copy of the ActionController::Parameters @@ -439,6 +497,18 @@ module ActionController end end + def merge(other_hash) + new_instance_with_inherited_permitted_status( + @parameters.merge(other_hash) + ) + end + + # This is required by ActiveModel attribute assignment, so that user can + # pass +Parameters+ to a mass assignment methods in a model. + def stringify_keys + dup + end + protected def permitted=(new_permitted) @permitted = new_permitted @@ -453,7 +523,7 @@ module ActionController def convert_hashes_to_parameters(key, value, assign_if_converted=true) converted = convert_value_to_parameters(value) - self[key] = converted if assign_if_converted && !converted.equal?(value) + @parameters[key] = converted if assign_if_converted && !converted.equal?(value) converted end @@ -482,7 +552,8 @@ module ActionController end def fields_for_style?(object) - object.is_a?(Hash) && object.all? { |k, v| k =~ /\A-?\d+\z/ && v.is_a?(Hash) } + (object.is_a?(Hash) || object.is_a?(Parameters)) && + object.to_unsafe_h.all? { |k, v| k =~ /\A-?\d+\z/ && v.is_a?(Hash) } end def unpermitted_parameters!(params) @@ -571,7 +642,7 @@ module ActionController else # Declaration { user: :name } or { user: [:name, :age, { address: ... }] }. params[key] = each_element(value) do |element| - if element.is_a?(Hash) + if element.is_a?(Hash) || element.is_a?(Parameters) element = self.class.new(element) unless element.respond_to?(:permit) element.permit(*Array.wrap(filter[key])) end diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb index 8379d089df..967bbd62f8 100644 --- a/actionpack/lib/action_dispatch/routing/url_for.rb +++ b/actionpack/lib/action_dispatch/routing/url_for.rb @@ -171,6 +171,10 @@ module ActionDispatch route_name = options.delete :use_route _routes.url_for(options.symbolize_keys.reverse_merge!(url_options), route_name) + when ActionController::Parameters + route_name = options.delete :use_route + _routes.url_for(options.to_unsafe_h.symbolize_keys. + reverse_merge!(url_options), route_name) when String options when Symbol diff --git a/actionpack/test/controller/api/params_wrapper_test.rb b/actionpack/test/controller/api/params_wrapper_test.rb index e40a39d829..53b3a0c3cc 100644 --- a/actionpack/test/controller/api/params_wrapper_test.rb +++ b/actionpack/test/controller/api/params_wrapper_test.rb @@ -7,7 +7,7 @@ class ParamsWrapperForApiTest < ActionController::TestCase wrap_parameters :person, format: [:json] def test - self.last_parameters = params.except(:controller, :action) + self.last_parameters = params.except(:controller, :action).to_unsafe_h head :ok end end diff --git a/actionpack/test/controller/parameters/nested_parameters_test.rb b/actionpack/test/controller/parameters/nested_parameters_test.rb index 3b1257e8d5..7151a8567c 100644 --- a/actionpack/test/controller/parameters/nested_parameters_test.rb +++ b/actionpack/test/controller/parameters/nested_parameters_test.rb @@ -136,7 +136,7 @@ class NestedParametersTest < ActiveSupport::TestCase authors_attributes: { :'0' => { name: 'William Shakespeare', age_of_death: '52' }, :'1' => { name: 'Unattributed Assistant' }, - :'2' => { name: %w(injected names)} + :'2' => { name: %w(injected names) } } } }) diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 2ed486516d..05532ec21b 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -253,7 +253,6 @@ class ParametersPermitTest < ActiveSupport::TestCase assert @params.to_h.is_a? Hash assert_not @params.to_h.is_a? ActionController::Parameters - assert_equal @params.to_hash, @params.to_h end test "to_h returns converted hash when .permit_all_parameters is set" do @@ -284,6 +283,5 @@ class ParametersPermitTest < ActiveSupport::TestCase test "to_unsafe_h returns unfiltered params" do assert @params.to_h.is_a? Hash assert_not @params.to_h.is_a? ActionController::Parameters - assert_equal @params.to_hash, @params.to_unsafe_h end end diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index b991232a14..0a20b2c3fb 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -45,7 +45,7 @@ class TestCaseTest < ActionController::TestCase end def test_params - render text: ::JSON.dump(params) + render text: ::JSON.dump(params.to_unsafe_h) end def test_query_parameters diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index 21fa670bb6..bd81846a7d 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -14,7 +14,13 @@ class WebServiceTest < ActionDispatch::IntegrationTest def dump_params_keys(hash = params) hash.keys.sort.inject("") do |s, k| value = hash[k] - value = Hash === value ? "(#{dump_params_keys(value)})" : "" + + if value.is_a?(Hash) || value.is_a?(ActionController::Parameters) + value = "(#{dump_params_keys(value)})" + else + value = "" + end + s << ", " unless s.empty? s << "#{k}#{value}" end diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb index d77341bc64..c2300a0142 100644 --- a/actionpack/test/dispatch/request/json_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb @@ -113,7 +113,7 @@ class RootLessJSONParamsParsingTest < ActionDispatch::IntegrationTest def parse self.class.last_request_parameters = request.request_parameters - self.class.last_parameters = params + self.class.last_parameters = params.to_unsafe_h head :ok end end -- cgit v1.2.3 From 84b861f1aae1b63525dcac99b3df2100b6739010 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Tue, 14 Jul 2015 14:57:10 -0400 Subject: Update documentation on `AC::Parameters` --- actionpack/CHANGELOG.md | 2 +- .../action_controller/metal/strong_parameters.rb | 38 ++++++++++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) (limited to 'actionpack') diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index b1fe2af990..c70e6a771f 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -6,7 +6,7 @@ `permitted?` status or even getting back a pure `Hash` object instead of a `Parameters` object with proper sanitization. - By stop inheriting from `HashWithIndifferentAccess`, we are able to make + By not inheriting from `HashWithIndifferentAccess`, we are able to make sure that all methods that are defined in `Parameters` object will return a proper `Parameters` object with a correct `permitted?` flag. diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index a93748556e..3f5e8c53a8 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -151,6 +151,9 @@ module ActionController @permitted = self.class.permit_all_parameters end + # Returns true if another +Parameters+ object contains the same content and + # permitted flag, or other Hash-like object contains the same content. This + # override is in place so you can perform a comparison with `Hash`. def ==(other_hash) if other_hash.respond_to?(:permitted?) super @@ -357,6 +360,8 @@ module ActionController convert_hashes_to_parameters(key, @parameters[key]) end + # Assigns a value to a given +key+. The given key may still get filtered out + # when +permit+ is called. def []=(key, value) @parameters[key] = value end @@ -393,11 +398,19 @@ module ActionController new_instance_with_inherited_permitted_status(@parameters.slice(*keys)) end + # Returns current ActionController::Parameters instance which + # contains only the given +keys+. def slice!(*keys) @parameters.slice!(*keys) self end + # Returns a new ActionController::Parameters instance that + # filters out the given +keys+. + # + # params = ActionController::Parameters.new(a: 1, b: 2, c: 3) + # params.except(:a, :b) # => {"c"=>3} + # params.except(:d) # => {"a"=>1,"b"=>2,"c"=>3} def except(*keys) new_instance_with_inherited_permitted_status(@parameters.except(*keys)) end @@ -427,15 +440,16 @@ module ActionController end end + # Performs values transformation and returns the altered + # ActionController::Parameters instance. def transform_values!(&block) @parameters.transform_values!(&block) self end - # This method is here only to make sure that the returned object has the - # correct +permitted+ status. It should not matter since the parent of - # this object is +HashWithIndifferentAccess+ - def transform_keys(&block) # :nodoc: + # Returns a new ActionController::Parameters instance with the + # results of running +block+ once for every key. The values are unchanged. + def transform_keys(&block) if block new_instance_with_inherited_permitted_status( @parameters.transform_keys(&block) @@ -445,6 +459,8 @@ module ActionController end end + # Performs keys transfomration and returns the altered + # ActionController::Parameters instance. def transform_keys!(&block) @parameters.transform_keys!(&block) self @@ -458,6 +474,8 @@ module ActionController convert_hashes_to_parameters(key, @parameters.delete(key), false) end + # Returns a new instance of ActionController::Parameters with only + # items that the block evaluates to true. def select(&block) new_instance_with_inherited_permitted_status(@parameters.select(&block)) end @@ -469,16 +487,21 @@ module ActionController end alias_method :keep_if, :select! + # Returns a new instance of ActionController::Parameters with items + # that the block evaluates to true removed. def reject(&block) new_instance_with_inherited_permitted_status(@parameters.reject(&block)) end + # Removes items that the block evaluates to true and returns self. def reject!(&block) @parameters.reject!(&block) self end alias_method :delete_if, :reject! + # Return values that were assigned to the given +keys+. Note that all the + # +Hash+ objects will be converted to ActionController::Parameters. def values_at(*keys) convert_value_to_parameters(@parameters.values_at(*keys)) end @@ -497,6 +520,8 @@ module ActionController end end + # Returns a new ActionController::Parameters with all keys from + # +other_hash+ merges into current hash. def merge(other_hash) new_instance_with_inherited_permitted_status( @parameters.merge(other_hash) @@ -504,8 +529,9 @@ module ActionController end # This is required by ActiveModel attribute assignment, so that user can - # pass +Parameters+ to a mass assignment methods in a model. - def stringify_keys + # pass +Parameters+ to a mass assignment methods in a model. It should not + # matter as we are using +HashWithIndifferentAccess+ internally. + def stringify_keys # :nodoc: dup end -- cgit v1.2.3