Assuming the class property $_pdo
is an instance of the PDO
class, this SQL in vulnerable to SQL-injection attacks. When inserting data into a database it is quite common this is user input, which should never be trusted and sanitized carefully.
The reason this is vulnerable is because you inject the actual values directly into the SQL query. You are using prepared statements (great), but unfortunately wrong. The SQL query passed to the prepare
method should take placeholders of the actual value instead. I recommend using named parameters. An example of such could be:
$sql = "INSERT INTO users (username, password) VALUES (:username, :password) LIMIT 1";
Here the placeholders are :username
and :password
. You should then bind the actual values to the query after is has been prepared. An example when using the SQL from above could be:
$username = 'awesomeness';
$password = 'secret';
$stmt = $this->_pdo->prepare($sql);
/*
* Note: the actual value passed to the PDO::bindParam method
* must be a variable. You should therefore store the actual
* data inside a variable first.
*/
$stmt->bindParam(':username', $username, PDO::PARAM_STR);
$stmt->bindParam(':password', $password, PDO::PARAM_STR);
/*
* Check if the query was executed. This does not check if any data was actually
* inserted as MySQL can be set to discard errors silently.
*/
if(!$stmt->execute()) {
throw new ErrorException('Could not execute query');
}
/*
* Check if any rows was actually inserted.
*/
if(!$stmt->rowCount() == 0) {
throw new ErrorException('Could not insert data into users table.');
}
return true;
This will greatly increase your resistence against SQL injection attacks. For your method to support this it should be restructured.
public function insert($table, array $data)
{
/*
* Check for input errors.
*/
if(empty($data)) {
throw new InvalidArgumentException('Cannot insert an empty array.');
}
if(!is_string($table)) {
throw new InvalidArgumentException('Table name must be a string.');
}
$fields = '`' . implode('`, `', array_keys($data)) . '`';
$placeholders = ':' . implode(', :', array_keys($data));
$sql = "INSERT INTO {$table} ($fields) VALUES ({$placeholders})";
var_dump($sql);
// Prepare new statement
$stmt = $this->pdo->prepare($sql);
/*
* Bind parameters into the query.
*
* We need to pass the value by reference as the PDO::bindParam method uses
* that same reference.
*/
foreach($data as $placeholder => &$value) {
// Prefix the placeholder with the identifier
$placeholder = ':' . $placeholder;
// Bind the parameter.
$stmt->bindParam($placeholder, $value);
}
/*
* Check if the query was executed. This does not check if any data was actually
* inserted as MySQL can be set to discard errors silently.
*/
if(!$stmt->execute()) {
throw new ErrorException('Could not execute query');
}
/*
* Check if any rows was actually inserted.
*/
if($stmt->rowCount() == 0) {
var_dump($this->pdo->errorCode());
throw new ErrorException('Could not insert data into users table.');
}
return true;
}
This code has not been tested thoroughly, but the basic functionality should be there.
The usage of the above function has been altered slightly as there are only 2 arguments now. The $data
argument takes an associative array of the field name as key and field-value as array value.
$table = 'users';
$fields = [
'username' => 'awesomeness',
'password' => 'secret'
];
try {
/*
* Have used the word 'object' as I could not see the actual
* class name.
*/
$object->insert($table, $fields);
}catch(ErrorException $exception) {
echo $exception->getMessage(); // Should be handled with a proper error message.
}
Hope this can help guide you, happy coding!
_pdo
is defined and how this function is called. \$\endgroup\$