HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Increase view counter with each page view

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
eachwithviewincreasepagecounter

Problem

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.

 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;
}
?>

Solution

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.

Context

StackExchange Code Review Q#115223, answer score: 3

Revisions (0)

No revisions yet.