Help required to fully and properly parameterize a SELECT query

[excelmaster]

Hello All,

 

I'm having trouble trying to "wrap my head around" how I can make the query in my code below more efficient, elegant, and certainly immune to a SQL-injection attack

 

So, I have a form (which can be seen in the attached screenshot) which has the user first select a search-type via a dropdown listbox. The two choices pertain to two columns in the underlying table i.e. store_name and item_description. Once that selection is completed the user is expected to enter a search-term (which is typically in relation to the choice selected in the first prompt).

 

Now, what I would like to happen is that the query be created based on both, the value in the search-type prompt, as well as the value in search-term textbox, and of course, the query must be a parameterized prepared statement (to minimize the possiblity of a SQL-inection attach).

 

Essentially I would like to get rid of the following code (which I believe is potentially redundant, and contains a hard-coded column name between the WHERE and LIKE), to make things less complicated and more elegant/flexible

if ($searchtype == "store_name")
   {
       $sql = "SELECT * FROM `shoplist` WHERE store_name LIKE :searchterm ORDER BY store_name ASC";
   }
else if ($searchtype == "item_description")
   {
       $sql = "SELECT * FROM `shoplist` WHERE item_description LIKE :searchterm ORDER BY store_name ASC";
   }

Here's what I tried:

// Tried this first but it didn't work...gave some kind of error pertaining to the parameter 2    
// Prepare the query ONCE
$query = "SELECT * FROM `shoplist` WHERE ? LIKE ? ORDER BY ? ASC";

$searchterm = '%' . $searchterm . '%';

$statement = $conn->prepare($query);
$statement->bindValue(1, $searchtype);                                                        
$statement->bindParam(2, $searchterm);
$statement->bindValue(3, $searchtype);
$statement->execute();

// ----------------------------------------------------------------------------------------------------------- //

// Tried this as well, but was getting unexpected results (mostly all rows being returned, regardless of input)
// Prepare the query ONCE
$query = "SELECT * FROM `shoplist` WHERE :searchtype LIKE :searchterm ORDER BY :searchtype ASC";

$searchterm = '%' . $searchterm . '%';

$statement = $conn->prepare($query);
$statement->bindValue(':searchtype', $searchtype);
$statement->bindValue(':searchterm', '%' . $searchterm . '%');
$statement->execute();

Here's the complete code (pertaining to the problem/request):

// create short variable names
if(isset($_POST['searchtype']))
    {                                            
        $searchtype=$_POST['searchtype'];
    }
if(isset($_POST['searchterm']))
    {                            
        $searchterm=trim($_POST['searchterm']);
    }

if(isset($_POST['searchtype']) && isset($_POST['searchterm']))
    {
        if (!get_magic_quotes_gpc())
            {
                $searchtype = addslashes($searchtype);
                $searchterm = addslashes($searchterm);
            }
    }

if(isset($_POST['searchtype']) && isset($_POST['searchterm']))
    {
        // ****** Setup customized query ******
        if ($searchtype == "store_name")
            {
                // The following statement is potentially open to SQL-inection attack and has therefore been REM(arked)
                // $sql = "SELECT * FROM `shoplist` WHERE " . $searchtype . " LIKE :searchterm ORDER BY store_name ASC";

                // The following may not be open to SQL-injection attack, but has the column name hard-coded in-between the WHERE and LIKE clauses
                // I would like the the column name to be set based on the value chosen by the user in the form dropdown listbox for "searchtype"
                $sql = "SELECT * FROM `shoplist` WHERE store_name LIKE :searchterm ORDER BY store_name ASC";
            }
        else if ($searchtype == "item_description")
            {
                // The following statement is potentially open to SQL-inection attack and has therefore been REM(arked)
                // $sql = "SELECT * FROM `shoplist` WHERE " . $searchtype . " LIKE :searchterm ORDER BY item_description ASC";

                // The following may not be open to SQL-injection attack, but has the column name hard-coded in-between the WHERE and LIKE clauses
                // I would like the the column name to be set based on the value chosen by the user in the form dropdown listbox for "searchtype"
                $sql = "SELECT * FROM `shoplist` WHERE item_description LIKE :searchterm ORDER BY item_description ASC";
            }

        $statement = $conn->prepare($sql);
        $statement->bindValue(':searchterm', '%' . $searchterm . '%');
        $statement->execute();
        ...
        ...
        ...
    }

Thanks guys!

 

 

[Jacques1]

Hi,

 

the parameters are only for values, not identifiers. Simply set the column name in the query:

$search_query = '
    ...
    WHERE ' . ($searchtype == 'store_name' ? $searchtype : 'item_description') . ' LIKE ...
';

The ternary operator is just to make sure that $searchtype will never lead to an injection. If you trust your previous validations, you might insert the variable directly.

[mac_gyver]

only data values (strings, numbers, dates, ...) can be bound in prepared queries. the sql query syntax (keywords, column names, functions, operators...) is parsed when the query is prepared. only values are supplied through binding.

 

since your $searchtype is a column name, it cannot be supplied as a bound parameter.

 

 

Essentially I would like to get rid of the following code (which I believe is potentially redundant,

 

 

that logic is redundant in that it repeats parts of the query that don't change. you should only code for things that are different. the logic should only validate/supply "store_name" or "item_description" as the column name that gets put into the rest of the query statement.

 

the columns that are permitted to be used in the $searchtype need to be validated and limited to insure that only the values you have chosen can be used and to prevent sql injection or errors to be triggered (accidentally or on purpose.)

 

one possible way -

$permitted_searchtypes = array("store_name", "item_description"); // define which columns can be used as the search type

$searchtype = strtolower($searchtype); // force letter case

if(in_array($searchtiype,$permitted_searchtypes)){

    $sql = "SELECT * FROM `shoplist` WHERE $searchtype LIKE :searchterm ORDER BY store_name ASC";

    // prepared, bind, and run query here...

}

[excelmaster]

@Jacques1 and @mac_gyver: Thank you to the both of you for providing me with much a more elegant and compact code solution (which BTW works just great as-is).

 

I certainly didn't know that I could use the ternary operator in a SQL statement...and I certainly didn't think that I could/should setup permitted values/column-names in an array variable and use that for the query. I have learnt something new from the both of you.

 

Now, while on this subject I would also like to sanitize another input, in order to block the possiblility of SQL-injection, and I would greatly appreciate help with this one as well.

 

So, for/against all of my displayed rows, I display a checkbox to the extreme left of the row..which the user is expected to check for each row that they want updated. Upon clicking the [submit] button, the IDnumber(s) for each of the selected row(s) is passed/POSTed as an array.

 

I would like to be able to sanitize that IDnumber array (to ensure that nothing bad has been included instead of IDnumbers) before it's sent to the query.

 

 

Here's the relevant bits-and-pieces of my code brought together:

// loop through the results
foreach($result as $row)
    {
        print '<tr>';
            print '<td><span class="filler-checkbox"><input type="checkbox" name="IDnumber[]" value="' . $row["idnumber"] . '" /></span></td>';                                
            ....
            ....
        print '</tr>';
    }
    ...
    ...
    ...
print '<div class="full-row-buttons">';
    print '<input class="update-button-purchased" name="toggle-purchased" value="Flag selection as *Purchased/Not Purchased*" type="submit"/>';
    print '<input class="update-button-later" name="toggle-purchase-later" value="Flag selection as *Buy Later/Buy Now*" type="submit"/>';                            
print '</div>';

if(isset($functionSelected)){
    //Force all passed IDs to be integers and filter out 0/empty values
    $IDnumbersAry = array_filter(array_map('intval', $_POST['IDnumber']));

    //Format list into a comma separated string
    $IDnumberList = implode(', ', $IDnumbersAry);

    switch($functionSelected){
        case "update-delete":
            //Detemine the process to run
            switch($buttonClicked)
                {
                    case "Flag selection as *Purchased/Not Purchased*":
                        //Toggle purchased flag for selected records where the
                        //purchase_later_flag is NOT 'Y'
                        if(!empty($IDnumberList))
                            {
                                $query = "UPDATE shoplist
                                    SET purchased_flag = IF(purchased_flag='Y', 'N', 'Y')
                                        WHERE idnumber IN ($IDnumberList)
                                            AND purchase_later_flag <> 'Y'";                                                            
                                $statement = $conn->exec($query);
                            }
                        break;
                    ....
                    ....
                    ....
                }
            ...
            ...
            ...
        ...
        ...
        ...
    }
}
                                  

Here's the screenshot:

 

[Jacques1]

Personally, I'm not a fan of this type casting approach, because it mangles the user input. Invalid input is invalid input, not “Let's assume you meant the number {some guess}”. And of course this only works with numbers.

 

Generate a list of parameters and then pass your values as usual:

$update_stmt = $database->prepare('
    UPDATE ...
    WHERE idnumber IN (' . $implode(',', array_fill(0, count($_POST['IDnumber'], '?'))) . ')
');
$update_stmt->execute($_POST['IDnumber']);

Edited May 20, 2014 by Jacques1

[excelmaster]

@Jacques1: Using the code snippet you provide above I've come up with the following...which seems to throw a few errors (see attached picture), and I'm hoping you can help me resolve these errors:

 

I tried a few things...but nothing seemed to work (to fix these errrors). I did change your "$implode" to just "implode" though...since that was throwing an error as well.

$query = $conn->prepare('UPDATE shoplist
     SET purchased_flag = IF(purchased_flag="Y", "N", "Y")
     WHERE idnumber IN (' . implode(',', array_fill(0, count($_POST['IDnumber'], '?'))) . ')');
$query->execute($_POST['IDnumber']);

Thank you.

 

 

[Psycho]

I use this approach all the time

 

    //Force all passed IDs to be integers and filter out 0/empty values
    $IDnumbersAry = array_filter(array_map('intval', $_POST['IDnumber']));

 

 

There is zero difference in the results of doing this than in adding all the values as parametized values in the query. however, with respect to the rest of the code, you have a check to see if the array contains any values before running the query. But, that check is buried under two switch statements and an if(). Assuming all the logic under those two switch statements require this array of values, you could do that check first.

 

I'll add one last comment. I typically wrap my queries within functions or methods. The sanitizing of the input data would typically occur before the function/method is called. But, I don't want to risk passing potentially unsafe values to the function/method at some later point. So it would make sense to parametize the values anyway. I just prefer to run intval/array_filter before that so I can implement and error handling/messaging to the user.

[excelmaster]

@Psycho: Your approach (provided to me a few days back, much thanks) is exactly what I have in-place...but I just wasn't sure if that piece of code is indeed sanitizing the selected checkboxes/IDnumbers (before they get sent-to/used-by the query).

 

If it is indeed helping with sanitization then I certainly do not have to add the additional code provided by @Jacques1.

 

Thanks.

[Jacques1]

What I personally don't like about type casting with intval() or similar functions is that it mangles the user input and turns it into something they didn't ask for. It can also lead to value truncation.

 

For example, if the user (accidentally) enters an invalid value like "abc", you silently turn that into the number 0 and return all items with that ID (if there are any). Why 0? The user didn't ask for 0, they entered an invalid value and should be getting an empty list and/or an error message.

 

Type casting will also lead to problems with big numbers, because PHP integers are limited to a certain platform-dependent range. For example, a 32 bit system can neither hold an INT UNSIGNED nor a BIGINT. If the input is too big, it will be silently truncated.

 

Things like this can cause weird behavior and hard-to-find bugs for both your users and you, so it's probably best to avoid them altogether. I don't think saving one line of code is worth the trouble.

[excelmaster]

@Jacques1: Your example of "type-casting" now makes perfect sense to me, and I agree that it shold be avoided for exatly the reason you mentioned. (BTW, in your post from yesterday, I didn't quite understand what you meant by type-casting, since you didn't provide a clear example as you did today.

 

So, would you know the solution to fix the 3 errors that are being reported by your snipped of code?

 

Thanks once again to the both of you. Much appreciated!

[Jacques1]

I don't always test my code, so C&P isn't really a good idea. 

 

The closing parenthesis for the count() must be right after the $_POST['IDnumber'].

[excelmaster]

That certainly was it!!! That fixed all 3 errors and it's working now! Thank you!

 

I must be blind, to have not caught that...but then again, yes I was lazy and just did a straight C&P and assumed everything else would be correct!

 

Cheers

[Psycho]

@Jacques:

 

If 0 is a possible value, then using intval() is a bad idea. But, in almost all cases I am using it for values related to an auto-increment field which begins with 1. I guess my point is not so much about the use of intval() so much as it can be appropriate to perform a validation/sanitization of the submitted values instead of just relying upon the parametization in the query.

 

It all depends on the use case. So, intval() could be replaced with an appropriate process for the validation as needed.

[excelmaster]

Actually...I spoke/typed too soon!!!

 

I am now seeing the following error, and it is pointing to the WHERE clause (listed in the code block lower down):

 

"Catchable fatal error: Object of class PDOStatement could not be converted to string in /var/www/index.php on line 167"

WHERE idnumber IN (' . implode(',', array_fill(0, count($_POST['IDnumber']), '?'))) . ')';

I'm not entirely sure what that means, or how to fix it.

[Jacques1]

Check your syntax. You've added a closing parenthesis instead of moving the misplaced one.

'
	WHERE idnumber IN (' . implode(',', array_fill(0, count($_POST['IDnumber']), '?')) . ')
';

[excelmaster]

Yes indeed!!! That certainly was the case (and the culprit)!

 

Thank you again...all fixed, tested, and fully solved now.