CGI is Okay, but Bad Code is Irresponsible

Update: IBM dW and the author have improved the code; I've followed up at Politely Suggesting Improvements.

IBM's developerWorks published an article yesterday describing a simple Ajax web login service.

The original code was horribly insecure, Bobby Tables-style "Anyone can log in without knowing a password merely by manipulating the query parameters" insecure. Fortunately, IBM fixed the code. Unfortunately, it's still bad Perl 5:

#!/usr/bin/perl
# BAD CODE DO NOT USE use CGI qw(:all);
# BAD CODE DO NOT USE use DBI;
# BAD CODE DO NOT USE use DBD::mysql;
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # read the CGI params
# BAD CODE DO NOT USE $CGI = new CGI();
# BAD CODE DO NOT USE $username = $CGI->param("username");
# BAD CODE DO NOT USE $password = $CGI->param("password");
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # connect to the database
# BAD CODE DO NOT USE $DBH = DBI->connect("DBI:mysql:database=mydb;host=localhost;port=2009",
# BAD CODE DO NOT USE                                         "mydbusername", "mydbpassword")
# BAD CODE DO NOT USE   or die($DBI::errstr);
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # check the username and password in the database
# BAD CODE DO NOT USE $query = qq{SELECT id FROM users WHERE username=? && password=?};
# BAD CODE DO NOT USE $query = $DBH->prepare($query);
# BAD CODE DO NOT USE $query->execute($username,$password);
# BAD CODE DO NOT USE $ref = $query->fetchrow_hashref() || 0;
# BAD CODE DO NOT USE $userID = $ref->{"id"};
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # create a JSON string according to the database result
# BAD CODE DO NOT USE $JSON = ($userID) ?
# BAD CODE DO NOT USE         qq{{"success" : "login is successful", "userid" : "$userID"}} :
# BAD CODE DO NOT USE         qq{{"error" : "username or password is wrong"}};
# BAD CODE DO NOT USE
# BAD CODE DO NOT USE # return JSON string
# BAD CODE DO NOT USE print qq{Content-type: application/json; charset=utf8\n\n};
# BAD CODE DO NOT USE print $JSON;

Perhaps "bad" is too strong a word. Here's that code cleaned up and written with a better foundation for writing real code. (After all, is this article not intended to give people a starting point for writing real code?)

#!/usr/bin/perl -T

use strict;
use warnings;

use CGI;
use DBI;

my $q        = CGI->new;
my $username = $q->param("username");
my $password = $q->param("password");

my $dbh = DBI->connect(
    'DBI:mysql:database=mydb;host=localhost;port=2009',
    'mydbusername', 'mydbpassword'
) or die $DBI::errstr;

my $sth = $dbh->prepare(
    'SELECT id FROM users WHERE username=? and password=?;'
);
$sth->execute($username, $password);

my $json;

if (my ($id) = $sth->fetchrow_array)
{
    $json = qq|{"success" : "login is successful", "userid" : "$id"}|;
}
else
{
    $json = qq|{"error" : "username or password is wrong"}|;
}

print $q->header(
    -type    => 'application/json',
    -charset =>  'utf-8',
), $json;

The differences are minor, but the corrected version of the code is a little less clunky. It follows the Perl style guide more closely (using the idioms of both the CGI and DBI modules) and avoids unnecessary work.

Is this nitpicky?

Perhaps. Perl is nothing if not pragmatic... but if you're going to publish example code used as the core of a login system, why wouldn't you write the best code you possibly can? It may be acceptable to disclaim responsibility for even explaining the security problems of storing plaintext passwords in the database, but posting code vulnerable to SQL injections in 2011 is irresponsible.

It's okay to write baby Perl, but it's baffingly irresponsible to publish said baby Perl as an example from which you expect other novices to learn.

It's almost like publishing home chemistry experiments for second graders where half of the instructions are "Mix together household cleaning ingredients. Be careful; some of them might be dangerous if you mix them together, but that's not the point of these instructions, so you are on your own."

With that said, some people have criticized the example for the use of the CGI module and the CGI mechanism for dynamic web programs in 2011—but that's a well-intentioned if wrong-headed criticism.

Certainly Plack offers an easier mechanism of experimentation and example deployment (which scales to production-ready deployment easily), once you understand Plack and have it installed and configured for your application...

... but for didactic purposes, as a baseline execution model which is relatively easy to understand due to its maturity and ubiquity, CGI is a decent choice for deployment.

The Perl world does need more articles and posts and examples of how to use Plack to replace the CGI model in all of its forms, but the irresponsibility in publishing this article rests with the editors who allowed the further propagation of bad Perl cobbled together.