Though prepared statements do a pretty good job protecting your DB from injection attacks, it never hurts to perform some additional checks on the data you're receiving. For example: when a request is supposed to contain an email address, you might perform a SELECT query usign that email address, or insert it in your DB. If so, you should add this:
if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{
exit();//redirect, optionally show error message
}
Same goes for any posted data, as one of the comments mentions: XSS attacks are one of the biggest threats on the web today. I've dealt with the matter in this answer.
The very least you could do is:
$execParam = array_map(
'strip_tags'
array(
':buildtitle' => $_POST['addbuildtitle'],
':builddesc' => $_POST['addbuilddesc'],
':buildcover' => $imagepath,
':userid' => $_POST['adduserid'],
':frontpage' => $frontpage,
':addtags' => $_POST['addtags']
)
);
$statement->execute($execParam);
Here, I've just called strip_tags on each and every single item you're about to insert, to avoid some jokester posting
<script>location.href = 'http://malware-infested-site.org';</script>;
in the addbuilddesc field... because, if you don't filter those things out, people visiting your site won't be all too happy about that... and be honest: would you?
You can take this as far as you want. Since you're obviously using a traditional *SQL DB, you might use the table schema, to determine if the POST params are of the correct type and length:
//assume blogtitle is a VARCHAR(255) field:
':buildtitle' => sprintf('%255s',$_POST['addbuildtitle'])
//and userID is an unsigned int, 11 long
':userid' => sprintf("%11d",abs((int)$_POST['adduserid']))
This ensures your :buildtitle bind will be no longer than 255 chars. The userid value will not be negative, and will be an integer of max 11 digits long.
But to implement this would mean a huuuge mess of sprintf calls, so you could use dataModels for this (check pretty much all of my DB related answers on this site for details), and use getters and setters that use this sprintf function to format the data accordingly.
still, this will mean a lot of overhead for each and every query. Sadly, the tradoff will always be one of performance vs security. If security is all you're after, you'll also note that, in my other answers, I basically advise everybody who's trying to create a solid (as in strong, but also as in S.O.L.I.D.) query system, to have a look at doctrine. No need to reinvent the wheel...
Anyway, I'm going off course...
Now, as far as injection goes, prepared statements are pretty robust, provided you don't take it to new, absurd, extremes. I've seen people use variables instead of the DB name, just like I've seen them use POST params to fill in the table name. You don't seem to do that, and that's good... great even. Just a friendly warning: if ever you consider using variables in those parts of your query that should be static (SELECT fieldnames FROM tblNames), step back and rethink your approach, because that's where you may still introduce vulnerabilities to your system.
Basically, prepared statements work like this:
- Pass a string, from which a statement is to be prepared. This is processed by the DB server as is.
- The DB server parses, and "prepares" the query (optimization etc...)
- Pass the arguments, with which the statement is to be executed. These values (binds) are sent via a different protocol and here's where they're being escaped correctly.
As you can see, the binds are what is being escaped and sent completely separatly, that's why prepared statements are safe. This also explains why the actual SELECT and FROM clauses in a prepared statement may not be gotten from user input. This part of the query isn't processed in the same way the binds are...
Other niggles:
- At no point do you seem to be checking
isset($_POST['adduserid']) and other post keys. You just seem to expect they're there. That might trigger notices, and slows your code down. Just isset() all those params.
- You have a semi-colon at the end of your
if statement. If you copy-pasted this code, then that's going to give you grief.
- Ensure, especially with
INSERT's to set the PDO::ERR_MODE correctly, and insert in transactions. In case something goes wrong, you can still use rollBack!
Here's a little sample of how that would look
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);//force PDO to throw on error
try
{
$db->beginTransaction();//start here
$stmt = $db->prepare($query);
$stmt->execute($sanitizedInsertParamsArray);
$db->commit();//make insert permament
}
catch(PDOExcpetion $e)
{
$db->rollBack();//revert insert!
exit($e->getMessage());
}
The code above will throw an instance of PDOException if the prepare, execute call or even the commit fails. As long as commit hasn't been called, the transaction is still pending (ie the data isn't really inserted yet).
So simply put, this snippet asks the DB: prepare this statement, then try to execute it with these values, if you did that, then save it. If any of these three steps failed, $db->rollBack() just tells the DB: OK, so it didn't work, forget about it... don't try and save half of the data, just forget everything.
Transactions are a good way to avoid orphaned data (as are FK's, btw). And they make for a safer query all together, too...
