New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AC::Parameters not inherited from Hash #20868

Merged
merged 2 commits into from Jul 15, 2015
Jump to file or symbol
Failed to load files and symbols.
+167 −38
Diff settings

Always

Just for now

Copy path View file
@@ -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 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.
*Prem Sichanugrist*
* Replaced `ActiveSupport::Concurrency::Latch` with `Concurrent::CountDownLatch`
from the concurrent-ruby gem.
@@ -104,10 +104,12 @@ def initialize(params) # :nodoc:
# 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,22 @@ def self.const_missing(const_name)
# params = ActionController::Parameters.new(name: 'Francesco')
# params.permitted? # => true
# Person.new(params) # => #<Person id: nil, name: "Francesco">
def initialize(attributes = nil)
super(attributes)
def initialize(parameters = {})
@parameters = parameters.with_indifferent_access

This comment has been minimized.

@tenderlove

tenderlove Jul 15, 2015

Member

Does this need to convert all incoming hashes to HWIAs? Or can we change initialize to:

def initialize(parameters = {}.with_indifferent_access)
  @parameters = parameters

I want to make sure the constructor is doing as little work as possible.

This comment has been minimized.

@sikachu

sikachu Jul 15, 2015

Member

I tried to change the constructor to not calling .with_indifferent_access but that totally broke the tests. I think it's better to keep it like that to make sure that params we are dealing with is HWIA. I feel like we'll run into weird issue if we don't do that.

@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
else
@parameters == other_hash
end
end
# Returns a safe +Hash+ representation of this parameter with all
# unpermitted keys removed.
#
@@ -162,28 +175,25 @@ def initialize(attributes = nil)
# 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
end
# 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 <tt>Hash#each_pair</tt>
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 +357,13 @@ def permit(*filters)
# params[:person] # => {"name"=>"Francesco"}
# params[:none] # => nil
def [](key)
convert_hashes_to_parameters(key, super)
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
# Returns a parameter for the given +key+. If the +key+
@@ -361,8 +377,12 @@ def [](key)
# 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 +395,24 @@ def fetch(key, *args)
# 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
# Returns current <tt>ActionController::Parameters</tt> instance which
# contains only the given +keys+.
def slice!(*keys)
@parameters.slice!(*keys)
self
end
# Returns a new <tt>ActionController::Parameters</tt> 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
# Removes and returns the key/value pairs matching the given keys.
@@ -384,7 +421,7 @@ def slice(*keys)
# 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 <tt>ActionController::Parameters</tt> with the results of
@@ -393,36 +430,80 @@ def extract!(*keys)
# 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
# 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)
# Performs values transformation and returns the altered
# <tt>ActionController::Parameters</tt> instance.
def transform_values!(&block)
@parameters.transform_values!(&block)
self
end
# Returns a new <tt>ActionController::Parameters</tt> 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)
)
else
super
@parameters.transform_keys
end
end
# Performs keys transfomration and returns the altered
# <tt>ActionController::Parameters</tt> instance.
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
# Returns a new instance of <tt>ActionController::Parameters</tt> with only
# items that the block evaluates to true.
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!
# Returns a new instance of <tt>ActionController::Parameters</tt> 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 <tt>ActionController::Parameters</tt>.
def values_at(*keys)
convert_value_to_parameters(@parameters.values_at(*keys))
end
# Returns an exact copy of the <tt>ActionController::Parameters</tt>
@@ -439,6 +520,21 @@ def dup
end
end
# Returns a new <tt>ActionController::Parameters</tt> with all keys from
# +other_hash+ merges into current hash.
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. It should not
# matter as we are using +HashWithIndifferentAccess+ internally.
def stringify_keys # :nodoc:
dup
end
protected
def permitted=(new_permitted)
@permitted = new_permitted
@@ -453,7 +549,7 @@ def new_instance_with_inherited_permitted_status(hash)
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 +578,8 @@ def each_element(object)
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 +668,7 @@ def hash_filter(params, filter)
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
@@ -171,6 +171,10 @@ def url_for(options = nil)
route_name = options.delete :use_route
_routes.url_for(options.symbolize_keys.reverse_merge!(url_options),
route_name)
when ActionController::Parameters

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 13, 2015

Member

In fact, I think we should not allow parameters instance directly on url_for. See #20797. I'll work with @byroot to finish that so we can remove all this code.

This comment has been minimized.

@sikachu

sikachu Jul 14, 2015

Member

Sounds good. There actually are tests that failed if I didn't modify this line, though.

I'll let you decide if you want to get #20797 in first and have me rebase it or not.

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
@@ -7,7 +7,7 @@ class UsersController < ActionController::API
wrap_parameters :person, format: [:json]
def test
self.last_parameters = params.except(:controller, :action)
self.last_parameters = params.except(:controller, :action).to_unsafe_h

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 13, 2015

Member

Do we have a Parameters#to_h?

This comment has been minimized.

@sikachu

sikachu Jul 13, 2015

Member

Yes, but because of #18006 to_h only returns safe parameters (after slicing) while to_unsafe_h returns everything as-is.

Since this test is testing all parameters and doesn't care about Strong Parameters, to_unsafe_h works fine here.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 13, 2015

Member

Yeah, that is that I thought 👍

head :ok
end
end
@@ -136,7 +136,7 @@ def assert_filtered_out(params, key)
authors_attributes: {
:'0' => { name: 'William Shakespeare', age_of_death: '52' },
:'1' => { name: 'Unattributed Assistant' },
:'2' => { name: %w(injected names)}
:'2' => { name: %w(injected names) }
}
}
})
@@ -253,7 +253,6 @@ def assert_filtered_out(params, key)
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 @@ def assert_filtered_out(params, key)
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
@@ -45,7 +45,7 @@ def render_body
end
def test_params
render text: ::JSON.dump(params)
render text: ::JSON.dump(params.to_unsafe_h)
end
def test_query_parameters
@@ -14,7 +14,13 @@ def assign_parameters
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
@@ -113,7 +113,7 @@ class << self
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
@@ -91,6 +91,16 @@ def url_for(options = nil)
end
end
super(options)
when ActionController::Parameters
unless options.key?(:only_path)
if options[:host].nil?
options[:only_path] = _generate_paths_by_default
else
options[:only_path] = false
end
end
super(options)
when :back
_back_url
ProTip! Use n and p to navigate between commits in a pull request.