New Topic/Question
Reply
Ok so I have a side gig where I work as a contractor with my former full time employer. The company recently attained the services of a small local web development company to do some work on the project I work on. After the local company committed their stuff to source control I was asked to do a code review of their stuff to get an idea of their competency level. This is what I found.
First of all they were using deprecated mysql* functions for database access. Ok, not a big deal, even though the project has a Database class to do this stuff. I can see maybe they were not aware of this. Then I notice none of the incoming data is escaped in any way. It's wide open to SQL injection and XSS in literally dozens of places. This is when I started to get worried.
The site has members, people can register using Facebook in which case their Facebook id is stored along with their other account information. One of the things this company had to do was check if any of the logged in members Facebook friends were members of the site identified by the Facebook id we store. This is what I saw:
$users;
$friendsList = $facebook->api($fbProfile.'/friends');
$user_fb_uid;
$sql = "SELECT `member_id`, `facebook_uid` FROM `members`";
$result = mysql_query($sql);
if($result){
while($data = mysql_fetch_object($result)) {
$users[] = $data->member_id;
$user_fb_uid[] = $data->facebook_uid;
}
}
$bpFriends = Array();
foreach($friendsList['data'] as $i=>$friend){
$friendID = $friend['id'];
foreach($user_fb_uid as $i=>$blogUser){
if($friendID == $blogUser){
$bpFriends[] = $users[$i];
}
}
}
return $bpFriends;
So first they select EVERY member, tens of thousands of them and growing every day. Then they store the member ids(primary key) and the Facebook ids in two arrays. Then they iterate over the results of an Facebook API call that gets the logged in users friends and inside that loop they iterate through every one of the previously fetched members records(inexplicably naming the value variable $blogUser) seeing if they can find a match....
I'm also seeing allot of things like this:
$result = mysql_query("SELECT * FROM table WHERE field = somevalue");
if($result)
{
$i = 0;
while($data = mysql_fetch_object($result)
{
$i = $i + 1;
}
if($i > 0)
.....
}
There are thousands of lines of code like this. The code review has turned into an all day event when I was expecting it to take ~4 hours max. And this work did not come cheap, the company charged a fair price. I'm not even sure how to word the response I will eventually send with the code review....
MultiQuote
Correct me if I'm wrong though.
| 