Not escaping the value in Active Record WHERE functions

[eluser]ebynum[/eluser]
Several Active Record functions (including at least _where() and set()) have a parameter called $escape that provides a way to keep the value of the AR operation from being surrounded by apostrophes (to allow things like setting a timestamp field to NOW(), and presumably to allow subselects in the WHERE clause).

In set, the code looks like this:

Code:

if ($escape === FALSE)
{
    $this->ar_set[$this->_protect_identifiers($k)] = $v;
}
else
{
    $this->ar_set[$this->_protect_identifiers($k)] = $this->escape($v);
}

and in _where, like this:

Code:

if ($escape === TRUE)
{
    // exception for "field<=" keys
    if ($this->_has_operator($k))
    {
        $k =  preg_replace("/([A-Za-z_0-9]+)/", $this->_protect_identifiers('$1'), $k);
    }
    else
    {
        $k = $this->_protect_identifiers($k);
    }
}

if ( ! $this->_has_operator($k))
{
    $k .= ' =';
}

$v = ' '.$this->escape($v);

These are obviously VERY different. In the SET operation, the key has _protect_identifiers called on it no matter what, and the value is only escaped if the $escape flag is set.

In the WHERE operation, the key only has _protect_identifiers called on it when the $escape flag is set, and the value is escaped no matter what.

Am I correct to assume that this is a bug and that the intended behavior is that _protect_identifiers should always be called, and that it is the escaping that is supposed to be controlled by the $escape flag?

[eluser]barbazul[/eluser]
the third parameter is about escaping table names and field names.... not the values.

I'm still investigating some other bugs I found around this issue but here's the solution to your problem:

Code:

$this->db->where('date > (SELECT date FROM `stories` WHERE id = $id)');
$my_story = $this->db->get('stories');

using a single parameter in the where method produces a where clause as it is passed.
you should manually call escape() on $id though

[eluser]barbazul[/eluser]
I'll continue the discussion here instead of the bug tracker

First of all I am not saying it's the way I like it or even the way it makes more sense... I'm just saying that the bug tracker was created to check that things work as they're documented so that's what I put my effort into

On the other hand, it makes some degree of sense:

where() has mainly 3 ways to be called:

1. One parameter string: this is the case you need to use in your example an is intended for "complicated" where clauses

2. Parameter field-value pair: This is the simplest case scenario where you need a field to be compared against a value. The value should always be escaped.

3. One parameter hash: This is an extension of the second case which takes multiple field-value pairs in an assosiative array

On cases 2 and 3 the value passed should always be escaped

When working with the set() method you always refer to fields and the values that they will be set to, so the field names should always be protected.
On the other hand the values passed might not be literal values (you can set one field to be the sum of other two fields and crazy things like that) so you might need to have the values unescaped.

[eluser]ebynum[/eluser]
[quote author="barbazul" date="1207853180"]
When working with the set() method you always refer to fields and the values that they will be set to, so the field names should always be protected.
On the other hand the values passed might not be literal values (you can set one field to be the sum of other two fields and crazy things like that) so you might need to have the values unescaped.[/quote]

How is that any different from this situation:

Code:

SELECT * FROM books WHERE publishDate > NOW()

Code:

UPDATE books SET publishDate = NOW() WHERE id = 0

Both of these have a two-value comparison, and they have an identical second parameter - NOW(). Why should I have to use Active Record as follows:

Code:

$this->db->where('publishDate > NOW()');
//    instead of
$this->db->where('publishDate >','NOW()',FALSE);

when I can do

Code:

$this->db->set('publishDate','NOW()',FALSE);

[eluser]barbazul[/eluser]
It's a good point.

I'll assume that they simply can't cover all the possibilities.

In the particular cases of SQL keywords (such as NOW() ) which are specific to each sql engine it gets even more complicated to cover them all up.

that's the reason I try to never use SQL keywords in queries:

Code:

$this->db->where('publishDate >',date("Y-m-d"));

[eluser]ebynum[/eluser]
I definitely understand that, and see the issue with using SQL Engine specific keywords ...

This, however, results in the database looking at the time relative to the web server, and not relative to the database server itself. Obviously in the case of same-system servers, this is a relative non-issue, and in the case of multi-system servers, it is an issue that can (and probably should) be mitigated with something like ntp ... and finally, I hadn't considered that NOW() wasn't part of the official SQL spec.

That said - I'll go back to my earlier question.

Why is it "guaranteed" that the user will want to escape their fields/tables in SET operations, but not in WHERE operations.
Why is it "guaranteed" that the user will want to escape their values in WHERE operations, but not in SET operations?

[quote author="barbazul" date="1207854600"]On the other hand the values passed might not be literal values (you can set one field to be the sum of other two fields and crazy things like that) so you might need to have the values unescaped.[/quote]
And in a WHERE operation, the values passed can also be the sum of two other fields, ...

I understand that you're not so much disagreeing with me here as talking about the status quo, so please don't think any of my questions are intended to be accusatory.

[eluser]barbazul[/eluser]
Nothing is "guaranteed". I just put myself in the shoes of whoever first wrote those methods to see the logic they've used.

In the case of SET operations, I can't think of something like:

Code:

SET CONCAT(field1,field2) = 'some value'

so you'll always be passing real field names in the first parameter (or array key) and they get correctly escaped
but you might be passing something weird on the value like

Code:

SET field1 = (field2+field3)/2

in which case you might or not want the value to be escaped


In the case of WHERE it gets tricker as the SQL syntax is more flexible, so the number of possible combinations is enormous.
The solution the guys at EllisLab came up with is pretty flexible, and they say in the docs:

Note: All values passed to this function are escaped automatically, producing safer queries.

So basically that's the way they decided it should work.
It might not be the most ellegant solution but it works fine in the vast majority of cases and if you want to do something that hasn't been covered up, you can write your own clause:

Code:

$this->db->where("date > NOW()");

and you can always write your entire query if you don't like how they get generated by CI:

$this->db->query("UPDATE mytable SET name='barbazul', `order`=3 WHERE date > NOW() AND `status`='active'");

Now, on a different discussion topic I'd really like to see some important keywords to be implemented as part of CI. NOW() is definitely one of them.
I've previously worked on the implementation of random ordering, and though I can still be improved It got released.
So as you see whatever it is that you don't like or feel there is room for improvement, you can always propose a different solution and, if the community likes it it will eventually show up in the core

Regarding the date() example... it was just to illustrate how I always try yo keep the specifics of MySQL out of the way.

[eluser]Derek Allard[/eluser]
Thanks both for all your attention here.

ebynum, if

Code:

$this->db->where('date >',"(SELECT date FROM `stories` WHERE id = $id)",FALSE);
$my_story = $this->db->get('stories');

produced

Code:

SELECT * FROM (`stories`) WHERE date >(SELECT date FROM `stories` WHERE id = 3)

would you consider it solved? Without getting into nitty-gritty details, AR isn't able to handle every possible query, but I agree with you that FALSE in that case should prevent the quotes around the "value".

I'm labeling the bug report for now as "not a bug", however I've introduced a fix to prevent quotes around where clauses with "FALSE" enabled.