5
\$\begingroup\$

I created a dynamic insert function for PDO.

It does work, but I want to be sure that it is secure and well coded.

function insertNewRows($pdo, $table, $fields, $placeholders, $values=''){

    if($values){
        $result = $pdo->prepare("INSERT INTO $table ($fields) VALUES ($placeholders)");
        $result->execute($values);
    }else{
        $result = $pdo->query("INSERT INTO $table ($fields) VALUES ($placeholders)");
    }

}

Using positional placeholders:

$session_id = '1000000001';
$createdBy  = 12;

$fields = "session_id, username, createdBy";
$placeholders = "?, 'boss 21', ?";

$values = array($session_id,  $createdBy);

insertNewRows($pdo, 'web_statistics', $fields, $placeholders, $values);

Not using positional placeholders:

$fields = "session_id, username, createdBy";
$placeholders = "'abc', 'boss 25', 3";

insertNewRows($pdo, 'web_statistics', $fields, $placeholders);

I want to use the same tactic for UPDATE and SELECT functions, so am I good to do so with the code I have written?

\$\endgroup\$
6
\$\begingroup\$

I don't see any point in this function in its present form.

Given you've got a general purpose query function,

function pdo($pdo, $sql, $args = NULL)
{
    $stmt = $pdo->prepare($sql);
    $stmt->execute($args);
    return $stmt;
}

it will be cleaner, easier to use and more readable to have a conventional SQL than such random rags of a code:

$session_id = '1000000001';
$createdBy  = 12;

$sql = "INSERT INTO web_statistics (session_id, username, createdBy) VALUES (?,?,?)";
pdo($pdo, $sql, [$session_id,  'boss 21', $createdBy]);

The same goes for SELECT and UPDATE. You can find a complete set of examples by the link above.

Update.

Looks like this question got some traction, due to some mysterious ways of Stack overflow. It means it will attract people other than the OP, who may have the same question but have different code, or just curious about Dynamic insert function for PDO in general. Well then, I must admit that the answer is rather a trick and too localized, being focused on the actual code in the OP. For the present code, where different query parts are still hardcoded in the different function parameters, it is indeed makes a lot more sense to have them hardcoded in the form of a conventional SQL query instead.

But of course the common idea of a dynamic insert function for PDO is different. it's rather a function that is called like this:

prepared_insert($pdo, $table_name, $data);

where $data is an associative array like this

$data = [
    'name' => $name, 
    'password' => $hashed_password,
];

Well then for this case I've got a simple INSERT helper function for PDO Mysql

function prepared_insert($pdo, $table, $data) {
    $keys = array_keys($data);
    $keys = array_map('escape_mysql_identifier', $keys);
    $fields = implode(",", $keys);
    $table = escape_mysql_identifier($table);
    $placeholders = str_repeat('?,', count($keys) - 1) . '?';
    $sql = "INSERT INTO $table ($fields) VALUES ($placeholders)";
    $pdo->prepare($sql)->execute(array_values($data));
}

Note that his function will need a helper function of its own,

function escape_mysql_identifier($field){
    return "`".str_replace("`", "``", $field)."`";
}

to protect identifiers added to the query.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Nice, it looks good! Will do like this. Thanks for such a fast answer! \$\endgroup\$ – Ingus Apr 1 at 13:32
  • \$\begingroup\$ Just tested INSERT and UPDATE . .. Worked like a charm! \$\endgroup\$ – Ingus Apr 1 at 13:44
  • 1
    \$\begingroup\$ I updated the answer, to provide the actual insert helper function :) \$\endgroup\$ – Your Common Sense Apr 2 at 7:37
2
\$\begingroup\$

Rather than sending comma-delimited strings, why not simply pass an array with value/key pairs eg:

array(
   "field_str" => "foo",
    "field_int" => 100,
   "field_float" => 3.1415926535,
);

So you can pass values in mixed data types. This is more flexible. Then loop on the array and build a parameterized query.

But you still require validation of the field names and table names, it would be a good idea to enclose them in backticks or brackets.

The problem with this approach is that a hacker who finds a flaw could insert arbitrary data in any table, for example the table of users. Also, if the table name is prefixed with a dot, it could be possible to insert records to a table in another database. PDO will not protect against injection here.

To secure this function you need lots of validation and you are never sure you will cover all possibilities. That does not sound like a good idea overall.

Like suggested by @Your Common Sense, it would better to have a reusable function but provide the SQL query yourself.

You can still develop wrapper functions, for example a function to add/modify/delete a user. In this function you can put all the logic and table names to avoid repetition. It is actually a good idea to use some level of abstraction. There is the DRY rule (don't repeat yourself). Write functions for repeated stuff.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for answer! Your suggestion about array seems to be unnecessary here as it puts extra work on validation. But i do agree about using functions for repeated stuff. \$\endgroup\$ – Ingus Apr 1 at 13:51
  • \$\begingroup\$ It is the table and field names that require validation to protect against injection attacks. The array not so much, because the values can be parameterized. The table and field names cannot. The array does not involve extra work but it cannot solve the problem completely. \$\endgroup\$ – Anonymous Apr 1 at 14:02
  • 1
    \$\begingroup\$ There are plenty of SQL injection techniques. Whenever you process input from a remote client there is a risk of injection using comments delimiters, nullbyte injection, etc. Malicious values can potentially contaminate (and rewrite) the whole query if the code is not solid and validation is weak. I suggest that you run SQLmap against your application - you might be surprised at what this tool could find with just the default settings. It is always a good idea to pentest yourself, before someone else does. \$\endgroup\$ – Anonymous Apr 1 at 14:13
  • 1
    \$\begingroup\$ @Ingus the key word is "usually". This "usually" notion is the source of 90% of actual injections in the world. An application is never a static piece of code. It evolves. Other people join to the project, knowing absolutely nothing of your notions and intents. They see a function, consider it safe and use it. There is a lot of people thinking that certain form elements are "not coming from the user side". This is why the entire function must be safe, not some unspoken ideas on how it should be used. \$\endgroup\$ – Your Common Sense Apr 2 at 5:11
  • 1
    \$\begingroup\$ From this standpoint I don't really get the kind of open-ended answers like this one.It basically suggests you can do something but really shouldn't because of danger. Or you must do some vague "lots of validation" but without a single hint on how it must be done. And "enclose in backticks or brackets." is not a validation at all \$\endgroup\$ – Your Common Sense Apr 2 at 5:14
1
\$\begingroup\$

Use a query builder, like the Doctrine one, for instance: https://www.doctrine-project.org/projects/doctrine-dbal/en/2.10/reference/query-builder.html#sql-query-builder

| improve this answer | |
\$\endgroup\$

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.