Mysql flexible insert record with bind_param or bind_value

Question

I want to insert rows in a database table with the precaution of SQL-injection. I'm using below flexible MySQL insert function, which enters a record in a given table. Is this the best approach or is there any other approach to do the same?

<?php
/**
 * This code writes data in database. It also takes care of sql injection case
 *
 * Entry function name : mainFunc
 * Entry function arguments : 
 * $payload = [
 *   'tableName' => 'table_name',
 *   'data' => ['column1_name' => column1_value, 'column2_name' => column2_value, etc]
 * ];
 */

Class AddDBEntry {
   private $dbName;
   private $dbUserName;
   private $dbPassword;
   private $dbHost;
   private $mysqli;

  // $stmt = The SQL Statement Object
  // $param = Array of the Parameters
  public function dynamicBindVariables($stmt, $params) {
    if ($params != null) {
      // Generate the Type String (eg: 'issisd')
      $types = '';
      foreach($params as $param) {
        if(is_int($param)) {
          // Integer
          $types .= 'i';
        } elseif (is_float($param)) {
          // Double
          $types .= 'd';
        } elseif (is_string($param)) {
          // String
          $types .= 's';
        } else {
          // Blob and Unknown
          $types .= 'b';
        }
      }

      // Add the Type String as the first Parameter
      $bind_names[] = $types;

      // Loop thru the given Parameters
      for ($i=0; $i<count($params); $i++) {
        // Create a variable Name
        $bind_name = 'bind' . $i;
        // Add the Parameter to the variable Variable
        $$bind_name = $params[$i];
        // Associate the Variable as an Element in the Array
        $bind_names[] = &$$bind_name;
      }

      // Call the Function bind_param with dynamic Parameters
      call_user_func_array(array($stmt,'bind_param'), $bind_names);
    }
    return $stmt;
  }

  public function mainFunc($payLoad) {
    $tableName = $payLoad['tableName'];
    $data = $payLoad['data'];

    $this->mysqli = new mysqli($this->dbHost, $this->dbUserName, $this->dbPassword, $this->dbName);

    if ($this->mysqli->connect_errno) {
      echo  "Failed to connect to MySQL: (" . $this->mysqli->connect_errno . ") " . $mysqli->connect_error;
      die();
    }

    $stmtString = 'insert into ' . $tableName . ' (' . implode(', ', array_keys($data)) . ')' . ' values ' . '(' . str_repeat('?, ', count($data) - 1) . '?)';

    $params = array_values($data);
    if (!($stmt = $this->mysqli->prepare($stmtString))) {
      echo "Unable to prepare statement";
      // stop here, $stmt is not set, cannot continue
      die();
    }

    $stmt = $this->dynamicBindVariables($stmt, $params);
    $stmt->execute(); 

    if ($stmt->affected_rows <= 0) {
      echo "Error in insert";
    }
    $stmt->close();
  }
}

Answer 1

There is a vast room for the improvement, but the main and the most critical problem is SQL injection. However, I have to admit, it is not that obvious, making too many PHP users prone to it. I even wrote a dedicated article, An SQL injection against which prepared statements won't help. It explains how to inject through column names, but in your case it's even simpler, as anyone could write any SQL right into $tableName, entering something like this:

news; drop table users --

in a $payLoad['tableName']. So here goes the golden rule for the SQL injection prevention:

The golden rule

Although people just learned by heart that prepared statements is what protects them from SQL injection, the complete rule consists of two parts:

1. A placeholder to substitute any data goes to the query
2. A white list for the everything else.

So now you can tell that your code lacks the other part.

The implementation

In a nutshell, a white list is a very simple thing to implement - just write down all the possible choices directly in your code and choose values to be added in the SQL query from this list only.

However, for such a generic function like yours, it could be a tedious task, to list all the possible table and column names. On the other hand, if you write all this stuff down, it won't be different from writing just a regular query, but that's sort of opposite to your initial goal. However "safety first" is typically the desired side to err on, and so I would recommend to reduce the flexibility, for sake of security. So for the moment I would recommend to avoid using the mainFunc() function at all, and instead using a series of hard-coded queries.

Improvements.

• creating a new connection every time you are running a query is a big NO. Although it doesn't matter for a primitive page that runs just a single query, but it will become a disaster for any site that is a little more complex than that. Create just a single connection, and then use it for all queries during the script's lifetime.
• function dynamicBindVariables is rather useless, because:
  • every current PHP version lets you to bind parameters right away
  • 99% of time you can use just "s" for any variable bound
• error reporting: Completely flawed. To get the idea refer to my article PHP error reporting. In short, just allow mysqli to generate errors by itself, but never echo an error message out.

So I would make your class like this:

Class DB
{
    public function __construct($host, $user, $pass, $db, $charset)
    {
        mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
        $this->mysqli = new mysqli($host, $user, $pass, $db);
        $this->mysqli->set_charset($charset);
    }

    public function query($sql, $params, $types = "")
    {
        $types = $types ?: str_repeat("s", count($params));
        $stmt = $this->mysqli->prepare($sql);
        $stmt->bind_param($types, ...$params);
        $stmt->execute();
        return $stmt;
    }
}

used like this for SELECT

$db = new DB("localhost", "root", "", "test", "utf8");
$row = $db->query("SELECT ? bar", ['foo'])->get_result()->fetch_assoc();

or like this for insert

$db = new DB("localhost", "root", "", "test", "utf8");
$data = [$username, $email, $password];
$row = $db->query("INSERT INTO users VALUES (NULL, ?,?,?)", $data);

although it is not that overly automated as you wanted, but it's 100% safe from SQL injection, given table and column names are hard-coded in SQL, while the data is bound through a prepared statement.