0

Is there a best practice to validate params in a controller?

@user = User.find_by_id(params[:id])

If I tamper with the param to give it an invalid :id param, say by visiting "/users/test", I can generate the following error:

Conversion failed when converting the nvarchar value 'test' to data type int.

I am thinking right now of params that won't go straight to a model and can be validated by model validations.

MrDanA
  • 11,489
  • 2
  • 36
  • 47
user2266813
  • 87
  • 2
  • 7
  • 2
    That's what model validations are for. – usha Sep 05 '13 at 14:51
  • 1
    What did you try and what do you expect? How did you "tamper the param"? This is too vague. Besides, Vimsha has a point: why don't you use model validations? – Mischa Sep 05 '13 at 14:54
  • @Vimsha A model validation won't work in this case. I believe the asker is saying, if they navigate to "/users/1/edit" that's fine, but if they navigate to "/users/test/edit" it throws that error, because the string "test" can't be converted to an integer in order to do `User.find_by_id`. This has nothing to do with model validations. – MrDanA Sep 05 '13 at 14:55
  • I am tampering the url by going to e.g. http://user/zzz instead of user/1 – user2266813 Sep 05 '13 at 14:57
  • @MrDanA I guess you're right, but this should be made clear in the question. We shouldn't have to guess what the problem is exactly. – Mischa Sep 05 '13 at 14:57
  • 1
    Please edit the question to make this clear, so I can undo my downvote. – Mischa Sep 05 '13 at 14:59
  • @Mischa Yes, the asker could have been more clear, but clarity is subjective. Only one parameter was shown, for `:id`, so it's not a bad assumption to think that's the parameter they are concerned about at the moment. – MrDanA Sep 05 '13 at 14:59
  • if case params[:id] = "12add" ? – tuấn bùi Jun 21 '21 at 08:05

1 Answers1

2

Yes you should always validate your parameters. People can always mess around with the parameters in their web browser's address bar, or modify parameters stored in the DOM. Another example where parameters can be screwed up is if the webpage is left open a long time. Imagine someone is viewing the page "/users/3/edit" and leaves it open for an hour, then hits refresh. In the mean time that user may have been deleted. You don't want your website to crash - it should handle that gracefully.

Depending on your database and adapter, doing User.find_by_id("test") will not crash. But your database/adapter was not able to convert the string in to an integer. One thing you can do in this particular case is use Ruby's .to_i method.

User.find_by_id(params[:id].to_i)

If params[:id] = "12", Ruby will convert that to the integer 12 and the code will run fine. If params[:id] = "test", Ruby will convert that to the integer 0, and you should never have a database record with an ID of 0.

You can also use regular expressions to test if a string is an integer.

But in general, yes, try to always validate your parameters so you can handle errors gracefully and control the data coming in.

Community
  • 1
  • 1
MrDanA
  • 11,489
  • 2
  • 36
  • 47
  • 1
    In these cases I think the app should generate a 404, because you're trying to go to a URL that doesn't exist. I guess the `to_i` approach is a good way to do that. Asker may want to change `find_by_id` to `find_by_id!`. This will generate a raise `ActiveRecord::RecordNotFound` exception, which (in production) will show a "404 Not Found" page. `find_by_id` will simply return `nil` instead of an exception, which may lead to other unexpected behavior. – Mischa Sep 05 '13 at 15:13
  • Depends on what the programmer wants, I guess. I like to catch as many errors as I can, redirect to an appropriate page, and show a helpful flash error message. – MrDanA Sep 05 '13 at 15:16