CI 1.7 - Error on querys (quotes escaping problems)

[eluser]auston[/eluser]
Alright - we worked out a patch - We posted about it on our blog

Download the entire CodeIgniter 1.7.0 framework with the fix here

[eluser]skattabrain[/eluser]
Thanks for the effort auston ... not sure I'm in quite yet ... but if it gets hot me to need 1.7 ... i'll take a look into this.

I try to keep my core as virgin as possible.

[eluser]mycroes[/eluser]
[quote author="auston" date="1231895703"]Alright - we worked out a patch - We posted about it on our blog

Download the entire CodeIgniter 1.7.0 framework with the fix here[/quote]

As I already commented on your blog, there's no patch there, just perhaps a patched CI install. You're a first-time poster, referring to yourself as 'we', linking to a blog with not a lot of real content, any scammer would do better. So for that matter, I'm just assuming that if I wouldn't question your code changes, I shouldn't question the intentions of scammers either. So for me that's not gonna happen. Write a proper patch, file a bug and show you're at least a bit competent.
Regards,

Michael

[eluser]auston[/eluser]
Mycroes

Thanks for the rude feedback - I suppose. I was just trying to contribute. I don't know the proper procedure, so I apologize for my incompetence. I suppose it is a patched CI install.

Whatever the case, I will file a bug as per your request. At my convenience, due to my busy schedule... you know... scamming people...

[eluser]auston[/eluser]
[quote author="mycroes" date="1231911619"][quote author="auston" date="1231895703"]Alright - we worked out a patch - We posted about it on our blog

Download the entire CodeIgniter 1.7.0 framework with the fix here[/quote]

As I already commented on your blog, there's no patch there, just perhaps a patched CI install. You're a first-time poster, referring to yourself as 'we', linking to a blog with not a lot of real content, any scammer would do better. So for that matter, I'm just assuming that if I wouldn't question your code changes, I shouldn't question the intentions of scammers either. So for me that's not gonna happen. Write a proper patch, file a bug and show you're at least a bit competent.
Regards,

Michael[/quote]

OK So, here we go.

The problem is in SQL selects in ActiveRecord - they're not being properly escaped. I've filed a bug - http://codeigniter.com/bug_tracker/bug/6452/

The real problem resides in the database/drivers/mysql/mysql_driver.php (or mssql, mysqli, sqlite, etc etc)

Currently it is:

Code:

function _escape_identifiers($item)
    {
        if ($this->_escape_char == '')
        {
            return $item;
        }
    
        if (strpos($item, '.') !== FALSE)
        {
            $str = $this->_escape_char.str_replace('.', $this->_escape_char.'.'.$this->_escape_char, $item).$this->_escape_char;            
        }
        else
        {
            $str = $this->_escape_char.$item.$this->_escape_char;
        }
        
        // remove duplicates if the user already included the escape
        return preg_replace('/['.$this->_escape_char.']+/', $this->_escape_char, $str);
    }

The above code escapes everything, table alias and field identifier no matter what. It should be watching for a wildcard, because, in my experience you cannot legitimately escape a wildcard.

Here are 3 possible patches/replacements:

1.

Code:

function _escape_identifiers($item)
    {
        if ($this->_escape_char == '')
        {
            return $item;
        }

        if (strpos($item, '.') !== FALSE && strpos($item, '*') != TRUE)
        {
            $str = $this->_escape_char.str_replace('.', $this->_escape_char.'.'.$this->_escape_char, $item).$this->_escape_char;    
        }
        elseif (strpos($item, '*'))
        {
            //find out if we're doing database.table.field or just table.field and work accordingly
            $item_parts = explode('.', $item);
            if(count($item_parts) > 2)
            {
                $str = $this->_escape_char.str_replace('.', $this->_escape_char.'.'.$this->_escape_char, $item);

                //breaking it up to remove the last back tick before the asterisk
                $item_parts = explode('.', $str);

                //removing the last back tick before the asterisk
                $item_parts[2] = str_replace($this->_escape_char, '', $item_parts[2]);

                //putting it back together
                $str = implode('.', $item_parts);
            }
            else
            {
                $str = $this->_escape_char.str_replace('.', $this->_escape_char.'.', $item);
            }
        }
        else
        {
            $str = $this->_escape_char.$item.$this->_escape_char;
        }

        // remove duplicates if the user already included the escape
        return preg_replace('/['.$this->_escape_char.']+/', $this->_escape_char, $str);
    }

2.

Code:

function _escape_identifiers($item)
{
    if ($this->_escape_char == '')
    {
        return $item;
    }

    if (strpos($item, '.') !== FALSE)
    {
        $parts = explode('.', $item);
        foreach ($parts as $key => $val)
        {
            if ( $val != '*')
            {
                $parts[$key] = $this->_escape_char.$val.$this->_escape_char;
            }
        }
        $str = implode('.', $parts);    
    }
    else
    {
        $str = $this->_escape_char.$item.$this->_escape_char;
    }
    
    // remove duplicates if the user already included the escape
    return preg_replace('/['.$this->_escape_char.']+/', $this->_escape_char, $str);
}

3. (This is just like 2, but cut down to remove stuff that we don't think is needed)

Code:

function _escape_identifiers($item)
{
    if ($this->_escape_char == '')
    {
        return $item;
    }

    $parts = explode('.', $item);
    foreach ($parts as $key => $val)
    {
        if ( $val != '*')
        {
            $parts[$key] = $this->_escape_char.$val.$this->_escape_char;
        }
    }
    $str = implode('.', $parts);    
    
    // remove duplicates if the user already included the escape
    return preg_replace('/['.$this->_escape_char.']+/', $this->_escape_char, $str);
}

For clarity - WE is inclusive to myself and the other developer here - Tom. Thanks for your polite guidance and I hope you can find the graciousness in your heart to forgive our incompetence.

--
Auston

[eluser]mycroes[/eluser]
[quote author="auston" date="1231919359"]OK So, here we go.

The problem is in SQL selects in ActiveRecord - they're not being properly escaped. I've filed a bug - http://codeigniter.com/bug_tracker/bug/6452/

The real problem resides in the database/drivers/mysql/mysql_driver.php (or mssql, mysqli, sqlite, etc etc)
...

For clarity - WE is inclusive to myself and the other developer here - Tom. Thanks for your polite guidance and I hope you can find the graciousness in your heart to forgive our incompetence.

--
Auston[/quote]
You've proven your intentions with your response. I'm sorry that I sounded a bit harsh, but there's two things I don't like with open source software:
1. People complaining but not filing bugs if they should (actually I should also have filed the bug, not just you because you're new or anything)
2. People without any skills at all wanting to contribute, making the situation worse and worse only.

I believe you don't really fit in either of these categories. Your first response was also very nice, especially the joke about your busy schedule .

Now onto the technical issue... I noticed one bug in the code you posted, someone you use $parks instead of $parts. I guess that happened while putting it on the forum and that it's not in the code itself, but if it is, that won't work. Also, I think your code doesn't fix the issue I mentioned higher up in this thread: orderby('a, b') doesn't work anymore because it doesn't get escaped properly. Anyway, I'd rather see the CodeIgniter devs fix this.

As a last word about this issue I want to say I'm moving to Zend. I tried Zend before, and already liked it, but never made something with it. Recently some issues have come up that CodeIgniter just can't handle itself, and that can only being handled by hacks. What I'm especially missing is proper class and class loading support. It's nice that CI tries to abstract that so that it forces developers to make a controller with a specified set of functions etc., but it won't even allow me to extend modules without dropping a include line to the base module somewhere where it doesn't really belong. Zend is a far more proffesional framework, and anyone looking to do a large project should see if CodeIgniter can really give what they need, without extending it, because that's what's not working as good as it should with CI.
Regards,

Michael

[eluser]skattabrain[/eluser]
:lol:

[eluser]auston[/eluser]
[quote author="mycroes" date="1231948765"][quote author="auston" date="1231919359"]OK So, here we go.

The problem is in SQL selects in ActiveRecord - they're not being properly escaped. I've filed a bug - http://codeigniter.com/bug_tracker/bug/6452/

The real problem resides in the database/drivers/mysql/mysql_driver.php (or mssql, mysqli, sqlite, etc etc)
...

For clarity - WE is inclusive to myself and the other developer here - Tom. Thanks for your polite guidance and I hope you can find the graciousness in your heart to forgive our incompetence.

--
Auston[/quote]
You've proven your intentions with your response. I'm sorry that I sounded a bit harsh, but there's two things I don't like with open source software:
1. People complaining but not filing bugs if they should (actually I should also have filed the bug, not just you because you're new or anything)
2. People without any skills at all wanting to contribute, making the situation worse and worse only.

I believe you don't really fit in either of these categories. Your first response was also very nice, especially the joke about your busy schedule .

Now onto the technical issue... I noticed one bug in the code you posted, someone you use $parks instead of $parts. I guess that happened while putting it on the forum and that it's not in the code itself, but if it is, that won't work. Also, I think your code doesn't fix the issue I mentioned higher up in this thread: orderby('a, b') doesn't work anymore because it doesn't get escaped properly. Anyway, I'd rather see the CodeIgniter devs fix this.

As a last word about this issue I want to say I'm moving to Zend. I tried Zend before, and already liked it, but never made something with it. Recently some issues have come up that CodeIgniter just can't handle itself, and that can only being handled by hacks. What I'm especially missing is proper class and class loading support. It's nice that CI tries to abstract that so that it forces developers to make a controller with a specified set of functions etc., but it won't even allow me to extend modules without dropping a include line to the base module somewhere where it doesn't really belong. Zend is a far more proffesional framework, and anyone looking to do a large project should see if CodeIgniter can really give what they need, without extending it, because that's what's not working as good as it should with CI.
Regards,

Michael[/quote]

The ActiveRecord Order_By works according to the documentation provided here - http://ellislab.com/codeigniter/user-gui...ecord.html

You need to specify how you want each column sorted:

Code:

$this->db->order_by('id ASC, user_id DESC');

P.S. I fixed that $item_parks to be $item_parts.

[eluser]Phil Norton[/eluser]
[quote author="thespy" date="1231885037"][quote author="skattabrain" date="1231880045"]i gotta say, i'm puzzled as to why we haven't seen a 1.7.1 so people can start using 1.7 without worries of this issue. Unless I'm wrong ... but is this still an issue for many?

using 1.7 on new projects, but not upgrading my past projects until this is in a core release.[/quote]
Same here, I'm still on 1.6.3 due to this.[/quote]

Me too. Would be nice to hear from an "official" person if this is close to being resolved?

[eluser]sdclee[/eluser]
This is an issue in 1.7.2 (downloaded and tested today) but there doesn't appear to be any news on it, so I thought I'd just nudge the thread a little (apologies if that constitutes unwanted noise).