Secure your code review: 8 key questions to ask

Whether you’re reviewing a team member’s pull request, pairing, or even reviewing your own code before deploying, code review provides a moment to step back from the code and ask yourself the important questions.

Does this do what it’s supposed to? Did I miss any bugs? Will the code be easy to work with next time? But do you ask any questions about the security of the code you’re looking at? Most of us still don’t know how to tell if code is secure.

Here are eight security questions to ask yourself the next time you’re reading or reviewing code.

1. Where’s the input going?

The number one trick hackers use to exploit an application is to trick it into treating user input as code. Common examples of this are SQL Injection and Cross-site Scripting attacks. When we review code, we should always pay attention to how any new input in the system is treated.

The first thing to check is whether the input’s being used in code that will be evaluated, like in a regular expression or a database query? If it is, it should be properly sanitized.

The next thing is to think about what the data will be used for later. Is it being stored in a database? Is it being sent off to another service? Will it eventually be displayed to a user? You either need to know it will be sanitized or escaped when it’s used, or you’ll need to sanitize it on the way in.

The systems many of us work on today are highly distributed. This introduces the problem that you never quite know where data will end up or what it’ll be used for. It’s likely your API will expose potentially compromised data to other microservices, your data lake, or even third parties. Keep this in mind when considering which measures are enough to consider your code secure.

2. Are the right AAA checks in place?

Every interaction with your system should check the following boxes:

• Authentication – Does the code check that the request really is coming from the person or system it claims to be coming from? E.g. does the user have a valid JWT?

• Authorization – Does the code check the user is allowed to perform the action in question? E.g. does the user have the admin role?

• Accounting – Does the code record who did what, so that you can check back later in case there is an issue?

Always keep an eye out for AAA slip-ups when reviewing code—they’re one of the easiest mistakes to make—and make sure there are some automated tests for any new AAA code to prevent it being accidentally removed in future.

3. Are the assets changing in a meaningful way?

It’s good to have a plan for how to protect your data, but what happens when the kind of data you hold changes? When reviewing code, be on the look-out for any changes that affect the overall security profile of the application. Sometimes a seemingly small change can actually increase a company's legal liability for security issues and would require additional processes and practices to be implemented in order to keep users' data safe.

For example, if the code you’re reviewing introduces a new field regarding a user’s personal health, this would be classed as Special Category data by European law, so you’d need to have a good reason to store it and will need to be extra careful with your data in future if you do decide to store it.

4. Are you leaking data?

It’s very easy to accidentally return too much information in an API response. Let’s say you have a user object and your website allows users to browse other user’s profiles to view some public info. An easy mistake to make would be to make an API call for the user whose profile is being viewed, and return the whole object from the database. This might leak sensitive information such as a phone number or email address. When reviewing new code that returns data, check to see whether there’s an allow-list in place that ensures that only the specified fields are included in the response.

For logging, ensure that any new logging code is only including the information required to find and diagnose issues. Pay particular attention to making sure that no personal information such as email address is being logged. Remember that you can mask or truncate information too.

5. Are new 3rd party dependencies okay?

Does the code you’re reviewing introduce any new third-party code? If it does, it’s a good idea to do a little due diligence to see if the dependency is reliable and secure. Is it widely used? Well maintained? Remember to ask if there’s a simple way to achieve the task in question without adding a new dependency. Snyk’s Advisor tool is a handy way to check key stats for a new package you’re considering.

6. Have you checked the borders?

Security issues exist at the edges of our systems. Where user input arrives, where systems talk to a database, where they call an external service, or place messages on a queue. These are the key files to survey when reviewing code for security issues.

If I only had time to check two files in a new pull request for a modern web application I would look at the controller (or wherever the incoming request was dealt with) and the database access logic.

7. Has the config changed?

Security breaches are often the result of misconfiguration. I often see news stories about data being leaked because someone forgot to turn security on for their AWS S3 bucket, for example.

Config changes are usually small and often overlooked, but make sure that they make sense. Of course the biggest step to take is to manage your Infrastructure as Code. If your config is done manually there won’t be any code to review and very little chance of security issues being picked up.

8. Is anything being cached?

Few things are quite as embarrassing as showing one user’s sensitive information to another. Modern applications often have to operate at scale, and therefore caching becomes an important part of ensuring your system remains performant.

But there are some bits that should be cached and some bits that shouldn’t. Importantly, there are some bits that should be cached per user (using a cache key), so that you don’t end up putting one user’s bank balance on everyone else’s profile. Have a good understanding of your system’s default caching behavior and ensure that new endpoints will work with it in the right way.

Train your eyes for security

People who “have a good eye for software design” are often great at reviewing code, because they ask the right fundamental questions about the code in question. Now that we all recognize the importance of secure coding, it’s time to train yourself to have a good eye for security too.

Keep this list of question handy the next time you’re reviewing code and soon you’ll be known in your team as the person most likely to spot security issues before they’re shipped.