Skip to content
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

Improve requiring of scalar parameters #44297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

p8
Copy link
Member

@p8 p8 commented Jan 31, 2022

Summary

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(person: "Francesco").require(:person).permit(:name)
# => NoMethodError (undefined method `permit' for "Francesco":String)

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 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>

Separate require methods for scalar and non scalar params

By restricting require to arrays and hashes, and adding a
require_scalar method for scalar values we can prevent these problems.

This allows us to raise an ActionController::ParameterMissing if a
required param doesn't have the expected type:

# when a hash is passed but a scalar value is expected
ActionController::Parameters.new(name: { first: "Francesco" }).require_scalar(:name).downcase
# => ActionController::ParameterMissing (param is missing or the value is empty: name)

# when a scalar value is passed but a hash is expected
ActionController::Parameters.new(person: "Francesco").require(:person).permit(:name)
# => ActionController::ParameterMissing (param is missing or the value is empty: person)

require_scalar also restricts the required values to permitted
scalar values.

ActionController::Parameters.new(name: Object.new).require_scalar(:name)
# => ActionController::ParameterMissing (param is missing or the value is empty: name)

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:

params = ActionController::Parameters.new(name: "Francesco", age: "22", active: "true")
params.require_string(:name) # => "Francesco"
params.require_integer(:age) # => 22
params.require_boolean(:active) # => true

@rails-bot rails-bot bot added the actionpack label Jan 31, 2022
@p8 p8 force-pushed the actionpack/strong-params-require-ignore-scalar branch 2 times, most recently from 117ee0c to be7e314 Compare January 31, 2022 19:33
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 p8 force-pushed the actionpack/strong-params-require-ignore-scalar branch from be7e314 to 6e260aa Compare January 31, 2022 19:40
@p8 p8 changed the title Split Parameters#require method for scalar and non-scalar values Add Parameters#require_scalar method for scalar values Jan 31, 2022
@p8 p8 changed the title Add Parameters#require_scalar method for scalar values Improve requiring of scalar parameters Feb 1, 2022
@rails-bot
Copy link

rails-bot bot commented May 7, 2022

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.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label May 7, 2022
@martinemde
Copy link
Contributor

martinemde commented Apr 5, 2024

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?

@rails-bot rails-bot bot removed the stale label Apr 5, 2024
@p8
Copy link
Member Author

p8 commented Apr 5, 2024

@martinemde Feel free to extract this to a gem 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strong Params require with permit combination produce unexpected exception
2 participants