Code review for security advisory coverage applications

I added this as a comment to a different thread but no one seems to have found it there (hopefully not to have not liked it ;)). I dare moving it into a standalone discussion as I would really like more opinions on it. Hope you don't mind.

Since my own (after all, praise!, successful) application is really not long ago, I personally see three main thoughts leading applicants to the queue. I would like to first explain them and then make some suggestions on possible consequences.

Hi,

I've spent several days working on my first module and have found the code standards review to be incredibly difficult.

First of all - let me say that I completely agree with strict code standards - and it's vital that new modules stick to these standards in all ways.

My module was written for D6.

As you guys may all be fully signed up module maintainers it might be an idea for me to outline the steps I've worked through recently so you get an idea of the problems for people publishing new modules.

Obviously installed coder and coder_tough_love.

As klausis pareview script is very helpful to detect typicall project application problems I tried to set up an online service to make it easier for applicants to use without setting up a testing environment.

It was a all-in-one-day project, written quick and dirty - but It's working and maybe it'll also be useful ;-)

It can be found here: http://ventral.org/pareview

Currently the review process faces an issue that turns out really annoying.

TL; DR: The code checking scripts (pareview and drupalcs, which are really great besides!), complain when a file ends in \n but give green lights on \n\n. The docs read different. Currently this is more and more leading to disputes.

Can we solve this asap? Be it either that the documents are corrected or the review tools?

Related links collection:

<

ul>

Klausi's post in the Community Spotlight.

I think the project application process is hugely important to our community and even though there are efforts under way to automate much of it, it's clear that we will still need some human involvement for a good amount of time to come. I see community spotlight posts as a great way to help thank the folks who are doing this work and to help draw attention to the need for more helpers.

First, let me say that Features is great and that I think it is totally ok to have pure Features projects on drupal.org that somebody just clicked together. Yay for sharing!

I feel like there are people who feel comfortable doing the regular review, but not the security review portion of a typical review. So, I'd like to share with some folks how I tend to do that in one-on-one sessions. I hope that by doing these one-on-one it will provide more confidence to those folks than a blog post or screencasts or whatever might do.

Please update this wiki for things that are being worked on and any associated issues.

• Coder review of sandbox projects
• A tool to do status changes on issues. We need it to do "in needs work status for 4 weeks and no response, move to something else like won't fix and post a really nice message" but this seems like a nice tool to have for project_issue.module in general
• General coder cleanup/enhancements

Quicklinks

<

ul>

This came up in some discussion, and I want to know what others think. Comments have been made that a lot of the problems the security team encounters are with newer modules/maintainers. If this is true, I would like to get more feedback on the reviews being done. I can only speak for myself, but I've never received feedback of any kind. If I missed something important, or the group as a whole keeps missing the same things, I think it would be helpful to know this. It would make it much easier to create documentation with specific examples of things to look for.

We spend most of our discussion here talking about what isn't working, so I thought it would be nice to point out something awesome: we are currently down to only 100 issues in the "needs review" queue. That's a number we can get through in the next week, and then have no backlog for the very first time ever since sandbox reviews started. Reviews should be much, much smoother after that. Yay!

PROPOSAL SUMMARY:

1. Initial Git access agreement and sandbox creation stays status quo.

2. Develop automated testing capabilities for everything from coder to xss screening to scanning the repository to ensure it contains Drupal-like code.

3. Run ALL modules through automated testing before allowing promotion to full projects

4. If a user does not have 'create full project' permissions, then have them submit a 'Project Submission' to an issue queue.

5. The issue queue is intended as a 'sanity check' only ... not a full technical review. The main issues should be addressed through automated testing. The main purpose is:

   • keep tabs on how well the automated tests are working,

   • provide an alternative path for modules that "can't" pass the automated tests (with valid reasons), and

   • provide corrective direction and/or suggestions for items which can't be caught via automation.

6. Any issue in this queue which sits for two weeks in Active or CNR state, without new comments, automatically gets bumped to RTBC.

7. Applicants which clear the queue get access to a 'promote' link for that project.

8. Create full project' rights allow an individual the option of promoting any project which passes the automated testing directly, without applying via the queue (though they can still use the queue, if they so choose).

Optional discussion:

1. Tie granting of 'create full project' rights to a competency demonstration (via a challenge quiz).

With the backlog being what it is, is it a good idea to reply to any new contributions? Even if you haven't reviewed their code yet. I've noticed from the few pieces I've looked at that there are a lot of basic errors that really slow down the whole process. So a standard reply asking for patience, if the opening post isn't strictly as procedure is a tiny bit about how it won't stop your application from being handled but will slow the whole thing down yada yada.

Something like this:

Right now, the project application process only spot-checks security and API problems that happen to be in the specific lines of code of a specific project. A contributor can walk away with a "swiss cheese" understanding of Drupal best practices.

One way to combat this is to set up a "quiz" of some kind in front of granting people access. This idea has sign-off from the security team. I've created an issue for this over here: http://drupal.org/node/1290624

I know what it feels like to have your hard work critiqued, piles of issues to solve and to deal with some of the hardest discussions and posts we can have. I know the many hours spent, sacrificing time with our loved ones, time without sleep or the so many other things we give up to do our part.

In some irc discussions today heyrocker asked who was active in the project application review queue these days.

This query counts unique comments, so it kind of rewards people for posting a lot of comments and not necessarily just good comments, nor full reviews.

It does give us a sense of who is generally active in the queue.

Last 30 days

Interesting debate going on in http://drupal.org/node/1162758 ...

The applicant has submitted a module which requires a core edit ... the core edit basically adds a new hook to core, which the module then takes advantage to implement its functionality.

In discussion, the applicant makes the following point:

So, why don't allow people to make a choice themselves? Publish the module and we will see whether it is convenient to add 1 line of code to the core or not.
There are some modules out there that required lot more of core modifications.

This is intended as the 'Coles Notes' version of a four-part post. The rest of the proposal can be found at the following links:

• Part 1: Today's Project Application Process
• Part 2: Where the Existing Process Breaks
• Part 3: The Proposal
• Part 4: Sanity Check - Wait a Minute Here ...

This is part one of a four-part post. The rest of the proposal can be found at the following links:

• Part 1: Today's Project Application Process
• Part 2: Where the Existing Process Breaks
• Part 3: The Proposal
• Part 4: Sanity Check - Wait a Minute Here ...

The 'Coles Notes' summary of the actual proposal can be viewed HERE.

Or, if you prefer, download a copy of the entire proposal in Word format.**

This is part one of a four-part post. The rest of the proposal can be found at the following links:

• Part 1: Today's Project Application Process
• Part 2: Where the Existing Process Breaks
• Part 3: The Proposal
• Part 4: Sanity Check - Wait a Minute Here ...

The 'Coles Notes' summary of the actual proposal can be viewed HERE.

Or, if you prefer, download a copy of the entire proposal in Word format.**