Security is a broad topic which covers many aspects and is done at multiple levels. We have physical, virtual security which includes the code review process. We are considering security more than ever before because of how quickly we are adopting technology in our daily lives.
As developers, besides making our application secure we need to think about making changes to our codebase secure to avoid bugs, increase overall project awareness as well as make sure that unwanted changes do not get to end users.
We use code reviews and pull requests to cover this in our projects. However, just having this might not really be enough.
Similar to a door, it is not enough to close it, we may need to lock it as well. On the other hand, we have doors inside our house (project) that may not be locked, but we have an agreement as to when we can open them or what having them closed means. We can still barge in, but we do not do that.
The Need for a Process
Processes are particularly useful because we consider what we want to accomplish and take our time to think about how we are going to do this and what scenarios we may encounter. We end up with a checklist or a set of steps that we mostly only need to follow and at the very least partially understand.
This works great because we do not have to think each time about each scenario. It has all been covered and we only need to follow the process. This leaves us more time and brain power to implement that ticket.
The process of adding changes to our codebase is similar:
- we create a branch;
- we make some commits;
- we create a pull request starting the code review process;
- the codebase gets updated;
Why Code Review?
This is something we may take for granted; we do code reviews “because it is good”. We should ask a few questions when we get an answer like this. What exactly is good about it? Why should we keep doing it? Is it still beneficial? Are the other ways?
- Knowledge sharing & feedback, this is the first place where someone else looks at our code and can provide direct feedback on it. How easy it is to read and understand, propose or discuss alternatives and so on.
- Security, most of our applications rely on different packages and dependencies to function, seeing clearly if any were added, or if the features we implemented have security checks, is critical in preventing breaches before the change even gets to end users.
- Team cohesion, it is important that team members talk with one another, a code review is a perfect place where developers on the team can talk with one another about a work-specific thing and even poke ideas at each other. This is a difference between a team and a group of people that work together.
Common Anti-Patterns
Having a process is great, following it is even better. However, we may sometimes end up in specific situations where bypassing some steps is extremely tempting mostly because we can justify it.
Approval With Comments
This is probably the most common on a project. We usually trust our colleagues to make minor changes that we mention in a comment. Typos, moving some files around, the usual.
While this exposes a significant risk as someone ill-intended can abuse this trust, it is possible to make a mistake that is overlooked between minor fixes.
All code changes, regardless of what they are, should be reviewed.
Self-Approved Changes
Hopefully, we all approve of our work. However, when working with a team it helps to get a fresh pair of eyes to review our work. This is helpful as we sometimes get really focused on what we are working on and may start missing the forest for the trees.
Like the above scenario, if we can self-approve changes then we may not even bother with pull requests at all. The risk is greater in this case as there is no review whatsoever.
All code changes should be reviewed.
Late Night Fixes
This can be quite the situation. There is an ugly bug that users really do not like and really want to get rid of. Finding a solution proves to be a little bit trickier taking more time and we end up staying late while everyone left the office.
The hotfix is ready, but nobody is available to review it. We could unintentionally break the application in a different area. At the same time, there may be a better way to fix the problem.
Without having someone else to look over the changes and check if the bug is fixed, it is very risky to make a dire situation worse. Depending on the case, we may proceed, however it may be best to wait for the team.
Regardless, a review should happen in either case, even if we merged in the fix.
All code changes, regardless of when they were merged, should be reviewed.
Unavailable Developers for Review
A more general case: sometimes we have too much work on our plate and do not have enough time to do code reviews. Sometimes we can leave the pull requests pile a little bit, however, this may not always be possible.
We work with people we trust. So, if the changes do not immediately affect end users, then we may just go ahead and merge in the changes so the testers can check if everything still works.
Although this is not ideal, code review can be delayed and if a pull request was merged before it was reviewed, an additional pull request can address any found issues. This should be done before the sprint ends though.
All changes that get to end users should be reviewed.
Changes Outside of Version Control
This is a scenario mostly encountered with data changes. Sometimes we need to fix some data in the database or migrate information from one table to another. We typically do this with a SQL script which is not merged into version control because it is a one-time fix.
Pull requests are built on top of version control. So, without using it for this scenario we do not get the same benefits. The script still needs to be reviewed. This requires us to ensure that people know they looked over the exact same file.
We can address this by adding such scripts to source control, then they follow the same process as any code change. This will help us find previously run scripts, we may even reuse parts of them.
Alternatively, we can attach the scripts to the related tickets and have everyone involved in the review process leave comments to know that all checks have been met. This is riskier as we can simply upload a new version of the file, and nobody would know to review it.
All code that runs on an environment must be reviewed, even scripts that we run manually.
Securing Code Changes
Having the anti-patterns in mind, we can produce several rules that we should adhere to.
- Changes ending up on our main branches must always be reviewed.
- Prevent pushing changes directly on main branches.
- Make code review status explicit regardless of where or how it is made.
Following the above will reduce the risk of unwanted changes getting to end users. This will reduce the risk of exposing them to malicious applications.
Pull Request Restrictions
Pull requests can cover the first and third rule as we get this out of the box with most software development platforms, such as GitHub, Azure DevOps, BitBucket and so on.
Setting restrictions such as a minimum of one approver, other than the author, and resetting approvals when changes are added will ensure that pull requests are merged only if at least one other person has looked over the changes.
Branch Restrictions
Protecting main branches is essential. It is of little use to require approvals on pull requests if we can merge changes directly. We can easily bypass the entire code review process with this. Or we can even push changes on main branches by mistake.
Restricting the ability to make changes on main branches only through pull requests offers only one way to make them, further enforcing code review practices.
Configuration Restrictions
Ensure that only specific people may make changes to any of the above configurations. If everyone can lift pull requests or branch restrictions, then they bring little value in terms of protecting against ill-intended individuals.
We can still submit changes without code review. It only takes a few more clicks.
Process Adoption
The first step in solving a problem is becoming aware of it. Depending on the team, the above restrictions may reduce productivity or there may be small tweaks that need to be done so that developers do not get stuck in a process.
This is not something that needs to be done overnight. However, it is something that we should be thinking about long-term. This gives us time to go through a set of steps.
-
Process Evaluation
Periodically look over your code review process. See if anything needs to change. Check how team members follow the current process and so on. The key part of this step is to gather information and see what adjustments a process needs.
-
Formal Process
At first, no pull request or branch restrictions need to be added. Define a formal process while retaining general liberty on how changes are made to the codebase. Have people in the team follow this process to gather feedback, see what scenarios they encounter and update the formal process.
-
Process Enforcement
This is the last step, after working with a formal process and arriving at a satisfactory conclusion. The process can finally be enforced through restrictions forcing everyone to follow it. At this point, this should make a minor difference as everyone is already following the formal process.
This is only to ensure that what is formally agreed on is enforced. This prevents people from making accidental changes to the codebase as well as protecting against intruders.
Conclusions for Code Review Process
Security is a broad topic and is handled at many levels. Securing code changes is one step in increasing security by reducing the risk of malicious code ending up running for end users.
As with most things, processes are intended to make lives easier by being able to go through a checklist and to ensure that nothing was missed.
Being aware of the risks and defining a formal process are the first steps in securing code changes.
Enforcing processes through pull request and branch restrictions can be beneficial. However this depends on the platforms that we use, it may include licensing and additional costs.
It is critical to have a formal process, it is beneficial to have automated enforcement it to a degree.
This is an ongoing effort. There is no need to make it all in one go. Processes can be improved gradually through feedback and evaluation the same way we incrementally implement applications.
