Closed
Conversation
p8
force-pushed
the
actionpack/strong-params-require-ignore-scalar
branch
2 times, most recently
from
117ee0c to
be7e314
Compare
The usage of `ActionController::Parameters#require` is a bit ambiguous.
It can be called for hashes, arrays and scalar values.
When a scalar value is expected but a hash is passed you might get
unexpected results:
ActionController::Parameters.new(name: { first: "Francesco" }).require(:name).downcase
NoMethodError (undefined method `downcase' for #<ActionController::Parameters:0x00007f8c631264b0>)
Similarly, when a hash is expected but a scalar value is passed:
ActionController::Parameters.new(name: "Francesco").require(:name).permit
NoMethodError (undefined method `permit' for "Francesco":String)
There even is a warning documented in the rdoc of `require` to be
careful when requiring terminal values. For example, calling require
without permit can have unexpected results if unpermitted values are
passed:
ActionController::Parameters.new(name: Object.new).require(:name)
# => #<Object:0x00007f8c58637180>
By restricting `require` to arrays and hashes, and adding a
`require_scalar` method for scalar values we can prevent these problems.
`require_scalar` can also restrict the required values to permitted
scalar values.
p8
force-pushed
the
actionpack/strong-params-require-ignore-scalar
branch
from
be7e314 to
6e260aa
Compare
p8
changed the title
p8
changed the title
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Contributor
|
RubyGems.org gets hit with enough pen-testing that this is a real annoyance. I would really love to see something like this in rails, but barring that, I'd like to see your code extracted to a gem. @p8, what do you think? |
Member
Author
|
@martinemde Feel free to extract this to a gem 😄 |
4 tasks
Contributor
Member
Author
|
@MatheusRich yes! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The usage of
ActionController::Parameters#requireis a bit ambiguous.It can be called for hashes, arrays and scalar values.
When a scalar value is expected but a hash is passed you might get
unexpected results:
Similarly, when a hash is expected but a scalar value is passed:
This requires developers to handle these unexpected exceptions instead
of just rescuing/ignoring
ActionController::ParameterMissing.There even is a warning documented in the rdoc of
requireto becareful when requiring terminal values. For example, calling require
without permit can have unexpected results if unpermitted values are
passed:
Separate
requiremethods for scalar and non scalar paramsBy restricting
requireto arrays and hashes, and adding arequire_scalarmethod for scalar values we can prevent these problems.This allows us to raise an
ActionController::ParameterMissingif arequired param doesn't have the expected type:
require_scalaralso restricts the required values to permittedscalar values.
Fixes: #42953
Other Information
This would be a breaking change requiring deprecation warnings.
Maybe we should make it even stricter and introduce methods
for each permited scalar type. This allows us to have stricter
checks and maybe even coerce to the proper type: