Very basic PDO method using bindParam()

Go To StackoverFlow.com

1

I have a similiar class with a lot of methods, but getData() only returns the value of the $column parameter.

private $db;

function __construct()
{
    $this->db = new PDO('sqlite:\db');
}

public function getData($rowid, $column)
{
    $st = $this->db->prepare('SELECT ? FROM tbl WHERE rowid=?');
    $st->bindParam(1, $column, PDO::PARAM_STR);
    $st->bindParam(2, $rowid, PDO::PARAM_INT);
    if ($st->execute())
        return $st->fetchColumn();
    else
        return false;
}

Every other parts of the class and the left out half of getData() works. What's the problem here?

2012-04-04 17:31
by Gergő


2

bindParam is used to bind parameters, not identifiers. The value you bind there will be expanded as:

SELECT 'some_value' FROM tbl WHERE rowid='some_other_value';

...therefore equivalent to:

SELECT 'some_value';

You should only use parameters for actual parameters:

$this->db->prepare('SELECT '.$column.' FROM tbl WHERE rowid=?');

If your column is user-supplied and you want to escape it, use the proper escaping function. In this case, it's SQLite3::escapeString():

$column = SQLite3::escapeString($column);
$this->db->prepare('SELECT '.$column.' FROM tbl WHERE rowid=?');

If the column is not user-supplied, you don't really have to escape it.

2012-04-04 17:35
by netcoder
to add: If the column is user supplied, the code may smell - erenon 2012-04-04 17:36
@erenon: Possibly, but not necessarily. There are use cases where the user supplying the column name is a better design choice than maintaining a list of columns in the code. It's rare, but it may happen. Good point though - netcoder 2012-04-04 17:40


0

identifier is not a string.
you can't bind identifiers.
you have to whitelist them instead.

or - better - design your application proper way that will require no dynamical fieldname at all.

2012-04-04 17:38
by Your Common Sense
That is assuming $column is user-supplied, which may not even be the case. There is no way to tell with the actual code - netcoder 2012-04-04 17:44
that is assuming bad design - Your Common Sense 2012-04-04 17:47
@netcoder It is not. This class supposed to be some kind of more specific interface between PDO and the application logic. Only $rowid is user-supplied, and I properly escape it - Gergő 2012-04-04 17:51
@YourCommonSense I only use this application to try out things, but what's the problem with having such a universal method - Gergő 2012-04-04 17:53
@Gergő what's the point in "proper escaping" binded variable? What's the problem I could tell if I'd know the purpose - Your Common Sense 2012-04-04 18:13
@YourCommonSense "Escaping" was the wrong word, $rowid is supplied by the query string, and its validiated as an integer. In such cases should I just pass the parameters to execute()? And for single calls is it more reasonable to use query() and exec()? And finally is it a bad pratice in itself to have a method with column as a parameter, or briefly what should I pay attention to? Thank you in advance - Gergő 2012-04-04 19:54
Ads