3
\$\begingroup\$

HTML form field names must be equal to SQL table field names.

Changing only table name and allowed fields can be used in many other update pages.

How can I improve this?

$allowed = array("name","surname","email","rank"); 
$items = '';
foreach($_POST as $key => $value) {
    if (in_array($key , $allowed)) {
        $items.="`".str_replace("`","``",$key)."`". "='$value', ";
    }
}
$items = substr($items, 0, -2); 

$table = "users" ;
$userId = $_POST['userId'];

$sql = "UPDATE $table SET $items WHERE ID = ?";
if ($stmt = $mysqli->prepare($sql)) {
    $stmt->bind_param('i', $userId);
    $stmt->execute();
}

CODE REVIEW with PDO prepare statement

In accord with the answer, my first code was open to SQL injection and i change it with PDO prepared statement like a suggestion.

$dbh = new PDO('mysql:host=localhost;port=0000;dbname=xxx','yyy','zzz',
array(PDO::ATTR_PERSISTENT => false,PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION, PDO::ATTR_EMULATE_PREPARES => false));

$allowed = array("name","surname","email","rank");
$params = array();
$items = null;

foreach($_POST as $key => $value) {
    if (in_array($key , $allowed)) {
        $items .= "$key=:$key ,"; // create parametrized string
        $params[$key] = $value; // populate the array with allowed $_POST values 
    }
}

$items = substr($items, 0, -2); // escaping the last coma
$table = "users" ;

$sql = "UPDATE $table SET $items WHERE ID = :userId ";
$params['userId'] = $_POST['userId']; // add the WHERE param value to array
$stmt = $dbh->prepare($sql);
$stmt->execute($params);

In this way, the variable $items doing all job :)

\$\endgroup\$
3
\$\begingroup\$

As it stands right now, you are wide open to sql injection injection attacks because you are copying posted data directly into your sql update statement. All kinds of nasty things could be injected.

I started to rewrite it but it became a pain since mysqli requires you to bind each parameter individually. Way too much work. I'd suggest shifting to PDO which supports binding by arrays as well as named parameters. Not to mention exceptions.

Something like:

// Some test data
$_POST   = array('name' => 'New Name', 'email' => 'New Email', 'something' => 'nothing', 'userId' => 1);

$allowed = array("name","surname","email","rank"); 

$params = array();
$items = null;

foreach($_POST as $key => $value) {
    if (in_array($key , $allowed)) {
        if ($items) $items .= ', ';
        $items .= "$key=:$key";
        $params[$key] = $value;
    }
}
if (!$items) die("Nothing to update");
echo $items . "\n"; // name=:name, email=:email

$sql = sprintf('UPDATE %s SET %s WHERE ID = :userId','users',$items);

echo $sql . "\n"; // UPDATE users SET name=:name, email=:email WHERE ID = :userId

$params['userId'] = $_POST['userId'];

$stmt = $pdo->prepare($sql);
$stmt->execute($params); // This is reason enough to shift to PDO
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ With bind_param you can bind as much params as you want. (php.net/manual/de/mysqli-stmt.bind-param.php) \$\endgroup\$ – Marco Acierno Mar 7 '14 at 20:19
  • \$\begingroup\$ In theory but go ahead and show your code for binding a variable number of parameters. Unless something has changed, you can't just feed bind an array. \$\endgroup\$ – Cerad Mar 7 '14 at 20:29
  • \$\begingroup\$ @Cerad i understand and i change the code with PDO. Only 1 question : why you use sprintf in your code instead of : "UPDATE $table set $items WHERE ID = :userId" ? \$\endgroup\$ – geomo Mar 8 '14 at 14:58
  • \$\begingroup\$ Personal preference. Long time C programmer. I find sprintf's easier to read. Either approach is fine. \$\endgroup\$ – Cerad Mar 8 '14 at 15:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.