Safe PDO Parameter Generator?

[benanamen]

I saw this code on another forum and tried to reconcile it to the SQL Injection article here: https://phpdelusions.net/pdo/sql_injection_example

 

My question is, is this version I found safe? Is there a better way to do the same thing?

<form method=POST>
<input type=hidden name="data" value="sumdata">
<input type=hidden name="title" value="sumtitle">
<input type=hidden name="id" value="1">
<input type=submit>
</form>

<?php

if ($_SERVER['REQUEST_METHOD'] == 'POST')
    {
    $columns = ['title', 'data', ];
    $where = [];
    $values = [];

    foreach($_POST as $key => $val)
        {
        if (empty($val) || !in_array($key, $columns))
            {
            unset($_POST[$key]);
            }
          else
            {
            $where[] = " $key = ? ";
            $values[] = $val;
            $id = $_POST['id'];
            }
        }

    if (!empty($where))
        {
        array_push($values, $id);
        $where = implode(', ', $where);
        $sql = "UPDATE table SET $where WHERE userID = ?";

        // $stmt = $pdo->prepare($sql);
        // $stmt->execute($values);
        // These prints are JUST for showing you what happens in the query

        echo $sql;
        echo "<pre>";
        print_r($values);
        echo "</pre>";
        }
      else
        {
        echo "Update was not Successful.";
        }
    }
?>

Edited December 5, 2016 by benanamen

[requinix]

Prepared statements are the better way. Are you asking for the sake of hearing that answer repeated once more, or are you unsure how to deal with the variable number of placeholders?

[Jacques1]

You don't need dynamic columns at all:

$updateStmt = $databaseConnection->prepare('
    UPDATE table
    SET
        title = IFNULL(:title, title),
        data = IFNULL(:data, data)
    WHERE
        userID = :user_id
');
$updateStmt->execute([
    'title' => @$_POST['title'],
    'data' => @$_POST['data'],
    'user_id' => $id,
]);

When you do encounter a case where a dynamic query is in fact necessary, I would

• use a carefully tested function instead of inline code
• always set the strict flag when calling in_array() – weak typing is a bad idea for security, especially PHP's version of weak typing
• insert the hard-coded string rather than the user-provided one; again, PHP is a crazy language

Edited December 5, 2016 by Jacques1

[benanamen]

@requinix, The first part of the question was in reference to the code in the linked article regarding injection. I wanted to know if there was something I wasn't seeing in regards to injection in the similar code I posted. I know prepared statements are better and the posted code does generate a prepared statement.

 

The second part was, if you are going to dynamically generate a query, is there a better way to dynamically generate it than the posted code.

 

@Jaques1, I figured a dynamic query would go in a function. It would be pointless to keep writing the same thing more than once. I looked up the strict flag and even tested it here http://in_array.onlinephpfunctions.com/. I can't tell what the flag is doing or not doing. 

 

I have never seen the example you posted. What can you tell me about it?

[Jacques1]

I have never seen the example you posted. What can you tell me about it?

 

My prepared statement only updates the columns where the input value is not NULL, all others keep the old value. This a conditional update just like yours, but it doesn't require you to literally leave out the columns.

 

You shouldn't use empty(), though, because it has some confusing results. For example, the string “0” is considered “empty” and will be skipped, which makes no sense from the user's perspective. If you want to skip empty strings, do that explicitly:

'title' => (isset($_POST['title']) && $_POST['title'] !== '' ? $_POST['title'] : null)

I looked up the strict flag and even tested it here http://in_array.onlinephpfunctions.com/. I can't tell what the flag is doing or not doing.

 

By default, in_array() uses sloppy comparisons, which is very confusing and a common security problem:

var_dump( in_array(0, ['foo', 'bar']) );        // true
var_dump( in_array(true, ['foo', 'bar']) );     // true
var_dump( in_array('1e3', ['1000']) );          // true (WTF)

As you can see in the last case, even two strings(!) will be affected by PHP's crazy type juggling.

 

If you want in_array() to only return true when the input is fact present in the array, you need to enable strict checks with the third parameter.

[benanamen]

So in your example, if the value is null, you're just hiding an error with @? Also, there is a pretty significant difference from what I posted and your example, one is dynamic, the other is not.

 

Good point on the empty(). Sure enough, a field with zero will be skipped. I always want to know everything wrong with a piece of code.

 

I probably would never even use the posted code but it would seem that you would not want to skip empty strings on an update. How else are you going to update a column with data to no data?

 

I will have to spend some time with the is_array flag. I still don't understand it.

[kicken]

I will have to spend some time with the is_array flag. I still don't understand it.

The strict flag basically means php uses the === operator to compare instead of the == operator, which you can read about in the manual. There's also a comparison table

[benanamen]

Yeah, I saw that when I was looking into it. It made it much more clearer once I saw that. In testing though there was a few things that still didn't make sense in the results.