Rails controllers can get out of hand if you’re not very careful. Skinny Controller Fat Model is a great start. But what about handling errors? Isn’t it enough to just let Rails catch your exception and show a 500 Server Error page?
No, it’s not. Falling back on 500 Server Error for everything outside of the “happy path” through your code is sloppy coding.
I say this for three reasons:
The 5xx response codes are for server-side errors.
If the code is working properly but the request is wrong (no such document, an invalid request, or insufficient user privileges for the requested operation), the 4xx response codes are more appropriate. You should only return a 5xx series error message when your application is broken.
A generic “oops” page hides the cause of the error from the user.
Maybe the user followed a bad link, that goes to a document that has been deleted. They should see a 404 Not Found response, not a 500 Server Error. This is especially important if the user agent is a web service client actively under development. And remember that you can still customize the response body, so a 404 can show a search or site map to help them find the content that was Not Found, and a 403 might suggest that the user upgrade their subscription so they can see the Forbidden content.
Failing to trap bogus input is a recipe for a security vulnerability.
Rails will detect ridiculous input in many cases, but it doesn’t know about your business rules.
For example, what if someone fires up Firebug and figures out your API, and deletes her ex-boyfriend’s account because you forgot to check that the ID being deleted matches the currently logged in user? Oops.
You know who else got pwn3d because they put their security rules in the client? AOL. Don’t be like AOL. They got pwn3d big time. Make sure your controller handles the “unhappy path” too, and responds appropriately.
You make sure your controller handles these cases by writing tests, and checking that the response is correct. To do this, you need unambiguous errors that clearly explain what the controller didn’t like about the request.
Your controller tests should never expect an exception. Exceptions belong inside the application (such as between a model and a controller), not at the boundary.
Instead, you should be using something like assert_response :bad_request, and providing a useful textual message in the page that will help the user understand what they did wrong.
A nice bonus of using the right HTTP error responses is that your tests are more readable. And since controller tests tend to be really verbose already (mine tend to be ~10x as many LOC as the controller itself), the extra readability is really nice.
Great post! Especially the part about “you forgot to check that the ID being deleted matches the currently logged in user” …
Actually, Rails really needs better secure-by-default configuration. Stuff like attr_accessible is a step in the right direction, but … considering Rails is already a heavy full-stack framework, I’d make a standard auth mechanism part of the stack unless the developer specifically disables it. Having a standard baked in user/role/access system would make it easy-ish to have the meta-programming-based code (e.g. joins) perform checks on all data access, etc.
I’d even go so far as to make the production config automatically use https for urls, and add a utility to generate and handle certs for testing.
I disagree that it belongs in the framework, but only because of Rails application templates. In other words, you probably shouldn’t be starting with a generic Rails app anymore, unless your app is super weird and doesn’t resemble any of the available templates.
There’s still the issue of retooling: at what point does it make sense to reimplement your app on top of a template that uses stuff that didn’t exist when you started. So how do we retroactively beef up the security of some Rails app that’s 18+ months old? Maybe your argument about it belonging in the framework holds water after all. :)