Increase view counter with each page view

Question

My question is similar to before, but now the code has changed completely. I would like to understand if this code is vulnerable to mysql injection.

    <?php

// Define our querystring variables
$option = (array_key_exists('option', $_POST) ? $_POST['option'] : false);
$view   = (array_key_exists('view', $_POST) ?  $_POST['view'] : false);
$id     = (array_key_exists('id', $_POST) ?  intval($_POST['id']) : false);

// Check wheter our variables are correct in order to proceed
if ($option == 'com_content' && $view == 'video' && $id > 0) {

    // Connect to the database
    include_once("../configuration.php");
    $cg = new JConfig;
    $con = mysqli_connect($cg->host,$cg->user,$cg->password,$cg->db);
    if (mysqli_connect_errno()) {
        die('n/a');
    }

    // Add new hit: Update the `times_viewed` field corresponding to specific video id
    $query = "UPDATE " . $cg->dbprefix . "hdflv_upload 
                        SET `times_viewed` = `times_viewed` + 1 
                        WHERE `id` LIKE " . $id . ";";

    if (mysqli_query($con, $query) === true) {

    // Get the updated `times_viewed` of video id
        $query  = "SELECT `times_viewed` 
                             FROM " . $cg->dbprefix . "hdflv_upload 
                             WHERE `id` LIKE " . $id . ";";
        $result = mysqli_fetch_assoc(mysqli_query($con, $query));
    }

    // close the connection to the database
    mysqli_close($con);
    $addone = $result['times_viewed'];
    echo $addone;
}
?>

Answer 1

No, this code is not vulnerable to SQL injection. The use of intval() and if (… && $id > 0) guarantee that $id is a positive integer, so the UPDATE and SELECT queries cannot be malicious.

It is very weird that you use the LIKE operator, though. Is the id column not an INTEGER? I would expect that that works by converting every id into a string, then performing pattern matching. That would be slow because

• Converting every id in the hdflv_upload table would be extra work.
• The database cannot locate the row with the right id using an index; it has to examine every id.
• Pattern matching is more complicated than a simple integer equality check.

Note that in this version, you have fixed a race condition that existed in the previous version. In the previous version, it is possible to under-count if two pages simultaneously try to update the count, and the sequence ends up being SELECT, SELECT, UPDATE, UPDATE. This code is better because UPDATE hdflv_upload SET times_viewed = times_viewed + 1 WHERE … should be immune to race conditions.

Answer 2

I would like to understand if this code is vulnerable to mysql injection.

It's not vulnerable, but this isn't the preferred way to prevent SQL Injection.

Sanitizing the variables all over the place is a nightmare to maintain, especially in a larger code base. You never really know which variable is safe (did you clean that variable already? If so, as what? An integer? A string? Do you need quotes around it or not? ... which means that eventually, you will make a mistake). Also, checking the code for vulnerabilities takes a lot of work because the whole context must be taken under consideration.

Whenever possible, you should use prepared statements. It results in nicer code, and the vulnerability is defended right where it would happen, so you can be sure that it is always secure.

Limiting what your input can be when receiving it (as you are doing here when accessing $_POST['id'] by passing it to intval, as you know that it has to be an integer) is still always a good idea as defense in depth though.

Misc

• I would use isset instead of array_key_exists (it's shorter, and it also catches null).
• In production code, your comments aren't really necessary. They just describe what the code is already telling us.
• Your error checking could use some improvement. You check if your first query failed, but not your second. Also, if the first query fails, you don't report it, and you still access $result, which wouldn't exist at that point.
• Just dying with an unspecific error message isn't such a good idea, as it makes debugging difficult. I would throw an exception and deal with it elsewhere, but if this is your whole code, you could make the error message more specific, eg could not connect to database.