With SQL injection prevention using prepared statements, the UPDATE query is not executed

Hello,

In relation to the code from this website Updating row value from one table into another in MySQL database without success, I am trying to secure it against SQL injections using prepared statements. Please find the code which is a main part of the script below with explanation:


This is db_connection.php where where the insert() method is executed for the INSERT and UPDATE queries, and the insertID() method have to return the auto generated id used in the latest query, both in the ipn.php:

class dbConnect
{
    private $host = "localhost";
    private $user = "root";
    private $password = "";
    private $database = "database";
    private $conn;

    function __construct()
    {
        $this->conn = $this->connectDB();
    }

    function runQuery($query, $param_t, $param_val_arr)
    {
        $sql = $this->conn->prepare($query);
        $this->bindQueryParams($sql, $param_t, $param_val_arr);
        $sql->execute();
        $result = $sql->get_result();
        
        if ($result->num_rows > 0) {
            while ($row = $result->fetch_assoc()) {
                $resultset[] = $row;
            }
        }
        
        if (! empty($resultset)) {
            return $resultset;
        }
    }

    function bindQueryParams($sql, $param_t, $param_val_arr)
    {
        $param_val_ref[] = & $param_t;
        for ($i = 0; $i < count($param_val_arr); $i ++) {
            $param_val_ref[] = & $param_val_arr[$i];
        }
        call_user_func_array(array(
            $sql,
            'bind_param'
        ), $param_val_ref);
    }

    function connectDB()
    {
        $conn = mysqli_connect($this->host, $this->user, $this->password, $this->database);
        return $conn;
    }

    function insert($query, $param_t, $param_val_arr)
    {
        $sql = $this->conn->prepare($query);
        $this->bindQueryParams($sql, $param_t, $param_val_arr);
        $sql->execute();
    }

    // insertID() method have to return the auto generated id used in the latest query
    function insertID($query)
    {
        $sql = $this->conn->prepare($query);
        $this->bind_param($sql);
        $sql->execute();
        $new_id = $this->mysqli->insert_id;
    }
}

This is part of the ipn.php where everything is executed well up until to the part where the UPDATE query has to be executed:
$txn_id = !empty($_POST['txn_id'])?$_POST['txn_id']:'';
$custom = $_POST['custom'];

include("db_connection.php");
$db = new dbConnect();

// check whether the payment_status is Completed
$payment_completed = false;
if($payment_status == "Completed") {
	$payment_completed = true;
}
// check that txn_id has not been previously processed
$unique_txn_id = false; 
$param_t="s";
$param_val_arr = array($txn_id);
$result = $db->runQuery("SELECT id FROM user_subscr WHERE txn_id = ?",$param_t,$param_val_arr);
if(empty($result)) {
    $unique_txn_id= true;
}
if($payment_completed) {
    $param_t = "ds";
    $param_val_arr = array($custom, $txn_id);
    $payment_id = $db->insert("INSERT INTO user_subscr( user_id, txn_id) VALUES(?, ?)", $param_t, $param_val_arr);
}
// error_log throws that $payment_id is undefined variable
if($payment_id && !empty($custom)){
	$param_type = "dd";
	$param_value_array = array($subscr_id, $custom);
        // insertID() method have to return the auto generated id used in the latest query
	$subscr_id = $db->insertID;
	$update = $db->insert("UPDATE members SET subscr_id = ? WHERE id = ?", $param_t, $param_val_arr);
}

In the ‘members’ table in the row in ‘subscr_id’ column where logged in user is has to be stored the last payment id taken from ‘user_subscr’ table from the ‘id’ column, but instead it is not updated. The error_log throws that $payment_id variable is undefined. Also I am not sure that insertID() method works as it should or at all. I know that somewhere I’ve messed up something but need assistance in how to solve this issue.

Thank you.

Your insert() method in the class definition does not seem to return anything. So even if you call the section of code that only runs if $payment_completed is true, $payment_id would have nothing in it.

You see the difference between the end of runQuery() and the end of insert() - that line starting with return is what comes back to the calling code and is assigned to the variable you provided.

If $payment_id is undefined, though, that suggests your first insert into user_subscr isn’t running either, as I’d expect it to be null if it called a function that returns nothing. In turn, that suggests that $payment_completed is not true. Where does $payment_status come from?

For that final query, your parameter type variable is wrong - you set $param_type, but you pass $param_t into the function.

Here:

// insertID() method have to return the auto generated id used in the latest query

similarly, your insertID() method doesn’t return the ID that it should. You don’t use it in the code at the moment, so it doesn’t matter.

And here:

// check that txn_id has not been previously processed

you have code to check, but you don’t actually use the check - you insert the new row anyway. You might be able to simplify that by setting the txn_id column to have a unique constraint, then try the insert and trap the error if a row already exists - there was a discussion about that method fairly recently on here.

Is it correct that your user-id is stored as a double? I don’t know what difference it makes - if any - as I use PDO rather than mysqli. But it seems a strange choice.

I must add - I have little experience with writing class modules. Your code bindQueryParams looks complicated to me, but might be fine. I am presuming that you’ve tested to see whether it is working the way it should.

I have missed to write it in the example code.

While modifying the shortened version into more readable, I also have missed to provide full modification.

It is actually borrowed and modified from here https://www.pontikis.net/blog/dynamically-bind_param-array-mysqli, and as you noticed the call-user-func-array() function is used as a solution for array params because $stmt->bind_param() itself does not accept array of bound variables, only variable by variable when parametrizing it.

The next piece of code works, as I am a bit confused that instead INSERT query to return TRUE/FALSE as I have read on many sources on internet, the variable returns the actual source of INSERT query when dumping it:

$mysqli = new mysqli("localhost", "root", "", "database");

$insert = $mysqli->prepare("INSERT INTO user_subscr(user_id, txn_id) VALUES(?, ?)");
$insert->bind_param("ds", $custom, $txn_id);
$insert->execute();

and for the auto generated id for the last query put into variable is like this:

$subscription_id = $mysqli->insert_id;

and then same as the above code except for the UPDATE query as they are using the same sequence.

What bothers me is the code exactly in bindQueryParams() method, maybe I have something messed up with the parameters passing by reference, or something isn’t set in my php.ini or elsewhere (I am using XAMPP as a local host testing environment). I’ve read somewhere on the internet that PHP knows also to be buggy, but I won’t delve into this because I might be totally wrong, but for mysqli_stmt_bind_param() and other functions developers of PHP should definitely do something about it instead for us mortals to find alternative solutions.

That would be a good reason to use PDO instead of mysqli, if you want to pass parameters by array.

It is put into that variable in this line in insertID():

  $new_id = $this->mysqli->insert_id;

but as the very next line is the close-brace for that method definition, the value is lost. You don’t even look at the value in insert(), the method you call, and also don’t pass it back. Look at the last line of your connectDB() method, and see how that is able to provide a value back to the code that called it, and why insertID() does not do that.

As above - your query is executed inside your insert method, and that doesn’t return any value at all. So when you test for the return in this line

if($payment_id && !empty($custom)){

there is no value in $payment_id, and therefore the stuff inside the if() does not execute.

I don’t use mysqli as I mentioned above, so all this has passed me by. I’m fairly convinced that the issues here are minor things you’ve missed out of your code. If you want to pass parameters as arrays, use PDO instead of mysqli as it does that. I picked it because I thought I might need my code to connect to something like MS SQL Server, which PDO will also do.

I think you need to look at adding some debugging code into this, write some variable values into a log file if your code is executed in the background and you can’t just use echo. That way you can follow the progress of the code and understand why it’s not working the way you expect. You can use the error_log() function to send stuff to a log file, just while you’re working it out.

ETA - now I’ve read that article, I think there is nothing wrong with the bindQueryParams method - if your first call to it to check the transaction-id is working, there’s a reasonable chance the rest will too. Even that article says how much easier it is to achieve with PDO, though.

1 Like

Thanks for your time explaining things to me droopsnoot, I will take your advice for granted.

1 Like