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

Malformed parameters could make an example of ActionController raise NoMethodError #30519

Closed
akihikodaki opened this Issue Sep 4, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@akihikodaki

akihikodaki commented Sep 4, 2017

Steps to reproduce

For example,
http://api.rubyonrails.org/v5.1/classes/ActionController/Parameters.html#method-i-permit

params = ActionController::Parameters.new(user: { name: "Francesco", age: 22, role: "admin" })
permitted = params.require(:user).permit(:name, :age)

That works perfectly, but it cannot properly handle the following parameter:

{ user: "Francesco" }

Expected behavior

It should raise an error which can be handled easily like ActionController::ParameterMissing, and Rails should do so by returning 422.

Actual behavior

It raises:

NoMethodError: undefined method `permit'

System configuration

Rails version:
5.1.0

Ruby version:

ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]

khall added a commit to khall/rails that referenced this issue Oct 3, 2017

@Datasnuten

This comment has been minimized.

Show comment
Hide comment
@Datasnuten

Datasnuten Nov 8, 2017

NoMethodError occurs in this case because params.require(:user) returns the string "Fransesco", and the Ruby String class doesn't have any method named permit. The same happens, for example, with FalseClass if you try to Parameters.new(person: false).require(person).permit().
This is the intended behavior unless you want to force monkey patch an exception for every permit that isn't a Parameters instance.

Datasnuten commented Nov 8, 2017

NoMethodError occurs in this case because params.require(:user) returns the string "Fransesco", and the Ruby String class doesn't have any method named permit. The same happens, for example, with FalseClass if you try to Parameters.new(person: false).require(person).permit().
This is the intended behavior unless you want to force monkey patch an exception for every permit that isn't a Parameters instance.

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Nov 8, 2017

Even if params.require(:user).permit(:name, :age) is expected to be failed with the exception, the example and applications in real world aren't expected to fail with the exception, are they?
I think a new API should be introduced to avoid the issue.

akihikodaki commented Nov 8, 2017

Even if params.require(:user).permit(:name, :age) is expected to be failed with the exception, the example and applications in real world aren't expected to fail with the exception, are they?
I think a new API should be introduced to avoid the issue.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 9, 2017

Member

Yes, they are. require doesn't return only hashes that responds to permit. It is the job of your application to handle those cases.

Member

rafaelfranca commented Nov 9, 2017

Yes, they are. require doesn't return only hashes that responds to permit. It is the job of your application to handle those cases.

@sevenseacat

This comment has been minimized.

Show comment
Hide comment
@sevenseacat

sevenseacat Aug 2, 2018

Contributor

I think this should be looked at again. Expecting a hash of data is a very very common use case of strong parameters - the params.require(model_key).permit(field_names) pattern appears everywhere - and every single time is a potential 500 error to the user unless extra code is added to handle it. (Not just "those cases", it's almost every case.)

Contributor

sevenseacat commented Aug 2, 2018

I think this should be looked at again. Expecting a hash of data is a very very common use case of strong parameters - the params.require(model_key).permit(field_names) pattern appears everywhere - and every single time is a potential 500 error to the user unless extra code is added to handle it. (Not just "those cases", it's almost every case.)

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Aug 2, 2018

It may be too late, but I missed a reply so let me make a late (but simple) reply.

If you don't think you need an additional API, you still need to update the documentation, as the example is cited from it. You have closed this issue, but do you have one for the documentation problem?

akihikodaki commented Aug 2, 2018

It may be too late, but I missed a reply so let me make a late (but simple) reply.

If you don't think you need an additional API, you still need to update the documentation, as the example is cited from it. You have closed this issue, but do you have one for the documentation problem?

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