First off: You're not making full use of prepared statements. That could expose you to to SQL injection problems.
You should not be using string concatenation to build up the argument to $conn->prepare()
. Instead, you should be using a string constant as the SQL query, and relying upon bindValue()
to fill in dynamic values. In an ideal world you could write something like this:
$stmt = $conn->prepare("SELECT `lastMove` FROM :table".
" WHERE `id`=:gameID AND (`whiteUserID` = :userid".
" OR `blackUserID` = :userid) LIMIT 1");
$stmt->bindValue(':gameID', $_POST['gameID'], PDO::PARAM_INT);
$stmt->bindValue(':table', GAMES_TABLE, PDO::PARAM_STR);
$stmt->bindValue(':userid', USER_ID, PDO::PARAM_STR);
$stmt->execute();
However, PDO prepared statements don't let you specify the table name in this way, so you're forced to use string concatenation for the table name, like this:
$stmt = $conn->prepare("SELECT `lastMove` FROM ".
mysql_real_escape_string(GAMES_TABLE).
" WHERE `id`=:gameID AND (`whiteUserID` = :userid".
" OR `blackUserID` = :userid) LIMIT 1");
$stmt->bindValue(':gameID', $_POST['gameID'], PDO::PARAM_INT);
$stmt->bindValue(':userid', USER_ID, PDO::PARAM_STR);
$stmt->execute();
Using string concatenation with any non-constant string to build up the first parameter to prepare()
introduces a danger of SQL injection attacks, so you need to be careful here. You need to check carefully that GAMES_TABLE
is a constant and can never be influenced by the attacker (you might consider inlining it). Also, you might want to sanitize/escape it just in case, as in the code I've shown above. It is an annoying limit of prepared statements that they can't handle this for you.
Rule of thumb: prepared statements are safe from SQL injection, as long as the SQL query itself is a constant string.
Unfortunately, you may discover a few cases where prepared statements are too limiting, such as dynamic ORDER BY or LIMIT parameters -- as well as dynamic table names, as illustrated above. For these cases, you will probably need to use string concatenation. In these cases, you must be extremely careful, as using string concatenation re-introduces the risk of SQL injection vulnerabilities.
In any case, you should still use prepared statements for everything you possibly can, such as for USER_ID
, as it is less error-prone than doing the string concatenation yourself. Also, because prepared statements make it easier to verify you have avoided SQL injection flaws, it is to your benefit to use prepared statements instead of string concatenation everywhere you can.
Additional checks: To answer your original question, if PDO implemented its API properly, you would not need any other checks on an integer value to prevent SQL injection problems. Unfortunately, @Krzysztof Kotowicz has pointed out that PHP PDO library is buggy: it ignores the PDO::PARAM_INT
argument and treats everything as a string, regardless. Therefore, you probably ought to do the extra coding that's needed to make up for this flaw in the PDO library. In particular, you should coerce (cast) the value to an integer, before passing it. Thus, I suggest something like the following:
// don't use $_POST['gameID'] elsewhere; use $gameID
$gameID = (int) $_POST['gameID'];
$stmt = $conn->prepare("SELECT `lastMove` FROM ".
mysql_real_escape_string(GAMES_TABLE).
" WHERE `id`=:gameID AND (`whiteUserID` = :userid".
" OR `blackUserID` = :userid) LIMIT 1");
$stmt->bindValue(':gameID', $gameID, PDO::PARAM_INT);
$stmt->bindValue(':userid', USER_ID, PDO::PARAM_STR);
$stmt->execute();
Separately, you need to make sure that, if an attacker can guess a valid gameID, it does not help the attacker bypass any access control checks in your code. See, e.g., OWASP's insecure direct object reference vulnerability category.
For string values, it might also be prudent to sanitize the data to ensure it matches some regexp that is appropriate for that value.