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

Added validation of params for letters_controller. #28

Merged
merged 2 commits into from
Jul 17, 2014

Conversation

kimrgrey
Copy link
Contributor

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
Copy link
Contributor Author

@fgrehm, any suggestions?

@fgrehm
Copy link
Owner

fgrehm commented Jun 23, 2014

@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
Copy link
Contributor Author

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
Copy link
Owner

fgrehm commented Jul 17, 2014

Thanks!

fgrehm added a commit that referenced this pull request Jul 17, 2014
Added validation of params for letters_controller.
@fgrehm fgrehm merged commit db062e4 into fgrehm:master Jul 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants