A guest post by Adam Baldwin, the Chief Security officer at &yet and the Team Lead at ^Lift Security, where they help teams build more secure Node.js applications through assessments, consulting and education. Adam can be reached at @adam_baldwin.
As I wrote in my previous article on building secure Node.js applications, there are many things you can do to write Node apps that are more secure.
One of those suggestions is to review code manually as commits are merged into projects you are involved with.
Peer reviews are often suggested and often received with little hesitation as part of a development teams code quality process. Done properly, peer reviews can ensure project integrity, help educate developers and bring your development team closer together.
If you are already using peer reviews in your process, I hope to provide suggestions for you to improve your existing process. If you aren’t, hopefully you will start and this post gives you some insight into peer review as we have done it at &yet.
Why do Peer Review at all?
Peer reviews provide a way to exchange tribal knowledge between developers. If done properly it provides a way to learn from the alternative perspectives and experiences around you.
Sure, you can document expectations and policy around code, but from my experience those documents rarely get read and even if they are read and understood you need a way to enforce those guidelines.
+1 is worthless
When we first started to implement peer review into our dev process at &yet we went with a pretty standard way of giving approval on a pull request, comment, and so on by putting in a +1. We required approval by at least 2 other developers.
We quickly noticed that one developer giving another developer a +1 was pretty worthless. Why? It was completely without context.
Without providing a breakdown of what you actually reviewed, there is no way to know if what is expected of a peer review was actually performed.
Some of the things I’ve seen developers review code for are:
- The code actually does what it says it’s supposed to do
- Adherence to style and code that is cleanly readable
- Code that actually has appropriate tests for the functionality implemented
- Code that doesn’t contain any obvious business logic flaws
- Changes weren’t committed that would effect deployment process
- A changelog that has been updated
So when you give your approval for a peer review that “looks good,” make sure and provide some context as to what you actually did. It’s a great historical reference, provides useful feedback to the submitter and backs up up your +1.
Who should do the peer review?
Technically, any other team member should be able to give some perspective on your commits, however, the people that are the most effective are the developers that are building similar features or have experience building similar features and developers of the systems that you interact with.
Be careful not to discount the perspective of less experienced developers. Sometimes the lack of experience or ignorance allows them to ask the best questions and challenge the norm if empowered.
Be respectful, but don’t be afraid to give thorough feedback
Being at ^lift and at &yet plants me firmly between two communities: developers and security. Over the years of doing hundreds of security assessments I’ve learned that giving honest feedback without completely destroying the other persons motivation can be difficult.
You might even start feeling uncomfortable with the level of granularity in which you deliver your feedback, fearing that you might hurt other people’s feelings. Being pedantic about your team’s code, however, proves to be a good way to keep everyone on their feet, and keep the code style, quality and organization stable, as the project grows.
I like to remember that:
- This may be code I’m reviewing but there is another human on the other end of the pull request. Yes, humans have feelings and you should care about them.
- There are probably things they did well in the pull request that I could call out, which provides positive reinforcement.
Automate most of what you do
One important thing to remember about peer reviews is what a peer reviewer should not be doing. This is a simple rule and following it goes something like this. Don’t do work that a computer could be doing and can do easily.
As a reviewer I want to be confident that the following happen without any interaction from me as the reviewer. They all can be implemented as part of the commit process or part of a continuous integration (CI) process. I recommend checking out precommit-hook and travis-ci.
- Tests pass
- Code lints to project standards
- No known security issues exist in dependencies
If there is anything you to come away with from this post, it’s the fact that if you are going to do a peer review, you owe it to the committer to do a thorough review, provide thoughtful feedback and work to improve the areas that can be automated.
- Do peer reviews
- Automate what you can via precommit-hook, and continuous integration systems.
- Provide thoughtful, respectful and complete feedback.
- +1’s are worthless without context.
For more information, checkout our Node.js books that cover security in Safari Books Online.
Not a subscriber? Sign up for a free trial.
Safari Books Online has the content you need
|Mastering Node.js will take you deep into this exciting development environment. Beginning with a comprehensive breakdown of its innovative non-blocking evented design, Node’s structure is explained in detail, laying out how its blazingly fast I/O performance simplifies the creation of fast servers, scalable architectures, and responsive web applications.|
|Node Security will start by helping you delve into the building blocks that make up typical Node applications. By understanding all of the layers that you are building on top of, you can write code defensively and securely. In doing so, you will be able to protect your user’s data and your infrastructure, while still using the rock-star technology behind Node.js.|
|Node.js in Action is an example-driven tutorial that starts at square one and guides you through all the features, techniques, and concepts you’ll need to build production-quality Node applications. You’ll start by learning how to set up your Node development environment, including loading the community-created extensions. Next, you’ll run several simple demonstration programs where you’ll learn the basics of a few common types of Node applications. Then you’ll dive into asynchronous programming, a model Node leverages to lessen application bottlenecks.|