Everything

php

Home » Project » Newsgroups » php.internals » pdo: binding variables supplied to execute() is NotVeryUseful(tm)... »

pdo: binding variables supplied to execute() is NotVeryUseful(tm)...

php.internals

Thies C . Arntzen

21 years ago

hi,

pdo hat it's own query-parser, named variables are prefixed with a
colon... so far - so nice...

i have a function called insert which is called like this:

$db->insert('some_table', array('name' => $name, 'age' => $age));

it's implemented like this:

function insert($table, $values) {
$column_names = array_keys($values);
$col_list = implode(', ', $column_names);
$bind_list = implode(', :', $column_names);

$sql = "insert into $table ($col_list) values ($bind_list)";

$stmt = $this->prepareStatement($sql);

// works:
// this is how it has to be done..
//
foreach ($values as $col => $val) {
$stmt->bindParam(":$col", $values[ $col ]);
}
$stmt->execute();

// doesn't work
// and this is how i would like it instead
//
# $stmt->execute($values);
}


and this is 'cause pdo _expects_ the user to prefix the bound variables
with a colon. grr...

ppls, lets change it before it's too late. this "tiny bit" makes binding
harder than it should be, and we all know and understand that all user
of php should use bound variables all the time!

suggestion: auto-trim a leading colon from the internal binding tables.
that way "oldish" code would not break...


re, thies

PS: looking into that right now - hopefully "patch follows";-)

Lukas Smith

21 years ago

Thies C. Arntzen wrote:

> and this is 'cause pdo _expects_ the user to prefix the bound variables
> with a colon. grr...
>
> ppls, lets change it before it's too late. this "tiny bit" makes binding
> harder than it should be, and we all know and understand that all user
> of php should use bound variables all the time!

you are not alone:
http://oss.backendmedia.com/index.php?area=PDO&page=bindParam

regards,
Lukas

Jared Williams

21 years ago

>      $sql = "insert into $table ($col_list) values ($bind_list)";

Can I just point out that you've just negated the whole reason for having parameters in the first place, imo.

$table is just as vulnerable to an SQL injection attack, as any of the parameters where before we had parameter binding.

Jared

Lukas Smith

21 years ago

Jared Williams wrote:

>>     $sql = "insert into $table ($col_list) values ($bind_list)";
>
>
> Can I just point out that you've just negated the whole reason for having parameters in the first place, imo.

uhm the point of prepared queries is not to eliminate sql injection.
thats just an added benefit

> $table is just as vulnerable to an SQL injection attack, as any of the parameters where before we had parameter binding.

you are assuming that $table has not bee sanitized, which seems quite
unlikely to me that its even going to be a variable controled by user
input in the first place.

regards,
Lukas

Thies C . Arntzen

21 years ago

Am 25.03.2005 um 14:36 schrieb Jared Williams:

> Can I just point out that you've just negated the whole reason for
> having parameters in the first place, imo.

huh? just 'cuase you dislike my php-code you question the "value" of
bind alltogether?

>
> $table is just as vulnerable to an SQL injection attack, as any of the
> parameters where before we had parameter binding.
>

a) there's no real way to bind table-names or column-names.
b) binding is not only good against sql-injection but also for speed
(if the driver supports native-bind).
c) i (personally) would usually not pass unchecked user-data for table
or column-names.

tc

Jared Williams

21 years ago

> Am 25.03.2005 um 14:36 schrieb Jared Williams:
> > Can I just point out that you've just negated the whole reason for
> > having parameters in the first place, imo.
>
> huh? just 'cuase you dislike my php-code you question the
> "value" of bind alltogether?
>
> >
> > $table is just as vulnerable to an SQL injection attack, as
> any of the
> > parameters where before we had parameter binding.
> >
>
> a) there's no real way to bind table-names or column-names.

I know, I put in request that was rejected http://pecl.php.net/bugs/bug.php?id=3442

> b) binding is not only good against sql-injection but also
> for speed (if the driver supports native-bind).

Security is far far more important than speed.
Plus you've already throw away the speed by building the SQL on fly,
so its probably slower than building the SQL with the values embedded.

> c) i (personally) would usually not pass unchecked user-data
> for table or column-names.

Perhaps not, but someone else may follow your example.

Jared

Thies C . Arntzen

21 years ago

Jared Williams wrote:

>
>
>>Am 25.03.2005 um 14:36 schrieb Jared Williams:
>>
>>>Can I just point out that you've just negated the whole reason for
>>>having parameters in the first place, imo.
>>
>>huh? just 'cuase you dislike my php-code you question the
>>"value" of bind alltogether?
>>
>>
>>>$table is just as vulnerable to an SQL injection attack, as
>>
>>any of the
>>
>>>parameters where before we had parameter binding.
>>>
>>
>>a) there's no real way to bind table-names or column-names.
>
>
> I know, I put in request that was rejected http://pecl.php.net/bugs/bug.php?id=3442

this is on a different piece of paper.

>
>
>>b) binding is not only good against sql-injection but also
>>for speed (if the driver supports native-bind).
>
>
> Security is far far more important than speed.
> Plus you've already throw away the speed by building the SQL on fly,
> so its probably slower than building the SQL with the values embedded.

blablabla... with oracle for example you will see a _massive_ speed gain
when you have repetetive sql and you use bound variables.

>
>
>>c) i (personally) would usually not pass unchecked user-data
>>for table or column-names.
>
>
> Perhaps not, but someone else may follow your example.
>

have you read my code?

$db->insert('some_table', array('name' => $name, 'age' => $age));

if you follow my example you are safe!

jared, i fail to understand your "contribution" to the real subject ("do
we need a colon in from of every bound variable or not").

re, tc

Jared Williams

21 years ago

> jared, i fail to understand your "contribution" to the real
> subject ("do we need a colon in from of every bound variable or not").

I was pointing out your design was flawed. Which if was made more secure, like the following, the colons become less of an issue.


$db->insert('some_table', array(':name' => $name, ':age' => $age));

class ...
{
protected $tableInserts = array(
'some_table' => 'INSERT INTO some_table VALUES(:name, :age)'
);

function insert($table, $values)
{

if (isset($this->tableInserts[$table]))
{
$stmt = $this->prepareStatement($this->tableInserts[$table]);
$stmt->execute($values);
}
else
{
... Error...
}
}
}

Thies C . Arntzen

21 years ago

Am 25.03.2005 um 17:40 schrieb Jared Williams:

>> jared, i fail to understand your "contribution" to the real
>> subject ("do we need a colon in from of every bound variable or not").
>
> I was pointing out your design was flawed. Which if was made more
> secure, like the following, the colons become less of an issue.
>

my point is that on internals@ we usually discuss developing php and
not developing *with* php -

re, tc

Jared Williams

21 years ago

>
> my point is that on internals@ we usually discuss developing
> php and not developing *with* php -
>

Poor practices and insecure code should never go unchallenged anywhere, unless its cleared marked as such.

Jared

Johannes Schlueter

21 years ago

<posted & mailed>

Jared Williams wrote:

>> my point is that on internals@ we usually discuss developing
>> php and not developing *with* php -
>>
>
> Poor practices and insecure code should never go unchallenged anywhere,
> unless its cleared marked as such.

As Thies wrote: This list is for discussing the development _of_ PHP.
Therefore it's important to have samples which are showing the problem not
the whole stuff around it - such a thing won't be readable anymore and the
readers would spend much more time for understanding the code than for
discussing the problem. Everybody on this list should have enough knowledge
about checking input data and these things. The only thing one might do is
to add a disclaimer stating that the code might be unsafe but some dude
might still ignore this warning and the others get bored so it won't change
anything.

I hope we can get back to the point,
johannes

--
Johannes Schlüter                                  Mayflower GmbH / ThinkPHP
http://thinkphp.de                                   http://blog.thinkphp.de

Andrey Hristov

21 years ago

well said Johannes!

Johannes Schlueter wrote:

Marcus Boerger

21 years ago

Hello Thies,

Friday, March 25, 2005, 1:55:30 PM, you wrote:

> hi,

> pdo hat it's own query-parser, named variables are prefixed with a
> colon... so far - so nice...

> i have a function called insert which is called like this:

> $db->insert('some_table', array('name' => $name, 'age' => $age));

> it's implemented like this:

> function insert($table, $values) {
>      $column_names = array_keys($values);
>      $col_list = implode(', ', $column_names);
>      $bind_list = implode(', :', $column_names);

>      $sql = "insert into $table ($col_list) values ($bind_list)";

>      $stmt = $this->prepareStatement($sql);

>      // works:
>      // this is how it has to be done..
>      //
>      foreach ($values as $col => $val) {
>          $stmt->bindParam(":$col", $values[ $col ]);
>      }
>      $stmt->execute();

>      // doesn't work
>      // and this is how i would like it instead
>      //
>      # $stmt->execute($values);
> }

> and this is 'cause pdo _expects_ the user to prefix the bound variables
> with a colon. grr...

> ppls, lets change it before it's too late. this "tiny bit" makes binding
> harder than it should be, and we all know and understand that all user
> of php should use bound variables all the time!

> suggestion: auto-trim a leading colon from the internal binding tables.
> that way "oldish" code would not break...

> re, thies

> PS: looking into that right now - hopefully "patch follows";-)

We could easily add this behavior and i think it makes somewhat sense.
It just looks a bit like __wakepup/__sleep. The only 'but' is that i
suggest the behavior has its own method like 'bindParamArray'. If you
cannot work out a patch feel free to contact me. Maybe i'll find some
time during the conf.

--
Best regards,
Marcus                            mailto:mail@marcus-boerger.de

Thies C . Arntzen

21 years ago

Am 31.03.2005 um 17:28 schrieb Marcus Boerger:

> Hello Thies,
>
> Friday, March 25, 2005, 1:55:30 PM, you wrote:
>
>> and this is 'cause pdo _expects_ the user to prefix the bound
>> variables
>> with a colon. grr...
>
>> ppls, lets change it before it's too late. this "tiny bit" makes
>> binding
>> harder than it should be, and we all know and understand that all user
>> of php should use bound variables all the time!
>
>> suggestion: auto-trim a leading colon from the internal binding
>> tables.
>> that way "oldish" code would not break...
>
>
>> re, thies
>
>> PS: looking into that right now - hopefully "patch follows";-)
>
>
> We could easily add this behavior and i think it makes somewhat sense.
> It just looks a bit like __wakepup/__sleep. The only 'but' is that i
> suggest the behavior has its own method like 'bindParamArray'. If you
> cannot work out a patch feel free to contact me. Maybe i'll find some
> time during the conf.

hey marcus!

i don't quite understand the relation to sleep/wakeup here (i'm not
reading all the php lists, maybe you could point me to the right
thread?)

adding a new 'bindParamArray' with different semantics doesn't really
make things easier. and i think we should have once consistent
parameter-bind interface:

- a method bindParam
- an optinal associative array passed to execute.

all those functions expect to bind a <parameter name> to a statement.
this <parameter name> should work both with or without a leading colon.

by the way (just checked) - this is how the "native" oracle-oci binding
functions work. so oracle decided to make the colon in front of the
placeholder for bind optional. at the end of the day you can only bind
to a placeholder defined in your sql-statement. in the sql you have to
"mark" the placeholder by prepending a colon - but for the bind you
really don't need the colon. really!

so - again - lets make the colon *optional* - everywhere we allow
binding in PDO!

re, thies

Wez Furlong

21 years ago

Yep, will do.

On Apr 1, 2005 3:13 AM, Thies C. Arntzen <thies@thieso.net> wrote:

Lukas Smith

21 years ago

Wez Furlong wrote:

> Yep, will do.

I dont want to pretend that this change should open the flood gates for
radical changes. However there are still a few minor points beyond the
bindParam() that may need minor adjustments. Some of them can be added
later of course ..

1) DSN http://oss.backendmedia.com/index.php?area=PDO&page=DSN
It would be nice to be able as an array and for the string format it
would be nice to also have the option of using the established PEAR DSN
format

2) exec() http://oss.backendmedia.com/index.php?area=PDO&page=exec
It may be nice to have a simple one shot prepared query to be able to do
a prepare/execute in one call.

3) setAttribute/getAttribute
http://oss.backendmedia.com/index.php?area=PDO&page=setAttribute
Some of the "attributes" are actually more like options that can be
defined by the user. Others are defined by the client and yet others are
defined by the server and cannot be changed by the user. I think atleast
the user definable options should be moved to a separate
setOption/getOption set of methods.

4) minor API stuff in the fetch methods
I think it would be clearer to call the fetch() method fetchRow()
because that is what the method does. I also dont think we should
default to PDO_FETCH_BOTH. It may also be a nice touch towards
PEAR::[M]DB[2] to rename the fetchSingle() method to fetchOne(). Its
also more clearer since fetchSingle() might as well imply a single row.
Fetching a single row should probably be handled by a fetchCol() method.

regards,
Lukas

Lukas Smith

21 years ago

Lukas Smith wrote:

> Wez Furlong wrote:
>
>> Yep, will do.
>
>
> I dont want to pretend that this change should open the flood gates for
> radical changes. However there are still a few minor points beyond the
> bindParam() that may need minor adjustments. Some of them can be added
> later of course ..

I also noticed that bindParam() and bindColumn() are 1-indexed, which
will raise inconviniences with people using arrays for example to stored
the numeric indexes, similar in how having to add the ":" was a problem
for people using arrays to store the string parameters.

Beyond that rowCount() is more traditionally affectedRows() etc which
needlessly break with existing method names in both php core extensions
and PEAR packages. I would really like to avoid adding even more names
and instead sticking with picking names from this already fairly vast
name pool. It might even be a good idea to pick from the names most
commonly used. I have attached a reference to the document I once
drafted for this exact purpose [1]. I dont know where the names in PDO
originate from. They dont seem to match the current oci8 extension (I
first assumed these originated from the fact that alot of the API
matured during your work on the oracle driver), but this does not seem
to be the case.

I know I am getting on your nerves Wez, but I really think that the API
should at this stage still be open to optimizing the API. While its no
longer beta I still think its a bit premature for comments like these:

"There are releases on pecl.php.net, and there are (a few) people
running these in production. In addition, it's been advertised as
working this way for over a year."

How about sitting down together at php|tropics (I didnt plan on going to
a'dam .. dunno about you Wez .. but if necessary I will travel to a'dam
for a day) and simply going through each of the methods one by one.
Looking at what kind of methods names we currently are using for the
same or similar things in any of the current projects on php.net and
make any adjustments then?

[1] http://marc.theaimsgroup.com/?l=php-dev&m=106440782305693&w=2

regards,
Lukas

Wez Furlong

21 years ago

On 4/25/05, Lukas Smith <lsmith@php.net> wrote:

> I also noticed that bindParam() and bindColumn() are 1-indexed, which
> will raise inconviniences with people using arrays for example to stored
> the numeric indexes

Passing an array with numeric indices to execute() expects 0 based
indices, to cater for this case.  Manually calling a bind function
expects 1 based indices, as this is the convention for the majority of
database APIs.

> Beyond that rowCount() is more traditionally affectedRows() etc which
> needlessly break with existing method names in both php core extensions
> and PEAR packages.

Look, PDO is new.  It's not breaking anything.  Stop claiming that it
is breaking things; it doesn't touch your code; it doesn't change the
external API of your code.  It doesn't make MDB, PEAR DB or any other
code change its names.  It's just different.  Deliberately.

> How about sitting down together at php|tropics

Sure.

> (I didnt plan on going to
> a'dam .. dunno about you Wez .. but if necessary I will travel to a'dam
> for a day)

That's a hell of a commute for me.

> and simply going through each of the methods one by one.
> Looking at what kind of methods names we currently are using for the
> same or similar things in any of the current projects on php.net and
> make any adjustments then?

Maybe, but I don't see a compelling reason to change the names, or I
would have done it already.

--Wez.

Lukas Smith

21 years ago

Wez Furlong wrote:

>>How about sitting down together at php|tropics
>
>
> Sure.

>>and simply going through each of the methods one by one.
>>Looking at what kind of methods names we currently are using for the
>>same or similar things in any of the current projects on php.net and
>>make any adjustments then?
>
>
> Maybe, but I don't see a compelling reason to change the names, or I
> would have done it already.

Ok great!

We will see how much interest there is from others. Alot of these things
I have already run by you so if its just me and you there is little
reason to bother :-)

regards,
Lukas

Derick Rethans

21 years ago

On Mon, 25 Apr 2005, Wez Furlong wrote:

> On 4/25/05, Lukas Smith <lsmith@php.net> wrote:
> > I also noticed that bindParam() and bindColumn() are 1-indexed, which
> > will raise inconviniences with people using arrays for example to stored
> > the numeric indexes
>
> Passing an array with numeric indices to execute() expects 0 based
> indices, to cater for this case.  Manually calling a bind function
> expects 1 based indices, as this is the convention for the majority of
> database APIs.

But it's totally not what PHP does now. Everything is 0 based so why
make it *harder* for users by requiring a 1-based index here?

regards,
Derick

--
Derick Rethans
http://derickrethans.nl | http://ez.no | http://xdebug.org

Marcus Boerger

21 years ago

Hello Thies,

Friday, April 1, 2005, 3:13:10 AM, you wrote:

> Am 31.03.2005 um 17:28 schrieb Marcus Boerger:

>> Hello Thies,
>>
>> Friday, March 25, 2005, 1:55:30 PM, you wrote:
>>
>>> and this is 'cause pdo _expects_ the user to prefix the bound
>>> variables
>>> with a colon. grr...
>>
>>> ppls, lets change it before it's too late. this "tiny bit" makes
>>> binding
>>> harder than it should be, and we all know and understand that all user
>>> of php should use bound variables all the time!
>>
>>> suggestion: auto-trim a leading colon from the internal binding
>>> tables.
>>> that way "oldish" code would not break...
>>
>>
>>> re, thies
>>
>>> PS: looking into that right now - hopefully "patch follows";-)
>>
>>
>> We could easily add this behavior and i think it makes somewhat sense.
>> It just looks a bit like __wakepup/__sleep. The only 'but' is that i
>> suggest the behavior has its own method like 'bindParamArray'. If you
>> cannot work out a patch feel free to contact me. Maybe i'll find some
>> time during the conf.

> hey marcus!

> i don't quite understand the relation to sleep/wakeup here (i'm not
> reading all the php lists, maybe you could point me to the right
> thread?)

> adding a new 'bindParamArray' with different semantics doesn't really
> make things easier. and i think we should have once consistent
> parameter-bind interface:

> - a method bindParam
> - an optinal associative array passed to execute.

> all those functions expect to bind a <parameter name> to a statement.
> this <parameter name> should work both with or without a leading colon.

> by the way (just checked) - this is how the "native" oracle-oci binding
> functions work. so oracle decided to make the colon in front of the
> placeholder for bind optional. at the end of the day you can only bind
> to a placeholder defined in your sql-statement. in the sql you have to
> "mark" the placeholder by prepending a colon - but for the bind you
> really don't need the colon. really!

> so - again - lets make the colon *optional* - everywhere we allow
> binding in PDO!

> re, thies

hmmm,

this is just a copy'n'paste mistake somehow....

--
Best regards,
Marcus                            mailto:mail@marcus-boerger.de

Andi Gutmans

21 years ago

At 10:13 AM 4/1/2005 +0200, Thies C.Arntzen wrote:

>all those functions expect to bind a <parameter name> to a statement. this
><parameter name> should work both with or without a leading colon.
>
>by the way (just checked) - this is how the "native" oracle-oci binding
>functions work. so oracle decided to make the colon in front of the
>placeholder for bind optional. at the end of the day you can only bind to
>a placeholder defined in your sql-statement. in the sql you have to "mark"
>the placeholder by prepending a colon - but for the bind you really don't
>need the colon. really!
>
>so - again - lets make the colon *optional* - everywhere we allow binding
>in PDO!

Yeah, that makes a lot of sense to me too. Actually, (pardon my ignorance)
I was surprised to find that binding with name only isn't the standard method.
Andi