Part of the EllisLab Network
   
 
Not escaping the value in Active Record WHERE functions
Posted: 03 April 2008 01:02 PM   [ Ignore ]  
Summer Student
Total Posts:  9
Joined  01-25-2008

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:

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:

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?

Profile
 
 
Posted: 09 April 2008 11:28 PM   [ Ignore ]   [ # 1 ]  
Lab Assistant
Avatar
RankRank
Total Posts:  153
Joined  08-11-2006

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:

$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

 Signature 

Ruby on Rails migrations in codeigniter

Profile
 
 
Posted: 10 April 2008 08:46 AM   [ Ignore ]   [ # 2 ]  
Lab Assistant
Avatar
RankRank
Total Posts:  153
Joined  08-11-2006

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.

 Signature 

Ruby on Rails migrations in codeigniter

Profile
 
 
Posted: 10 April 2008 08:59 AM   [ Ignore ]   [ # 3 ]  
Summer Student
Total Posts:  9
Joined  01-25-2008
barbazul - 10 April 2008 08:46 AM

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.

How is that any different from this situation:

SELECT * FROM books WHERE publishDate > NOW()


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:

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


when I can do

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

Profile
 
 
Posted: 10 April 2008 09:10 AM   [ Ignore ]   [ # 4 ]  
Lab Assistant
Avatar
RankRank
Total Posts:  153
Joined  08-11-2006

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:

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

 Signature 

Ruby on Rails migrations in codeigniter

Profile
 
 
Posted: 10 April 2008 09:40 AM   [ Ignore ]   [ # 5 ]  
Summer Student
Total Posts:  9
Joined  01-25-2008

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?

barbazul - 10 April 2008 09:10 AM
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.

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.

Profile
 
 
Posted: 10 April 2008 10:25 AM   [ Ignore ]   [ # 6 ]  
Lab Assistant
Avatar
RankRank
Total Posts:  153
Joined  08-11-2006

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:

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

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:

$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.

 Signature 

Ruby on Rails migrations in codeigniter

Profile
 
 
Posted: 12 May 2008 01:31 PM   [ Ignore ]   [ # 7 ]  
Administrator
Avatar
RankRankRankRankRankRank
Total Posts:  6593
Joined  03-23-2006

Thanks both for all your attention here.

ebynum, if

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


produced

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.

 Signature 

DerekAllard.com - CodeIgniter, ExpressionEngine, and the World of Web Design
BambooInvoice - Open Source, CodeIgniter powered invoicing.

Profile
MSG
 
 
   
 
 
Post Marker Legend
New Topic New posts Hot Topic Hot Topic with new posts New Poll New Poll Moved Topic Moved Topic Sticky Topic Sticky topic
Old Topic No new posts Hot Old Topic Hot Topic with no new posts Old Poll Old Poll Closed Topic Closed Topic Announcement Announcements
Theme
Change Theme
Visitor Statistics
The most visitors ever was 719, on June 06, 2008 10:16 AM
Total Registered Members: 62662 Total Logged-in Users: 35
Total Topics: 77198 Total Anonymous Users: 5
Total Replies: 416742 Total Guests: 238
Total Posts: 493940    
Members ( View Memberlist )