fgrehm / letter_opener_web

A web interface for browsing Ruby on Rails sent emails
MIT License
718 stars 112 forks source link

Added validation of params for letters_controller. #28

Closed kimrgrey closed 10 years ago

kimrgrey commented 10 years ago

When some gem like rack-mini-profiler sends additional queries on URL /letters/mini-profiler-resources/includes.js to measure speed of page loading, LettersController receives {"id"=>"mini-profiler-resources", "style"=>"includes"} and crashes with Internal Server Error. It is sad =(

So, as for me, controller should check that:

  1. Received :id is valid and letter with this :id exists
  2. Received :style is valid and acceptable

Queries with invalid parameters should be ignored with render :nothing => true.

kimrgrey commented 10 years ago

@fgrehm, any suggestions?

fgrehm commented 10 years ago

@kimrgrey sorry for the delay!

Thanks for the PR but I believe that the issue is actually on rack-mini-profiler itself since it is generating wrong <script> tags when an engine's page is accessed. Here's the related issues I found:

But you are right, letter_opener_web can actually do better and not throw up a 500 error but I believe it would be more appropriate to return a 404 instead of render nothing: true. WDYT?

kimrgrey commented 10 years ago

Yep, rack-mini-profiler is just an example of not very friendly behavior of letters_controller under poor conditions. Сompletely agree with your comments about status code. Added returning of 404 and corresponding test for this case into the PR.

fgrehm commented 10 years ago

Thanks!