my ($email) = $dbh->quote($CGI->param('email') || undef);
my ($password) = $dbh->quote($CGI->param('password') || undef);
First, what is the purpose of $CGI->param('...') || undef? If the parameteter is missing, param() will return undef in scalar context, or an empty list in list context. But: If that parameter is empty or '0', you will replace it with undef. Why do you want to do that?
Second, calling DBIs quote() method is a sure sign that something is wrong with your code. No code outside DBDs should need to call the quote() method.
my $tmp = <<EOF;
SELECT
id
FROM
users
WHERE
LOWER(email) = LOWER($email)
AND password = $password
EOF
my $sth = &return_query($dbh, $tmp);
Here is what is wrong with your code. At least, you used $dbh->quote(), so you are not vulnerable to SQL injection here. But still, you construct a different SQL statement for each request, preventing any caching and forcing redundant work. See Re: Counting rows Sqlite and Re^2: Massive Memory Leak.
Re-think your return_query() function. I guess it is nothing more than a wrapper for $dbh->prepare($tmp) and $dbh->execute(). Drop that, or extend that wrapper to allow placeholder values being passed to execute(). Also, when you run your code from a persistant environment (FastCGI, mod_perl, Apache::Registry), switch to prepare_cached().
Also: Why do you use the ancient perl 4 syntax to call return_query()? Are you aware that that syntax is not only more typing, but also bypasses some error checks? Drop that ampersand, here and everywhere else.
Next problem: You seem to store passwords in plain text in your database. This is a really stupid idea. Use a salted hash instead, compare salt from the database + password from user input with the hash in the database.
my @row = $sth->fetchrow_array;
$result{userID} = undef;
if (@row) {
$result{userID} = $row[0];
Once you get some non-empty result from the database, you assume that there is only one row with the result. How can you be that sure? You compare the email using the database's LOWER function. Does the database make sure that LOWER(email) is unique for the entire users table?
if ($@) {
push(@{ $result{error} }, $@);
}
return encode_json(\%result);
%result seems to be a global variable. Why isn't it a local variable (i.e. my %result?
Why do you return %result JSON-encoded? With a local variable %result (my %result), you could simply return a reference to %result: return \%result.
Not your fault: CGI::Session has a scary ID generator that may bite you one day.
Alexander
--
Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
| 