Summary: The security team proposes that the first release of a project undergoes review.

Credits to Jakub Suchy for the initial idea.

Background

A lot of contrib quality and security issues stem from new maintainers not being familiar with Drupal APIs and their peculiarities. Think drupal_set_title($user_supplied_variable).

At the moment projects already undergo review; AjK reviews code samples before granting new maintainers CVS accounts. The drawbacks of this procedure:

  • A tremendous amount of thankless work has to be done by one person (AjK).
  • CVS account holders create other projects that are not reviewed.
  • Reviews are not public, no learning opportunity for others.

Proposal

We propose that the first release of a project is reviewed for blunders.

Before I continue let me state that:

  • We are not seeking Drupal Tough Love (tm) style reviews or reviews that deal with code style issues. Unless the code style issues make the module effectively unreadable.
  • We are also not proposing a "Hacker Safe" or "Goldplated" logo or endorsement of any kind. The only result of the review is that the release is posted.
  • We don't want to add too many roadblocks for collaboration.

Proposal (functionality)

Once a CVS account is granted, maintainers can create new projects, commit code, assign or be assigned cvs access, branch, create dev, alpha and beta releases, and use the issue tracker.

Creating releases for new projects

The first real release of a project will remain unpublished (or at least not appearing on project pages / update status) with a message to the maintainer.

A discussion node with a link to the tarball and comments enabled is placed on a queue.

Creating releases for existing projects as a new maintainer

When a project is transferred to a new maintainer, the next (non dev, alpha or beta) release will also require review.

Review

Users can review the queue and add comments. Users with the appropriate permission can then publish the release. This also marks the project in some way, so that subsequent releases are published without reviews.

In addition, users with the appropriate permission can set a status for CVS account holders that bypasses the need for this first release review.

We may need to setup a dedicated review team.

Feedback

As such a change affects quite a large part of the community, we need to hear your feedback on this. Please try to remain on topic.

Comments

mradcliffe’s picture

  • What sort of quality control issues are you worried about? Is the qc problem to reduce project issue queues?
  • Can anyone get around this by not publishing releases or tagging in such a way to bypass the system? Although I am not sure why someone would do this.
  • How big of a team is this going to require? It seems like you need either a super-developer who knows everything or a variety of reviewers who are knowledgeable more in some areas than others. Also, you don't want to stress out this position like the cvs application reviewer(s?) seem to be (?).

Time to enable flag on d.o, eh?

WorldFallz’s picture

I like the idea in principal but I'm a little concerned with implementation. Would module duplication be one of the review factors? While I agree with the philosophy that 5 modules that all address the same functionality is a bad thing, I do think some competition is good. For example, I think ubercart and ecommerce pushing each other is a good thing. Choosing between 2 options is a much different thing than trying to choose between 5 options.

Also, would the review criteria address things like modules being named http://drupal.org/project/nodeaccess and http://drupal.org/project/node_access?

Or are you just planning to address strictly code and security items?

Heine’s picture

Only code and security items; drupal_set_title($node->title), using mysql_query, using db_query without placeholders, using %s without quotes etc.

Michelle’s picture

I just wonder if there's enough manpower to do this. The security team is overworked as it is. ajk is frequently quite behind in the reviews. I don't fault him for this in the slightest, but it is a fact that people end up waiting sometimes for quite a while to get access because they're watiing for a review. I want to help him but, frankly, am intimidated by trying to do a security review myself when it's not my area of expertise.

My worry here is that we'll have folks who have worked hard to get to their 1.0 release, are all excited, ready to show it off to the world, and then end up waiting 2+ weeks before it's published. And what happens to the last dev snapshot before that moment? If that's not unpublished, too, then there could be a lot of people already using the module anyway.

So it's interesting... Just not sure that it's practical.

Michelle

andb’s picture

Michelle comments on what seems to be the main issue with this very good idea - is there enough manpower?

How many new modules are contributed and how many modules have new maintainers in the average month? How long does it take to review a module, and be honest about time to go to the queues, read any discussion issues from colleagues (as per Heine's document of opening a discussion for each module to be reviewed), review code and write comments about any found flaws? You can quickly calculate to see if the manpower is available to do this.

The idea has merit, especially the idea about capturing feedback from issues found so that other new Drupal developers don't fall into making the same mistakes.

Given that resources are limited it seems that perhaps a certain triage is needed here: first test reported security errors, secondly test modules from 1st time contributors and 1st project maintainers, thirdly publish automatically and test modules from people who have contributed before as time allows.

Another possibility would be to create a "catch the security bugs" game available to all d.o registered users, in order to expand the manpower of the security team without incurring the penalties involved with the bureaucracies of organising a larger group. However, keep in mind that there are a finite amount of Drupal developers, would this effort take resources away from such things as patch testing for upcoming releases?

In the end, Drupal's strength is the huge amount of human ingenuity that comes to the project though contrib. To set up a gatekeeper for that which would impose long delays would only hurt the project. Something as small as a 5 working day delay I feel could reduce the number of people who want to go through the effort to give back to the community. I suggest setting up a test to see what can really be handled and what kind of delay would be incurred.

David_Rothstein’s picture

Looking at Heine's list, I am wondering if some of these basic kinds of checks can be automated? (In fact, I believe that the Coder module does some basic security scanning already.)

Trying to automate it would clearly lead to some false positives, but that would not be so bad because the automated test would not have to be the final word; anything that it flagged could then go on for human review. This would reduce the amount of manual work required, since human reviewers could focus on things that were flagged rather than on every line of code in every module.

Also, an automated review would clearly not catch every security issue (or even close to it!), but if we're only talking about a quick visual scan anyway then it likely would be better than most humans would do.

See also #302643: run coder on patches which might be related.

catch’s picture

Why not a webmasters or infrastructure issue for this?

Heine’s picture

As this is a general feedback stage, not for implementation.

Dave Reid’s picture

http://drupal.org/node/526194 is basically another +1 for this. We could have stopped the mass-amount of themes this person has been adding at the first one. Now we have to do freaking cleanup.

Dave Reid’s picture

What would be even awesomer is if we could have a "CVS status" as a checkbox on the user account on d.org. That way we could temporarily disable CVS access for people like http://drupal.org/user/391740 and as a plus, the d.org webmasters with the admin users permission could make the change.

Michelle’s picture

It is possible to suspend cvs access. You do need to have access to the cvs apps to do it, though.

Michelle

greg.harvey’s picture

But why is there so little activity and from months ago? If we need community feedback, we'd better start telling the community this is under review - first I heard of it until Dave Reid put me on to the thread from a blog post I wrote suggesting exactly this!

I think the proposal as it stands is perfect. +1 from this corner.