patternphpMinor
UPDATE SQL with prepared stmt using only 1 variable
Viewed 0 times
updatesqlwithpreparedusingstmtvariableonly
Problem
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?
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.
In this way, the variable $items doing all job :)
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 :)
Solution
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:
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 PDOCode Snippets
// 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 PDOContext
StackExchange Code Review Q#43716, answer score: 3
Revisions (0)
No revisions yet.