From 8525cc325391bde15c86ee9be4b57cfc3330988e Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sat, 10 Aug 2024 11:03:37 -0700 Subject: [PATCH] doc updates --- actionpack/CHANGELOG.md | 47 +++- .../metal/strong_parameters.rb | 265 ++++++++++-------- .../parameters/parameters_allow_test.rb | 6 +- .../parameters/parameters_expect_test.rb | 12 +- 4 files changed, 189 insertions(+), 141 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index d0b2c349971d2..5d24d3c55df78 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,39 +1,56 @@ -* Introduce `params.expect` as the recommended way to filter params. - `params.expect` combines the usage of `params.permit` and `params.require` - to ensure url params are filtered with consideration for the expected - types of values, improving handling of manipulated params. +* Introduce safer and more explicit params handling methods. + - `params.expect` replaces `params.require(:table).permit(:attr)` + - `params.allow` is like `expect` but for optional params. - In the following example, each version returns the same result with - unaltered params, but the first example will raise generic NoMethodError, - causing the server to return 500 and potentially sending exception notices. + Each new method ensures params are filtered with consideration for + the expected types of values, improving handling of params and avoids + errors caused by params tampering. ```ruby # If the url is altered to ?person=hacked # Before - params.require(:person).permit(:name, :age, pie: [:flavor]) + params.require(:person).permit(:name, :age, pets: [[:name]]) # raises NoMethodError, causing a 500 and potential error reporting - params.permit(person: [:name, :age, { pie: [:flavor] }]).require(:person) - # raises ActionController::ParameterMissing, correctly returning a 400 error # After - params.expect(person: [:name, :age, { address: [:city, :state] }]) + params.expect(person: [:name, :age, pet: [[:name]]) # raises ActionController::ParameterMissing, correctly returning a 400 error ``` + You may also notice the new double array `[[:name]]`. In order to + declare when a param is expected to be an array of params hashes, + this new double array syntax is used. All new methods expect arrays + to be declared this way. + + In order to preserve compatibility, `permit` does not adopt the new + double array syntax and is therefore more permissive about wrong + types. Using `expect` or `allow` everywhere is recommended. + We suggest replacing `params.require(:person).permit(:name, :age)` with the direct replacement `params.expect(person: [:name, :age])` - to prevent external users from manipulating params to trigger 500 errors. + to prevent external users from manipulating params to trigger 500 + errors when the correct response is a 400 error. - Usage of `params.require` should likewise be replaced with `params.expect`. + Usage of `params.require(:id)` should likewise be replaced with `params.expect(:id)` which is designed to ensure that `params[:id]` + is a scalar and not an array or hash. ```ruby # Before - User.find(params.require(:id)) # will allow an array, altering behavior + User.find(params.require(:id)) # allows an array, altering behavior # After - User.find(params.expect(:id)) # only returns non-blank permitted scalars (excludes Hash, Array, nil, "", etc) + User.find(params.expect(:id)) # expect only returns non-blank permitted scalars (excludes Hash, Array, nil, "", etc) ``` + `allow` behaves similar to `expect`, returning nil, array, or hash + when a param is not of the correct type or is blank or missing. + `allow` is useful when a param is optional, but needs to be permitted + if it is present. + + Also adds `expect_only`, `allow_only`, and `permit_only` which raise + on the presence of unpermitted params, overriding default behavior. + Each optionally accepts a block that will yield the unpermitted keys. + *Martin Emde* * Speed up `ActionDispatch::Routing::Mapper::Scope#[]` by merging frame hashes. diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index a24a2b37d717f..3e3f385ed81cf 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -80,11 +80,13 @@ class InvalidParameterKey < ArgumentError # # Allows you to choose which attributes should be permitted for mass updating # and thus prevent accidentally exposing that which shouldn't be exposed. - # Provides three methods for this purpose: #expect, #require, and #permit. - # `expect` is used to permit and require parameters in one step. `require` - # and `permit` are available for more more precise control to mark parameters - # as required set the parameters as permitted to limit which attributes should - # be allowed for mass updating. + # + # Provides methods for filtering and requiring params: + # + # - `expect` to safely permit and require parameters in one step. + # - `allow` is the same as `expect`, but for optional parameters. + # - `permit` to filter params for mass assignment. + # - `require` to require a parameter or raise an error. # # params = ActionController::Parameters.new({ # person: { @@ -101,7 +103,8 @@ class InvalidParameterKey < ArgumentError # Person.first.update!(permitted) # # => # # - # It provides two options that control the top-level behavior of new instances: + # Paramaters provides two options that control the top-level behavior of new + # instances: # # * `permit_all_parameters` - If it's `true`, all the parameters will be # permitted by default. The default is `false`. @@ -116,8 +119,6 @@ class InvalidParameterKey < ArgumentError # * `:raise` to raise an ActionController::UnpermittedParameters # exception. # - # - # # Examples: # # params = ActionController::Parameters.new @@ -142,6 +143,22 @@ class InvalidParameterKey < ArgumentError # environment they should only be set once at boot-time and never mutated at # runtime. # + # Each of `expect`, `allow`, and `permit` also have matching methods + # `expect_only`, `allow_only`, and `permit_only` that raise an error when + # unpermitted parameters are present. + # + # The `_only` methods override the setting `action_on_unpermitted_parameters` + # allowing each action to define how unpermitted parameters are handled. + # + # ActionController::Parameters.action_on_unpermitted_parameters = :log + # params = ActionController::Parameters.new(options: { a: "123", b: "456" }) + # params.permit_only(options: [:a]) + # # => ActionController::UnpermittedParameters: found unpermitted keys: b + # # `*_only` methods also accept a block to handle unpermitted keys + # params.allow_only(options: [:a]) { |keys| @extra_keys = keys } + # # => #"123"} permitted: true> + # # @extra_keys # => ["b"] + # # You can fetch values of `ActionController::Parameters` using either `:key` or # `"key"`. # @@ -681,59 +698,85 @@ def permit(*filters) # Same as permit, but raises on unpermitted parameters by default. # Ignores ActionController::Parameters.action_on_unpermitted_parameters # - # If a block is given, it is called with the unpermitted keys. + # Accepts a block which will yield any unpermitted keys instead of raising. def permit_only(*filters, &block) block ||= method(:raise_unpermitted_parameters) permit_filters(filters, &block) end - # `expect` is the preferred way to require and permit parameters and work - # with nested parameters and Rails form params. + # `expect` is the preferred way to require and permit parameters. + # It safer than calling `permit` and `require` in sequence, which was + # previously recommended. + # + # `expect` is more strict with types to avoid a number potential pitfalls + # that may be encountered with the `.require.permit` pattern. + # + # For example: # - # It is similar to calling `permit` and `require` in sequence. - # Additionally, it avoids some potential pitfalls. Using `expect` protects - # an application from user triggered `NoMethodErrors` that can be caused by - # manipulated params. + # params = ActionController::Parameters.new(comment: { text: "hello" }) + # params.expect(comment: [:text]) + # # => # # - # `expect` adds the additional behavior of requiring `filetrs.keys` if one - # of the filters arguments is a Hash. - # This can be convenient for the standard format of params from Rails forms. + # params = ActionController::Parameters.new(comment: [{ text: "hello" }, { text: "world" }]) + # params.expect(comment: [:text]) + # # => ActionController::ParameterMissing: param is missing or the value is empty: comment # - # params = ActionController::Parameters.new(user: { name: "Martin", age: 40, role: "admin" }) - # permitted = params.require(:user).permit(:name, :age) # not recommended - # permitted = params.expect(user: [:name, :age]) # recommended - # # => #"Martin", "age"=>40} permitted: true> + # In order to permit an array of parameters, the array must be defined + # explicitly. Use double array brackets, an array inside an array, to + # declare that an array of parameters is expected. # - # In the example, `expect` will require the `:user` key and permit the - # `:name` and `:age` keys, and return the permitted params `{ name: "Martin", age: 40 }`. - # It is functionally the same as the following: + # params = ActionController::Parameters.new(comment: [{ text: "hello" }, { text: "world" }]) + # params.expect(comments: [[:text]]) + # # => [# "hello" } permitted: true>, + # # # "world" } permitted: true>] # - # permitted = params.permit(user: [:name, :age]).require(:user) + # params = ActionController::Parameters.new(comment: { text: "hello" }) + # params.expect(comments: [[:text]]) + # # => ActionController::ParameterMissing: param is missing or the value is empty: comments # - # `expect` aims to correct a potential error that can be triggered when an end user - # alters the params. In the following example, the params are altered by the user in - # order to cause a 500 error. + # `expect` is intended to protect against array tampering. # # params = ActionController::Parameters.new(user: "hack") - # params.require(:user).permit(:name, :age) # not recommended + # # The previous way of requiring and permitting parameters will error + # params.require(:user).permit(:name, pets: [:name]) # wrong # # => NoMethodError: undefined method `permit' for an instance of String - # params.expect(user: [:name, :age]) # handled as expected + # + # # similarly with nested parameters + # params = ActionController::Parameters.new(user: { name: "Martin", pets: { name: "hack" } }) + # user_params = params.require(:user).permit(:name, pets: [:name]) # wrong + # # user_params[:pets] is expected to be an array but is a hash + # + # `expect` solves this by being more strict with types. + # + # params = ActionController::Parameters.new(user: "hack") + # params.expect(user: [:name, { pets: [[:name]] }]) # # => ActionController::ParameterMissing: param is missing or the value is empty: user # - # Like `permit`, `expect` only allows permitted scalars to pass the filter. - # For example, + # # with nested parameters + # params = ActionController::Parameters.new(user: { name: "Martin", pets: { name: "hack" } }) + # user_params = params.expect(user: [:name, { pets: [[:name]] }]) + # user_params[:pets] # => [] # - # params.expect(:name) + # As the examples show, `expect` requires the `:user` key, and any root keys + # similarl to the `.require.permit` pattern. If multiple root keys are + # expected, they will all be required. + + # params = ActionController::Parameters.new(name: "Martin", pies: [{ type: "dessert", flavor: "pumpkin"}]) + # name, pies = params.expect(:name, pies: [:type, :flavor]) + # name # => "Martin" + # pies # => #"dessert", "flavor"=>"pumpkin"} permitted: true> # - # If `:name` is a key in params and its value is a `String`, `Symbol`, `NilClass`, - # `Numeric`, `TrueClass`, `FalseClass`, `Date`, `Time`, `DateTime`, `StringIO`, `IO`, - # ActionDispatch::Http::UploadedFile or `Rack::Test::UploadedFile`, it is permitted. + # When called with a hash with multiple keys, `expect` will permit the + # parameters and require the keys in the order they are given in the hash, + # returning an array of the permitted parameters. # - # If `:name` is missing, or if the value of `:name` is blank, nil, a `Hash` or an `Array`, - # then ActionController::ParameterMissing is raised. + # params = ActionController::Parameters.new(subject: { name: "Martin" }, object: { pie: "pumpkin" }) + # subject, object = params.expect(subject: [:name], object: [:pie]) + # subject # => #"Martin"} permitted: true> + # object # => #"pumpkin"} permitted: true> # - # You can also `expect` nested parameters just like permit. The result will - # `require(filters.keys)` on the permitted parameters. + # Besidse being more strict about array vs hash params, `expect` uses permit + # internally, so it will behave similarly. # # params = ActionController::Parameters.new({ # person: { @@ -746,15 +789,14 @@ def permit_only(*filters, &block) # } # }) # - # permitted = params.expect(person: [ :name, { pets: :name } ]) + # permitted = params.expect(person: [ :name, { pets: [[:name]] } ]) # permitted.permitted? # => true # permitted[:name] # => "Francesco" # permitted[:age] # => nil # permitted[:pets][0][:name] # => "Purplish" # permitted[:pets][0][:category] # => nil # - # You may declare that the parameter should be an array of permitted scalars by - # mapping it to an empty array: + # An array of permitted scalars may be expected with the following: # # params = ActionController::Parameters.new(tags: ["rails", "parameters"]) # permitted = params.expect(tags: []) @@ -762,64 +804,14 @@ def permit_only(*filters, &block) # permitted.is_a?(Array) # => true # permitted.size # => 2 # - # Like `permit`, `expect` can be used to permit nested parameters. - # Be careful because this opens the door to arbitrary input. In this case, - # `permit` ensures values in the returned structure are permitted scalars and - # filters out anything else. For example: - # - # params = ActionController::Parameters.new(user: { name: "Martin", age: 40, hax: [] }) - # permitted = params.expect(user: {}) - # permitted.permitted? # => true - # permitted.has_key?(:name) # => true - # permitted.has_key?(:age) # => true - # permitted.has_key?(:hax) # => false - # - # Note that if you use `expect` on a key that points to a hash, it will not - # allow the hash. You also need to specify which attributes inside the hash - # should be permitted. - # - # params = ActionController::Parameters.new({ - # person: { - # contact: { - # email: "none@test.com", - # phone: "555-1234" - # } - # } - # }) - # - # params.expect(person: :contact) # wrong if a hash is expected - # # => ActionController::ParameterMissing: param is missing or the value is empty: person - # - # params.expect(person: { contact: :phone }) # correct - # # => ##"555-1234"} permitted: true>} permitted: true> - # - # params.expect(person: { contact: [ :email, :phone ] }) # correct - # # => ##"none@test.com", "phone"=>"555-1234"} permitted: true>} permitted: true> - # - # Use `expect` to require multiple top level parameters. - # - # params = ActionController::Parameters.new(name: "Martin", age: 40, pies: [{ type: "dessert", flavor: "pumpkin"}]) - # name, age, pies = params.expect(:name, :age, pies: [:type, :flavor]) - # name # => "Martin" - # age # => 40 - # pies # => #"dessert", "flavor"=>"pumpkin"} permitted: true> - # - # When called with a hash with multiple keys, `expect` will permit the - # parameters and require the keys in the order they are given in the hash, - # returning an array of the permitted parameters. - # - # params = ActionController::Parameters.new(subject: { name: "Martin" }, object: { pie: "pumpkin" }) - # subject, object = params.expect(subject: [:name], object: [:pie]) - # subject # => #"Martin"} permitted: true> - # object # => #"pumpkin"} permitted: true> - # def expect(*filters) expect_only(*filters, &method(:unpermitted_parameters)) end - # Same as expect, but raises on unpermitted parameters, ignoring 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. + # Accepts a block which will yield any unpermitted keys instead of raising. def expect_only(*filters, &block) block ||= method(:raise_unpermitted_parameters) params = permit_filters(filters, explicit_arrays: true, check_unpermitted: false, &block) @@ -828,29 +820,60 @@ def expect_only(*filters, &block) values.size == 1 ? values.first : values end - # The 'allow' method provides a way to expect optional nested params in a way - # that is safe from user input. It is similar to `expect`, but it does not - # raise an error if the parameter is missing or the value is empty. + # The 'allow' method provides a way to expect optional nested params. + # Similar to `expect`, `allow` is safe from param tampering. Instead of + # raising an error when a root key is missing, it will return `nil`, an + # empty Array, or an empty Hash depending on the expected type. # - # `allow` is a preferred substitute for the following: + # For example, you might previously have needed something like this: # - # params.fetch(:user, {}).permit(:name, :age) + # params = ActionController::Parameters.new(user: "hack") + # # It can be difficult to require and permit optional parameters + # params.fetch(:user, nil)&.permit(:name, :age) # not recommended + # # => NoMethodError: undefined method `permit' for an instance of String + # # (might return nil, hash, or could raise, making actions complicated) # - # `allaw` will substitute nil, an empty hash, or array depending on - # what was expected by the given filters. + # Instead, use `allow`, which will return safely if the key is missing + # or if the wrong type is sent. # - # params = ActionController::Parameters.new({ id: 1 }) + # params = ActionController::Parameters.new(user: "hack") # person_params = params.allow(person: [:name, :age]) + # # => # # person_params[:name] # => nil # person_params[:age] # => nil # + # In order to return the correct type, `allow` requires arrays of params to + # be defined explicitly. Use double array brackets, an array inside an + # array, to declare that an array of parameters is expected. + # + # params = ActionController::Parameters.new(comment: [{ text: "hello" }, { text: "world" }]) + # params.allow(comments: [[:text]]) + # # => [# "hello" } permitted: true>, + # # # "world" } permitted: true>] + # + # params = ActionController::Parameters.new(comment: { text: "hello" }) + # params.allow(comments: [[:text]]) + # # => [] + # + # Conversely, if a hash is expected, but an arary is given, `allow` will + # return an empty hash. + # + # params = ActionController::Parameters.new(comment: { text: "hello" }) + # params.allow(comment: [:text]) + # # => # + # + # params = ActionController::Parameters.new(comment: [{ text: "hello" }, { text: "world" }]) + # params.allow(comment: [:text]) + # # => # + # def allow(*filters) allow_only(*filters, &method(:unpermitted_parameters)) end - # Same as allow, but raises on unpermitted parameters, ignoring 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. + # Accepts a block which will yield any unpermitted keys instead of raising. def allow_only(*filters, &block) block ||= method(:raise_unpermitted_parameters) params = permit_filters(filters, explicit_arrays: true, check_unpermitted: false, &block) @@ -1361,6 +1384,20 @@ def each_element(object, filter, &block) end end + # Called when an explicit array filter is encountered. + 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 unpermitted_keys(params) keys - params.keys - always_permitted_parameters end @@ -1489,20 +1526,6 @@ def hash_filter(params, filter, explicit_arrays: false, &block) 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 dfc669a932953..c681d34d4fa6e 100644 --- a/actionpack/test/controller/parameters/parameters_allow_test.rb +++ b/actionpack/test/controller/parameters/parameters_allow_test.rb @@ -58,7 +58,7 @@ class ParametersAllowTest < ActiveSupport::TestCase assert_equal({}, pies.to_h) end - test "key to nested array: returns permitted array" do + test "key to explicit array: returns permitted array" do params = ActionController::Parameters.new(name: "Martin", pies: [{ flavor: "pumpkin" }, { flavor: "chicken pot" }]) pies = params.allow(pies: [[:flavor]]) @@ -66,13 +66,13 @@ class ParametersAllowTest < ActiveSupport::TestCase assert_equal({ "flavor" => "chicken pot" }, pies[1].to_h) end - test "key to nested array: returns array when params is a hash" do + test "key to explicit 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 + test "key to explicit array: returns empty array when params empty array" do params = ActionController::Parameters.new(name: "Martin", pies: []) assert_equal [], params.allow(pies: [[:flavor]]) diff --git a/actionpack/test/controller/parameters/parameters_expect_test.rb b/actionpack/test/controller/parameters/parameters_expect_test.rb index 063a83f979557..cb1ac6c65140f 100644 --- a/actionpack/test/controller/parameters/parameters_expect_test.rb +++ b/actionpack/test/controller/parameters/parameters_expect_test.rb @@ -52,7 +52,7 @@ class ParametersExpectTest < ActiveSupport::TestCase end end - test "key to nested array: returns permitted array" do + test "key to explicit array: returns permitted array" do params = ActionController::Parameters.new(name: "Martin", pies: [{ flavor: "pumpkin" }, { flavor: "chicken pot" }]) pies = params.expect(pies: [[:flavor]]) @@ -60,7 +60,7 @@ class ParametersExpectTest < ActiveSupport::TestCase assert_equal({ "flavor" => "chicken pot" }, pies[1].to_h) end - test "key to nested array: raises when params is a hash" do + test "key to explicit array: returns array when params is a hash" do params = ActionController::Parameters.new(name: "Martin", pies: { flavor: "pumpkin" }) assert_raises(ActionController::ParameterMissing) do @@ -68,6 +68,14 @@ class ParametersExpectTest < ActiveSupport::TestCase end end + test "key to explicit array: returns empty array when params empty array" do + params = ActionController::Parameters.new(name: "Martin", pies: []) + + assert_raises(ActionController::ParameterMissing) do + params.expect(pies: [[:flavor]]) + end + end + test "key to mixed array: returns permitted params" do permitted = @params.expect(person: [:age, { name: [:first, :last] }])