CI David-CSD - 18 April 2007 11:07 AM

That’s not a bug. You should be checking to make sure you have a user_id before executing any statements. The framework did exactly what it says it will do.

It isn’t a bug and it isn’t a vulnerability.

I’d watch how you use choose your words.  People are jumping off the Ruby on Rails ship (and rightfully so) b/c DHH seems extremely bitter whenever you point out a problem with RoR.

How do other implementations of Active Record handle this?  If the others do what the OP expected, then maybe CI should do that to, or at least clearly document this behavior.

This is yet another reason why I refuse to use Active Record.  Too much “voodoo magic” going on in the background

I posted on your blog entry about this, but I’ll put it here also

While I’ll start by saying that use, this does appear to be a bug in the ActiveRecord class used by CI, there are ways to avoid this from happening.

First off, this issue should have never made an impact on you, other than finding the bug and fixing it. It should have never affected your users. You should have made your code change in a development environment, and tested them as a developer. Then pushed those changes to a staging site, for user acceptance testing with multiple users who are not developers, and then once your confirm the changes were valid, made the changes live. This should be your standard development cycle, and never be circumvented for even the tiniest changes, to avoid this from happening.

Secondly, you may want to examine your data validation practices. Most websites/applications are multi-tiered. You have the user interface, the backend, and the database. You validation practices should look like

1. (optional) Interface validation. Speaking of websites, this would be javascript to validate form entries before allowing the submission. This is just a little nicer than kicking responses from the backend, as it cuts down client to server communication.

2. Backend data validation. Even if you validate at the interface, validate the data when it reaches the server. You have no way of guaranteeing where the data comes from.

3. When passing from your script/run time engine to the database, validate again. Just to avoid cases like these. While much of your data has come direct from the source, things like your user id may have been generated as part of the algorithm creating the query. So validate the data going into the query. Though I imagine you were counting on ActiveRecord for this, and honestly, I would too.

Lastly, you may to consider how you put your words together when putting together blog entries such as these. Honestly, I was put off by your colorful language. I’m a sysadmin with over 10 years of experience, so believe me, I fully appreciate the level of frustration you get when you have situations like these. However, the way you wrote this post is much better suited for behind closed door conversations and letting out steam with your co-workers, not for the world (and possible future potential employers) to see. Just a thought.

Good luck with your future projects.

neolyphik - 20 April 2007 06:28 AM

CI David-CSD - 18 April 2007 11:07 AM

That’s not a bug. You should be checking to make sure you have a user_id before executing any statements. The framework did exactly what it says it will do.

It isn’t a bug and it isn’t a vulnerability.

I’d watch how you use choose your words.  People are jumping off the Ruby on Rails ship (and rightfully so) b/c DHH seems extremely bitter whenever you point out a problem with RoR.

How do other implementations of Active Record handle this?  If the others do what the OP expected, then maybe CI should do that to, or at least clearly document this behavior.

This is yet another reason why I refuse to use Active Record.  Too much “voodoo magic” going on in the background

ActiveRecord is supposed to handle things like cleaning up your data before putting it into the DB. Escaping and such. If you want to do it yourself, that’s perfectly fine too. Lots of people prefer finer control over their data within their code logic. However, I do agree this is a bug in activerecord that should be fixed for those that use it.

3rdRRAdmin - 18 April 2007 01:47 PM

I am surprised nobody has asked for ActiveRecord to generate ‘where userid IS NULL’ instead of ‘where userid=’ since NULL values actually can be useful while designing automated processes and people may want that possibility available. In the case of updating passwords, I would hope that no users had a null userid, and the query just wouldn’t occur.

My vote is for the use of ‘IS NULL’ as well, as it is the proper syntax for null values. 

I think this should be addressed in the next update as the user guide seems to emphasize the use of the Active Record class as a convenient way to generate queries.  As such, it should properly handle these specific cases.

I gather that you all agree that this is a bug. Question is has this been fixed ? Is there a bug tracking system for CI and if there is has anyone submitted this ?

I ran some tests with PHP, and, unfortunately, there’s no way from within a function (that I could find) to check to see if a parameter passed in was SET to null or simply never set at all.  That is, isset == is_null from this particular context.  Don’t know if this is a problem or not for you all, but personally I’d only want it to throw in an IS NULL if I literally pass in null.

p.s. they need to make an is_set and deprecate isset

His,

Very surprised not to find a bug fix in the new 1.5.4 release I reported the bug (http://codeigniter.com/bug_tracker/bug/2541/) as suggested by Derek Allard (http://codeigniter.com/forums/viewthread/56353/P15/#277554).

To be honest, I think it’s just unexpected behaviour, not a bug. I’d never put the contents of a variable directly into an SQL query without validating it first. Failing to validate input data is just asking for trouble.

Fair play if the functionality should be changed to something better, anyway, but I wouldn’t be so quick to condemn the developers for something which is not entirely their fault.

(Oops, didn’t realise this was 3 pages long. That’s going to seem a little out of context!)

Spotted the same thing. Quick solution?

The question is what to use as a default “do not use” value? Pretty much everything, true/false, null, ‘’, can be used in a valid database query, so how do we figure out whether the user is writing a custom query or is using the ActiveRecord? That is the real question…

I think this could be a quick solution, please verify!

If the value is NULL, and the key has no operator, then check the key against ‘IS NULL’.

The only problem is that is use the operator ‘IN ()’ a lot and the method _has_operator() doesn’t support it out of the box, so you should add it.

I’m not sure about everything else but i always put $this->db->limit(1); on any updates as this is something that can happen, by limiting it to 1 you limit the problem, i’d like to see a nice fix, i don’t think its a bug, but i think it could be fixed easily.

What is the current status of this? Has it been “fixed” as of v1.63? Is it considered a bug by the CI team? Is there an official recommendation on how best to handle this situation? If it is not fixed will it be in the upcoming release?