Secured Database Insert

Question

I've been working on a safe inserting method for a while, and I think I've found a pretty secure way to do this. Would you CRitters mind taking a look at it and maybe give me some tips to improve my script?

function dbRowInsert($table, $data) {
   require_once('../config.inc.php');

     $buildData = null;
     $countLoop = 1;

     foreach($data as $field) {
          $sep = ($countLoop!=count($data) ? ',' : '') ;
      if((int)$field == $field) {
        $buildData .= (int)$field . $sep;
      } else {
        $buildData .= '"' .mysqli_real_escape_string($conn, (string)$field) . '"' . $sep;
      }
      $countLoop++;
     }

   $fields = array_keys($data);

   mysqli_query($conn, "INSERT INTO" . $table . "(`" . implode('`, `', $fields) . "`)
                        VALUES('" . $buildData . "')");
}

Answer 1

It looks like you're trying to create a generic INSERT function to put any data into the given table based on an array, securely.

Since you're using mysqli_ a better approach might be to use bound parameters.

It's not really "good practice" to require your config file in the function (I'm assuming it'll be pulled in anyway), it would be better to pass your $conn variable (which I assume is a mysqli object) into the function.

I'd also structure $data so that it's an associative array of fields to values, for example (as a sort of theoretical person insert). This means your values are bound to the field names within that array which makes a foreach loop less error prone.

$data = array(
    'forename' => "Fred",
    'surname' => "Bloggs",
    'email' => "fred.bloggs@example.com"
);

You'd have to have done any validation checks beforehand of course (making sure that the email is valid for instance).

Now your function could be written:

function dbRowInsert($conn, $table, array $data) {
    $fieldInsert = "";
    $valueInsert = "";
    $valueTypes = "";

    //array for bound parameters
    $paramData = array();

    //loop through the fields
    foreach($data as $fieldName => &$fieldData) {

        //SQL inserts
        $fieldInsert .= "{$table}.{$fieldName}, ";
        $valueInsert .= "?, ";

        //determine the data types for the bound parameters
        if(is_float($fieldData)) { $valueTypes .= "d"; }
        elseif(is_int($fieldData)) { $valueTypes .= "i"; }
        else { $valueTypes .= "s"; }

        //push the field data to the bound parameters array (by reference)
        $paramData[] = &$fieldData;
    }

    //trim the trailing ", " that will have been left
    $fieldInsert = substr($fieldInsert, 0, -2);
    $valueInsert = substr($valueInsert, 0, -2);

    //prepend the bound parameters array with the data types string
    array_unshift($paramData, $valueTypes);

    //prepare the statement
    $stmt = $conn->prepare("INSERT INTO {$table} ({$fieldInsert}) VALUES ({$valueInsert})");

    //error trapping
    if(!$stmt) { throw new Exception("Failed to prepare statement - ERR: {$conn->error}", E_USER_ERROR); }

    //call_user_func_array wouldn't be necessary with PDO as 
    //you could bind parameters one at a time in the loop
    call_user_func_array(array($stmt, 'bind_param'), $paramData);

    //execute ... and relax
    $stmt->execute();
}

It looks a bit longer but is has the following advantages:

1. it uses a prepared statement
2. the parameters are bound by type so it'll not be attempting to escape integers as strings for instance

The biggest problem is that it's using mysqli_ which doesn't have a nice way to bind parameters (or values) to the statement when you have an unknown number of parameters to bind ... hence the ugly call_user_func_array usage.

This could easily be mitigated by using PDO rather than mysqli_ though.