We use codeigniter internally to develop our web solutions in somewhere in… net limited. Day before yesterday we suffered a terrible situation for an internal bug in code igniter which corrupted data inside some tables of our application database and then it tooks hours to find the origin of that bug, to fix it and to repair the corrupted data. Let me explain what happened.
Lets guess that we have one table named “users” with the following field
1. user_id
2. username
3. password
4. email
At some point, if you want to update the password field of this table, for a particular user, what will you write in your code?
$data = array("password"=>$new_password);
$this->db->where("user_id",$user_id);
$this->db->update("users", $data);
CodeIgniter’s ActiveRecord creates the query like the following
UPDATE users set password='{$new_password}’ where user_id='{$user_id}’;
Well, it’s ok and the quesry seems pretty fine. Now what should happen if you pass a valid user id to this code? Password of only that user will be updated. But what will happen when the passed $user_id is null?? Thats the most pathetic part that Codeigniter ActiveRecord plays. Instead of generating the following query,
UPDATE users set password='{$new_password}’ where user_id=”;
CodeIgniter’s ORM actually generates the following
UPDATE users set password='{$new_password}’ where user_id;
You find the difference of the above two queries right? one contains “where user_id=” ” and another contains just “where user_id” . Now if your backend database is MySQL and this query executes? You know what the hell will happen? It will replace all the user’s password with this new password instead of failing as MySQL count the “where user_id” part equals to false and returns all users. But If your Database is PostgreSQL, it fails, you are lucky.
So day before yesterday we suffered this problem against our commercial application which corrupts our user profile data. We immediate fixed the issue from our backup db (well, we lost 3 data) and then we started to find out what actually went wrong and found this vulnerable bug in CI.
So we suggest the CodeIgniter team to fix the issue immediately and change their ORM code so that it creates the query like the following if the value of passed argument is null. because it will fail to execute in all db. Otherwise the fellow user’s of code igniter, prepare for the dooms day.
UPDATE users set password='{$new_password}’ where user_id=”;
nice catchup. f**k CI 😉
sorry hasan, I am a Big CI Fan. I dont want to f**k it just for this small bug, they themselves will fix it.
CI Rocks. It makes my life a lot easier than before.
Regards
“You found a bug, you reported it, the code is being worked on. No need to blab all over the word about your “bad experience” with CI. Especially when the problem was partially yours for not checking your data. Methinks your ego must be gigantic.” — JAAulde
For the record, I am “JAAulde” from the CI forums (among places) and I want you to know that while I did originate the above quote in your thread at CI, I did not cross-post it to your blog.
Jim (JAAulde)
I am a CI fan, at the first, this small problem doesn’t append if you validate the form with form validation class, it is absolutely necessary validate the user input ( in the client side, and ALLWAYS in the server side ) before database modification, in other case you run with the risk of database corruption 😉 . Of course, if your project is a commercial project, this care is more important.
Hi Sevir, i agree that I must the user input beofre sending it to CI, if you check the forum post also there was a huge discussion that as I sent ti a null value and CI didn’t interpret it correctly.
If I send a “null” to CI, ActiveRecord must not apply any Fuzilogic to it and it should be treated as null, what CI developers overlooked.
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.
Sorry for my poor English, Bowman. And thanks again for you wonderful comment.
This is really too bad, because I asked a question about this exact same issue last year in the CodeIgniter forum. (well, close enough, but I was concerned with passing in a “false” instead of a null). All I got was a single rude response from a community member questioning my knowledge of PHP.
http://codeigniter.com/forums/viewthread/47012/
I really hope this behavior changes so that it doesn’t cause problems for anyone else in the future.
@Bowman, I do not work with him, but I do read his posts, because it helps me a lot. This post is really helpful, coz nowadays, I am building lots of models in CodeIgniters, and after now, I would be careful, while I am gonna run any query using the Active Record Class. He may not be native english speaker, but he is technically strong enough to grab the attention of almost all the php developers from Bangladesh.
@hasin bhai, I would be careful. Thanks for this post.
CI rocks. Can you post your patch?
well, as most developers will do before processing data is to first and foremost, validate entries. right? it’s a simple logic..
bug is a bug – but no reason to make it bigger then it should be. I also found 3 critical bugs in CI , posted it and fixed it BTW in my application.. nobodys perfect
Bad programming habit, but blame the framework 🙂
Next time, write model with default value for each parameter, then check it again before using it in a query.
well actually its your fault for being lazy making validations…..
humm… it is always good to have someone to blame for our incompetence.
This is not a bug of CI.
You must go with some basic “sanity checks” on data when you are using a database, if you are serious about development, of course.
Hello Hasin vai,
This bug is not all, codeigniter has a lot more weaknesses, like their input library doesn’t filter all malicious html tags. And also when using form validation with file uploads it won’t let u validate the file fields. And also if you want use form validation with modules it will give u some trouble. There are currently many layout library for codeigniter. but they only support one single block in the layout. that’s a problem.
I have created extended libraries for minimizing all those bugs. For every one interested, you can find those tahSin’s gaRAge. thanks.