Checklist for a Rails Application Code Audit

Our reinteractive developers have delivered over 20 complete application code audits through our Inspect service. As part of each audit we go through a checklist to ensure we cover every important aspect of an application.

Through these 20 audits we see many different levels of code quality, it is quite interesting that during the Inspect service we get to see locations where developers have obviously been under a time crunch and had to take a shortcut and also developers that have used design and coding patterns from other programming languages. We also have seen a lot of common situations that could be improved.

Because of this, I thought we would write up a checklist of things we look for when we do one of our Inspect services. Hopefully, this can be used by experienced developers as a hit list of areas they can look into in their application themselves.

Look at the good!

In doing code reviews, we realise we are looking at other peoples hard work and effort. Because of this, we are conscious to find out any good points and excellent examples of development. These deserve as much attention as any faults we find because it is important to provide a balanced report on the state of the codebase. The objective is to support and educate the existing development team - focusing solely on the negative is playing the blame game and doesn’t achieve anything constructive.

Security

As part of the process we really take a deep dive into security. In a code review, this really needs to be one of the biggest points you review in any application. All the pretty code in the world is useless if someone can do a simple SQL injection to crash your app or destroy your data.

Important things to look at for security are:

• Rails version, ensuring this is at the highest patch level,
• Gem versions, looking for outdated gems and getting these updated,
• Mass assignment issues, though this is becoming less of an issue with Rails 4 defaults,
• Password encryption methods, ensuring they (1) exist and (2) are strong enough,
• Third party service credentials, how are they handled in production, staging and development,
• Session security, cookie encryption, cross site scripting and other vulnerabilities,
• Privilege escalation, is it possible through not using user scoped ActiveRecord finders and other methods to bypass the application layer security,
• SQL injection issues, usually this is from trusting user data when it shouldn’t be,
• Other attack vectors from not properly sanitising or incorrectly trusting user data,
• Ensuring NO PASSWORDS are stored in the source code, or committed in any configuration file. Too many times we find this.

If you are doing this yourself and are not sure what to look for, utilising a tool like Brakeman can help you get important issues highlighted.

We also have an internal tool that verifies all gems being used in the application to confirm any outstanding security issues and we pass on this data.

Ruby and Gem Versions

We also take time to inspect the production version of Ruby being used. The most current version of Ruby will give the application significant performance boost, but it is not always possible to upgrade. If this is the case, making sure you are on the highest available patch version of the Ruby you are using is an important security concern.

Upgrading gems also might not be a straight forward issue, but usually getting up to the most recent ‘tiny’ version of the gem is possible and will usually result in performance and security improvements. A critical one here is that it is surprising how many Rails applications are not running on the most recent tiny version of Ruby on Rails. These tiny upgrades are usually quite straight forward and simple to do, but deliver incredibly important security updates for your application. An out of date Rails gem is a red flag to our inspectors.

Database Concerns

Another often overlooked area of a Ruby on Rails application is the database. I can’t understand why this is and can only think that this is because some Rails developers believe that writing optimised SQL or optimising the database is not important. It is. Rails applications live in the database and getting the database aspect of your app right is critically important. Some of the things we check in the database are:

• Using the most recent version of the database you are using, or at least the most recent patch version
• Checking for missing indices where they should be
• Checking for superfluous indices where they shouldn’t be
• Looking for possible errors due to Rails app race conditions caused by missing unique constraints on database tables
• Looking at data normalisation and spotting performance opportunities from refactoring
• Finding non “Rails way” coding with associations and tables
• Spotting areas where more complex database tools can be used, like stored procedures or complex indices to take the load off the Ruby code
• Finding areas where ruby objects are being instantiated unnecessarily
• Locating slow functions caused by inefficient use of generated AREL queries where more concise SQL could produce much faster results.

It should be noted that on all of these SQL versus Ruby points, we always want to review the actual performance data from real measurements because there is always the risk of premature optimisation. But when we do find something, it can result in a massive change. One of our clients had us review some SQL reporting tools, and we managed to get the report to run within 800ms, instead of the 25 seconds it was taking to do it in Ruby. Quite an improvement!

Application Setup & Documentation

Getting a Ruby on Rails application up and running can sometimes be a challenge. Improvements over the years through Bundler and best practices have definitely made it easier, but there are still many applications that are almost impossible to get running without some insider knowledge. Making your application easy to boot up for a new developer takes some work, but it will pay you back many times over the first time you need to get a contractor or new team up and running on your application.

Some of the things we look for in Application Setup are:

• An up to date readme is critical. We recommend our clients use our standard reInteractive Rails application README. This is a template that you fill out, highlighting all the major points a new developer needs to know to get your application up and running.
• A working db/seeds.rb file that actually loads the application up with data making it possible to run with a simple rake db:setup. Too many times we have been unable to start an application because the application is expecting certain data to be in the database.
• Required YAML or settings files included as examples; this means any configuration file that your application requires to start, should be in the right place renamed with .example. in the name, for example config/database.example.yml. This file should contain example settings that WORK for development and test mode. Keeping the correct extension on the end means that syntax highlighting will work when you edit them.
• Maintaining a Procfile with Foreman can greatly reduce the amount of time needed for a new developer to get up to speed.
• Maintaining a “happy path” walkthrough and smoke test in the README. This helps a new developer understand how the app should work.

If you want to test if this step is complete, it is simple, simply check out a clean copy of your app, follow your readme on how to setup the app and then run rake at the command line. If your tests do not run flawlessly, you have more work to do. If the tests run, boot the app exactly per the readme and if you can’t use it normally, you have more work to do.

Assets & Asset Pipeline

We review many aspects of the assets, which includes the Javascript, CSS, Images, Fonts and delivery through the asset pipeline to make sure the application is working well. Some of the things to look for here: * Javascript is written in a modular manner and follows Javascript best practices, a good reference for this is Douglous Crockford’s book Javascript: the Good Parts. * Javascript is namespaced well to avoid conflicts, * Javascript is reviewed for possible security issues, * Review of performance as needed on the Javascript code, * Images are shrunk and are as small as possible with meta data stripped using tools such as ImageOptim The above is not a complete list, it is a good idea for you to have a read of my tips on the asset pipeline for more areas you can look at here.

Controllers, Models and Views

I’ve put these into one area (though in our Inspect Rails code review service we spend many pages usually reviewing these) because there are many resources on the web on how to improve these.

There is the whole fat model, skinny controller mantra which has been abused by many Rails apps we have inspected. These days, using concerns, presenters, decorators are all useful tools to reduce complexity and should be investigated by the team.

As a quick note, if you are using any ActiveRecord callback on the model (such as before_save, after_validation or Observers etc), we consider this a code smell and work should be done to remove them.

Probably the most common thing we see is methods that are way too long or violate the single responsibility philosophy. Getting these two things right in your code will get you a long way to having a stable and maintainable Rails application.

A good tool you can use here are our friends at Code Climate, no automated tool ever will be perfect, but Code Climate do a good job at alerting you to situations in your application that you can jump on early.

External Services

One fairly specialised, but important point is the extraction of any interface to an external service. If your app makes use of Redis say, then you should avoid referring to the Redis gem anywhere in your code except for a single WorkerInterface class or the like. In this class, program in a shallow interface to Redis and then code the rest of your app against the WorkerInterface object. This way, if you ever need to replace Redis with something else, the impact to your whole application is minimal.

Tests / Specs

While this is at the bottom of the list, really, it is probably the most important one to have solidly in place in your application to ensure future maintainability and upgradability.

There are so many things you can check with your testing framework, but we focus on:

• Completeness of the test suite, using tools like simplecov while not perfect it will give you an idea of how well covered your application is and give you a measurable statistic you can work against,
• Continuous Integration server, if we see no details of a CI server in the Readme or code, then this is a concern and we always recommend one gets installed, unless the application is tiny and the testing suite runs in mere seconds, for any large application, a CI is critical,
• Speed of the test suite is also important as the faster it runs, the more likely it is going to be run!
• Review the integration of external tools and don’t test code you didn’t write
• Avoid running other tools like Redis or Memcache in tests, these should be stubbed out to keep the setup complexity of running the test low and the speed high,
• Use of clear integration tests to check the happy path of your application, try to avoid testing all edge cases in these as they are very slow to run and cumbersome to maintain
• Ensure all external requests to APIs or web services are stubbed out with tools like VCR
• Remove unrequired database calls by using something like Machinist or FactoryGirl to create in memory objects.
• Follow sites such as Better Specs and similar test unit related sites to improve your spec writing

User Experience and Design

In our code review service, we don’t deep dive into these areas, we do of course offer a UX Inspect service, but these are different and subject to different checklists and blog posts :)

Performance Analysis

“Performance” is a big subject, and another area we don’t delve too deeply into in our standard code audit. We do find and highlight internal issues that are obvious (such as with database queries and the like) however doing a performance analysis takes different tools and a different approach from a code audit perspective.

Summary

The above is a rough guide to what we review when doing a Ruby on Rails Application code review through our Inspect Service. Obviously, each of the above points requires a deep knowledge of Ruby and Ruby on Rails to get the best advice, so if you need help with your application, or want us to cast our eyes over the work you have done, please get in touch and book in an Inspect with us.