Review Process - Basic Steps

There's already documentation at http://drupal.org/node/894256

Is this mean to replace that?

In zzolo's post, which you can read here, he talked about formalizing the review process. This is meant as a first draft documenting the basic steps of the process, with some specific things that should be done. I probably need to give a little thought to the desired outcomes.

It looks like I have to answer this myself, but i feel somewhat qualified, based on my experience as an auditor. There are a few basic principals that apply:

• Identify where things can go wrong
• Are there controls in place?
• Are the controls being used?

In auditing, there are two types of controls,

1. Prevent controls are controls that prevent errors from occurring.
2. Detect controls are controls that detect errors that have occurred so that they can be corrected.

In auditing, if you can establish that good controls are in place and are being followed, you can cut down on the amount of time that you spend checking for things that may have fallen through the cracks. Unfortunately, in website security, once a breach occurs, it's too late, so we have to rely exclusively on prevent controls. Fortunately Drupal has done a good job of identifying where the problems can occur and putting controls in place. The code review itself is a form of prevent control, to make sure that procedures are being followed.

It's fairly easy to set up mechanical checks for common items that follow a predictable pattern. But you can not rely completely on mechanical means. You need to be aware of where problems can occur. Where is the project getting data from user input and where is it being displayed? What other types of things can go wrong? It's that type of awareness that will allow you to catch things that the mechanical checks won't catch.

I didn't know that you are a security guy. I'll have to read your book.

From the perspective of doing code reviews, Writing Secure Code is the main focus, and trying to come up with some basic procedures for checking compliance with controls. I noticed that for Drupal 7, Coder is doing more security reviews. I'm a little leery about relying on Coder too much without knowing what's going on under the hood. I've been doing things like using grep to find all sql queries, all form title and description attributes, etc. I'm a little concerned about losing the human factor by relying too much on software. I noticed that you are using Coder in your own security reviews. I'd be interested in your opinion of how effective it is, what it does and does not do.

From the perspective of a git administrator, I would be worried about people saying that they did a security review when they really didn't do much. In a formal audit situation, you would be required to document every procedure that you did and the results. That's a little too much to ask of a volunteer, and I would be interested in hearing from administrators what kinds of things they like to see that gives them some confidence that the security review was done in a reasonably competent way.

What I'm trying to come up with is some reasonably efficient techniques or procedures that can be used to conduct a review. Ideally a procedure would do two things:

1. Provide an objective means for the reviewer to make a decision, and
2. Provide some evidence to the git administrator that the reviewer did a good review.

I'm still not sure exactly what Coder does and does not do, but it looks like that's the best tool available for Drupal 7.

I was re-reading the GNU GPLv2 license text, and noticed this under the heading of how to apply the terms of the license to your program.

If you develop a new program, and you want it to be of the greatest possible use to the public, the best way to achieve this is to make it free software which everyone can redistribute and change under these terms.

To do so, attach the following notices to the program. It is safest to attach them to the start of each source file to most effectively convey the exclusion of warranty; and each file should have at least the "copyright" line and a pointer to where the full notice is found.

one line to give the program's name and an idea of what it does.
Copyright (C) yyyy name of author

Shouldn't we be advising project owners to put copyright notices in their source files?

The copyright notice is in a file called COPYRIGHT.txt in the Drupal root directory. It reads in part:

''All Drupal code is Copyright 2001 - 2010 by the original authors.''

I'm guessing that "All Drupal Code" covers contributed modules. It's a small detail, but people who are trying to get away with something often do it by exploiting small details. I think that there is really no harm in posting a copyright notice in the source file. It identifies you as the original author.