PHP code no longer works when switching to mysqli

Question

I'm trying to convert some php code that uses mysql into mysqli code. I'm not sure why it doesn't work - I didn't write the original code and am not that comfortable with the hash part of it, and it seems to be where the issue is. As I show in the code below, the "error" part gets echo'ed so it's something to do with the hash strings, but I don't really understand why changing to mysqli has broken the code. Both versions of the code are below, and the original code works. I deleted the variables (host name, etc.) but otherwise this is the code I am working with.

Mysql Code:

// Send variables for the MySQL database class.
function db_connect($db_name)
{
    $host_name = "";
    $user_name = "";
    $password = "";
    $db_link = mysql_connect($host_name, $user_name, $password) //attempt to connect to the database
        or die("Could not connect to $host_name" . mysql_connect_error());
    mysql_select_db($db_name) //attempt to select the database
        or die("Could not select database $db_name");
    return $db_link;
}

$db_link = db_connect(""); //connect to the database using db_connect function

// Strings must be escaped to prevent SQL injection attack. 
$name = mysql_real_escape_string($_GET['name'], $db_link); 
$score = mysql_real_escape_string($_GET['score'], $db_link); 
$hash = $_GET['hash']; 

$secretKey=""; # Change this value to match the value stored in the client javascript below 

$real_hash = md5($name . $score . $secretKey); 
if($real_hash == $hash) { 
    // Send variables for the MySQL database class. 
    $query = "insert into scores values (NULL, '$name', '$score');"; 
    $result = mysql_query($query) or die('Query failed: ' . mysql_error()); 
}

Mysqli code (doesn't work):

// Send variables for the MySQL database class.
function db_connect($db_name)
{
    $host_name = "";
    $user_name = "";
    $password = "";
    $db_link = mysqli_connect($host_name, $user_name, $password) //attempt to connect to the database
        or die("Could not connect to $host_name" . mysqli_connect_error());
    mysqli_select_db($db_link, $db_name) //attempt to select the database
        or die("Could not select database $db_name");
    return $db_link;
}

$db_link = db_connect(""); //connect to the database using db_connect function

// Strings must be escaped to prevent SQL injection attack. 
$name = mysqli_real_escape_string($_GET['name'], $db_link); 
$score = mysqli_real_escape_string($_GET['score'], $db_link); 
$hash = $_GET['hash']; 

$secretKey=""; # Change this value to match the value stored in the client javascript below 

$real_hash = md5($name . $score . $secretKey); 
if($real_hash == $hash) { 
    // Send variables for the MySQL database class. 
    $query = "INSERT INTO `scores` VALUES (NULL, '$name', '$score');"; 
    $result = mysqli_query($db_link, $query) or die('Query failed: ' . mysqli_error($db_link)); 
    echo $result;
}
else {
    echo "error"; //added for testing. This part gets echoed. 
}


mysqli_close($db_link); //close the database connection

FYI, `mysqli_real_escape_string()` is not advisable for SQL-injection protection. — Patrick Q, Jan 22 '19 at 19:00
MD5 is insufficiently robust for any sort of security measure. Use SHA2-256 at the absolute least. — tadman, Jan 22 '19 at 19:06
I would also recommend that when doing any INSERT statements is to always list the column names you are inserting into. It makes the statement self documenting and also helps if column orders are changed in different database schemas. — Nigel Ren, Jan 22 '19 at 19:11
When you say it "doesn't work," you should tell which line it fails on, and include any error message it returns. — Bill Karwin, Jan 22 '19 at 19:21

score 2 · Accepted Answer · answered Jan 22 '19 at 18:59

2

One notable "gotchu" is that the argument order is not the same between mysql_real_escape_string and mysqli_real_escape_string, so you need to swap those arguments in your conversion.

$name = mysqli_real_escape_string($db_link, $_GET['name']); 
$score = mysqli_real_escape_string($db_link, $_GET['score']);

answered Jan 22 '19 at 18:59

FThompson

28,352
13
60
93

This is what was stopping the code from running, it runs after changing this so that's definitely what was wrong. Thank you! – Kristen Jan 22 '19 at 20:56

Jeremy · Answer 2 · 2019-01-22T19:42:15.583

The last if statement is controlling whether the mysql query gets run or not. Since you say this script is echoing "error" form the else portion of that statement, it looks like the hashes don't match.

The $hash variable is getting passed in on the URL string in $_GET['hash']. I suggest echo'ing $_GET['hash'] and $real_hash (after its computed by the call to MD5) and verify that they're not identical strings.

My hunch is that the $secretKey value doesn't match the key that's being used to generate the hash that's passed in in $_GET['hash']. As the comment there hints at, the $secretKey value has to match the value that's used in the Javascript, or the hashes won't match.

Also, you may find that there's a difference in Javascript's md5 implementation compared to PHP's. They may be encoding the same input but are returning slightly different hashes.

Edit: It could also be a character encoding difference between Javascript and PHP, so the input strings are seen as different (thus generating different hashes). See: identical md5 for JS and PHP and Generate the same MD5 using javascript and PHP.

You're also using the values of $name and $score after they've been escaped though mysqli_real_string_escape, so I'd suggest making sure Javascript portion is handling that escaping as well (so the input strings match) and that the msqli escape function is still behaving identically to the previous version. I'd suggest echo'ing the values of $name and $score and make sure they match what the Javascript side is using too. If you're running the newer code on a different server, you may need to set the character set to match the old server. See the "default character set" warning at http://php.net/manual/en/mysqli.real-escape-string.php.

tadman · Answer 3 · 2019-01-22T19:24:58.933

It's good that you're taking the time to convert, though do convert fully to the object-oriented interface if mysqli is what you want to use:

// Send variables for the MySQL database class.
function db_connect($db_name)
{
    $host_name = "";
    $user_name = "";
    $password = "";

    // Enable exceptions
    mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

    $db = new mysqli($host_name, $user_name, $password);
    $db->select_db($db_name);

    return $db;
}

$db = db_connect(""); //connect to the database using db_connect function

$secretKey=""; # Change this value to match the value stored in the client javascript below 

$real_hash = md5($name . $score . $secretKey); 

if($real_hash == $_GET['hash']) { 
    // Don't include ; inside queries run through PHP, that's only
    // necessary when using interactive MySQL shells.

    // Specify the columns you're inserting into, don't leave them ambiguous
    // ALWAYS use prepared statements with placeholder values
    $stmt = $db->prepare("INSERT INTO `scores` (name, score) VALUES (?, ?)"); 
    $stmt->bind_param("ss", $_GET['name'], $_GET['score']);

    $result = $stmt->execute();

    echo $result;
}
else {
    echo "error"; //added for testing. This part gets echoed. 
}

// Should use a connection pool here
$db->close();

The key here is to use prepared statements with placeholder values and to always specify which columns you're actually inserting into. You don't want a minor schema change to completely break your code.

The first step to solving a complex problem is to eliminate all of the mess from the solution so the mistakes become more obvious.

PHP code no longer works when switching to mysqli

3 Answers3