This great code snippet came from editor in chief of php[architect] magazine, Oscar Merida. The best step you can take to prevent your code from showing up here on our site is subscribe to @phparch
Oscar sent us this code with the statement “The user must have really enjoyed writing raw SQL statements”
1234567891011121314
<?phpif($something){if($venue_group_id){$query="SELECT * FROM buildings WHERE building_group_id=$venue_group_id AND organization_id = $organization_id AND active = 1 ORDER BY venue_name ASC";}else{$query="SELECT * FROM buildings WHERE active = 1 AND organization_id = $organization_id AND building_group_id = ".$session->building_group_id." ORDER BY venue_name ASC";}}else{if($venue_group_id){$query="SELECT * FROM buildings WHERE building_group_id=$venue_group_id AND active = 1 ORDER BY venue_name ASC";}else{$query="SELECT * FROM buildings WHERE active = 1 AND building_group_id = ".$session->building_group_id." ORDER BY venue_name ASC";}}
That’s some interesting ordering of AND statements, let’s clean it up a bit:
1234567891011121314
<?phpif($something){if($venue_group_id){$query="SELECT * FROM buildings WHERE active = 1 AND organization_id = $organization_id AND building_group_id=$venue_group_id ORDER BY venue_name ASC";}else{$query="SELECT * FROM buildings WHERE active = 1 AND organization_id = $organization_id AND building_group_id = ".$session->building_group_id." ORDER BY venue_name ASC";}}else{if($venue_group_id){$query="SELECT * FROM buildings WHERE active = 1 AND building_group_id=$venue_group_id ORDER BY venue_name ASC";}else{$query="SELECT * FROM buildings WHERE active = 1 AND building_group_id = ".$session->building_group_id." ORDER BY venue_name ASC";}}
Now we can more easily see what our four queries are doing here. If $something is true, then we search the building_group_id based on $venue_group_id unless it’s false, then we use the value from $session instead. If $something is false, we again check the $venue_group_id and either use the value or again use the value from $session.
You’re probably thinking “oh I could turn this into a 1 liner!”, yes you likely could but I would caution you against over optimization and instead stick with code that is easier to human parse. Your future self will always appreciate your past self from being too clever.
If this was my code I would probably break up the sequel into parts and concatenate in the conditional statements so that we end up with the necessary query based on our conditions.
@devnuhl sent this one to us via a Gist earlier today;
Some background he gave us:
Honestly just stumbled onto this while updating the codebase. Oddly enough, it seems like the only usage of this function is in another function in the same file, which is being added to the gist now. Having gotten rid of some glaring errors in the regular expressions, I find it interesting that preg_replace is used in the one, but not the other. Just replace \s+ with a space.
Rolling your own functionality that PHP already provides is silly. If you believe you need to do this; do some searching. You’ve probably either found an edge case or you need to do some refactoring.
A JavaScript redirect is a fairly simple technique to easily redirect your users to a new location. Within the confines of a PHP application if you need the parent window’s URL, JavaScript is the easiest way to grab that info.
We have a PHP application that has a checkIfUserIsLoggedIn function and an $user_logged_in object is passed in to check if the user is logged in. On failure we need to redirect this user to the login screen on the same domain they’re currently on. Keeping them on the same domain is something we have to do because this application responds to many different domain names.
Here is the original code
someIncludedFile.php
123456789101112131415
<?phpfunctioncheckIfUserIsLoggedIn($user_logged_in){// Do something to check if login is active / currentif(!$user_logged_in){// user inactive / session expired / user logged out// redirect user to login screen on same domain?> <script type="text/javascript"> var pathToLoginScreen = '/path/to/login/screen var redirectURI = window.parent.location.href+pathToLoginScreen; window.location=redirectURI; </script><?php}}
somePartOfOurApp.php
12345678910
<?phpinclude('someIncludedFile.php');// this is our user login object used to check if login is active / current$user_logged_in=$user->logged_in;// Check if login is active/current$is_user_logged_in=checkIfUserIsLoggedIn($user_logged_in);// If the user's session is active / valid the app continues// Otherwise the user should have been redirected via JavaScript
What is wrong with it?
We are relying on the function to output raw JavaScript. This should be a check that gets acted on elsewhere.
What happens if JavaScript is turned off?
It gets worse…
What happens when you do this type of log in check and then update a record?
12345678910111213141516
<?phpinclude('someIncludedFile.php');// this is our user login object used to check if login is active / current$user_logged_in=$user->logged_in;// Check if login is active/current$is_user_logged_in=checkIfUserIsLoggedIn($user_logged_in);// We didn't get redirected MUST be a good user RIGHT!?// Do some active user logged in stuff// get some data$some_data=$data_source($find_this_data);// update data from source$update_result=$update_data($som_data);
There is still something wrong with it…
If the user turns off JavaScript, the redirect never happens, and the code continues.
Invalid users can update data as they please.
Where did we go wrong!?
We could have avoided this by doing a hard exit after our JavaScript redirect in our PHP conditional.
someIncludedFile.php
1234567891011121314151617
<?phpfunctioncheckIfUserIsLoggedIn($user_logged_in){// Do something to check if login is active / currentif(!$user_logged_in){// user inactive / session expired / user logged out// redirect user to login screen on same domain?> <script type="text/javascript"> var pathToLoginScreen = '/path/to/login/screen var redirectURI = window.parent.location.href+pathToLoginScreen; window.location=redirectURI; </script><?phpexit;}}
Adding the exit after our JavaScript redirect would prevent any code below from being executed. Since we were not doing an exit previously, all the code under our JavaScript redirect would run if the user had JavaScript disabled in their browser. This also applies to browsers that do not support JavaScript.
Lessons learned:
Always exit your PHP if you have to do a non-PHP URI redirect!
Friend of mine asked me to take a look at his PHP script. The original scope was to call this script via ajax, posting an email address to get saved to a database.
<?php//email signup ajax call$MySQLConnectFile='congifdb.php';if(file_exists($MySQLConnectFile)){$MySQLFileExists==TRUE;}if($MySQLFileExists){require_once($MySQLConnectFile);}else{echo'<b>Error:</b> <i>Could not read the MySQL Connection File. Please try again later.';mysql_close();exit();}//sanitize data$email=mysql_real_escape_string($_POST['signup-email']);//validate email address - check if input was emptyif(empty($email)){$status="error";$message="You did not enter an email address!";}elseif(!preg_match('/^[^\W][a-zA-Z0-9_]+(\.[a-zA-Z0-9_]+)*\@[a-zA-Z0-9_]+(\.[a-zA-Z0-9_]+)*\.[a-zA-Z]{2,4}$/',$email)){//validate email address - check if is a valid email address$status="error";$message="You have entered an invalid email address!";}else{$existingSignup=mysql_query("SELECT * FROM signups WHERE signup_email_address='$email'");if(mysql_num_rows($existingSignup)<1){$date=date('Y-m-d');$time=date('H:i:s');$insertSignup=mysql_query("INSERT INTO signups (signup_email_address, signup_date, signup_time) VALUES ('$email','$date','$time')");if($insertSignup){//if insert is successful$status="success";$message="You have been signed up!";}else{//if insert fails$status="error";$message="Ooops, Theres been a technical error!";}}else{//if already signed up$status="error";$message="This email address has already been registered!";}}//return json response$data=array('status'=>$status,'message'=>$message);echojson_encode($data);mysql_close();exit;?>
I immediately said “hey, let me take this and rework it a bit for you”.
A few problems right off the bat:
Not using PDO
Not using filter_var()
Not always outputting json
Not using prepared statements
My first task was to move everything to use PDO and prepared statements. The second step was to break the “check if email already signed up” and “add email” functionality to their own functions. This makes the code a bit cleaner in my opinion. I also made sure every error or condition checked out put to the $status and $message variables so the ajax call would always return any error status.
dbconfig.php
1234567891011121314151617181920212223242526272829
<?php$database_host='localhost';$database_name='signup';$database_user='user';$database_password='I<3CSIPHP';$conn=newPDO("mysql:host=$database_host;dbname=$database_name",$database_user,$database_password);functionCheckThisSignUp($email,$conn){$params=array(':email'=>$email);$query="SELECT * FROM signups WHERE signup_email_address = :email";$rows=$conn->prepare($query);$rows->execute($params);$data=$rows->fetchall();// close the database connection$errors=$conn->errorInfo();return$data;}functionAddSignUp($email,$conn){$params=array(':email'=>$email,':date'=>date('Y-m-d'),':time'=>date('H:i:s'));$query="INSERT INTO signups (signup_email_address, signup_date, signup_time) VALUES (:email, :date, :time)";$create_user=$conn->prepare($query);$create_user->execute($params);if($create_user->rowCount()==1){//if insert is successfulreturnarray('status'=>'success','message'=>'You have been signed up!');}else{//if insert failsreturnarray('status'=>'error','message'=>'Ooops, Theres been a technical error!');}}
<?php//email signup ajax call$MySQLConnectFile='dbconfig.php';if(file_exists($MySQLConnectFile)){require_once($MySQLConnectFile);}else{$status="error";$message="Could not read the MySQL Connection File. Please try again later!";}//sanitize data$email=filter_var($_POST['signup-email'],FILTER_SANITIZE_EMAIL);//validate email address - check if input was emptyif(filter_var($email,FILTER_VALIDATE_EMAIL)){$signup_check=CheckThisSignUp($email,$conn);if(empty($signup_check['0'])){$signup_results=AddSignUp($email,$conn);$status=$signup_results['status'];$message=$signup_results['message'];}else{//if already signed up$status="error";$message="This email address has already been registered!";}}else{$status="error";$message="You have entered an invalid email address!";}//return json response$data=array('status'=>$status,'message'=>$message);echojson_encode($data);?>
Lessons learned:
Use PDO over mysql_connect
Be consistent in your error handling
Prepared statements help keep your database safe.
FILTER_SANITIZE_EMAIL & FILTER_VALIDATE_EMAIL Are very easy to use
Try and debug unexpected behavior with that in your code. Go ahead. Try. You’ll be
reduced to a wimpering pile of crazy in just a few short hours.
There simply aren’t words sufficient to express the horror of this snippet. The worst
thing about this is I’ve seen similar code in the wild, and so has Graham.
Tweet could not be processed
Looks like osCommerce might have some ‘splainin’ to do …
It’s a cold, cruel world of bad code sometimes. I’m sorry I had to share this with
you. Hopefully it helps more than it hurts, since knowing what bad code looks
like is one of the first steps in being able to fix it.
Today’s entry is a guest post the from world-renowned Michelangelo van Dam, PHP master, community hero, consultant, and all around nice guy. Thanks for the post, Mike!
Yes, I’m repeating myself and apparently I need to. A quick search on github today revealed that 86,000+ people still use $_GET in their mysql statements!
Tweet could not be processed
A few examples to get your attention:
12345678910111213141516
<?phpif(!is_numeric($_GET['id'])){die("Numeric id !");}$id=$_GET['id'];mysql_query("DELETE FROM executions WHERE executionId=$id");switch($_GET['table']){case"Sabores":$sql="insert into Sabores values(0,'{$_GET['nombre']}', '{$_GET['descripcion']}','{$_GET['imagen']}','{$_GET['color']}')";$getid=$_GET['id'];mysql_query("DELETE FROM comment WHERE id='$getid'");header("Location: success.php");
People please, don’t do it this way! You’re opening up your application to a whole lot of hurt and damage beyond repair if you implement this kind of code in production!
Remember this line: Filter input, escape output.
Filter all incoming data, no matter if they come from user input, database, web services or even from OCR using cameras!
Ensure this incoming data is always in the format you want and validates to rules you have defined. Yes, it’s a lot of work to filter and validate everything, but a lot less to clean up your database when they’ve done a “Little Bobby Tables” on your application.
Use an abstraction layer like PDO and properly prepare statements with named variables to ensure data will be properly escaped before it enters your database. Even if you haven’t taken the time to sanitise your data, this will be your final line of defence. Use it! Period!!!
This will be your basics to get started with PDO, but check out php.net/pdo for more information.
<?php/** * Example how to list items from a databas using PDO * * @author Michelangelo van Dam */ini_set('display_errors',1);error_reporting(-1);$pdo=newPDO('mysql:host=localhost;dbname=<DBNAME>','<USERNAME>','<PASSWORD>');$result=$pdo->query('SELECT * FROM `items`');?><table> <tr> <th>ID</th> <th>ITEM</th> <th>PRICE</th> </tr><?phpwhile($row=$result->fetch(PDO::FETCH_ASSOC)):?> <tr> <td><?phpechohtmlentities($row['id'],ENT_QUOTES,'utf-8')?></td> <td><?phpechohtmlentities($row['item'],ENT_QUOTES,'utf-8')?></td> <td><?phpechohtmlentities($row['price'],ENT_QUOTES,'utf-8')?></td> </tr><?phpendwhile?></table>
Getting a single item from MySQL:
1234567891011121314151617181920212223242526
<?php/** * Example how to list items from a databas using PDO * * @author Michelangelo van Dam */ini_set('display_errors',1);error_reporting(-1);// sanitize input and validate it's an integer$id=filter_input(INPUT_GET,'id',FILTER_VALIDATE_INT);$pdo=newPDO('mysql:host=localhost;dbname=test','test','test');// Use prepare statements for separating PHP and SQL$stmt=$pdo->prepare('SELECT * FROM `items` WHERE `id` = :id');$stmt->execute(array(':id'=>$id));// Get the result you want$item=$stmt->fetch(PDO::FETCH_ASSOC);?><dl> <dt><?phpechohtmlentities($item['item'],ENT_QUOTES,'utf-8')?></dt> <dd><?phpechohtmlentities($item['price'],ENT_QUOTES,'utf-8')?></dd></dl>
Now get moving to php.net/pdo and start learning doing things the right way!
— Michelangelo van Dam
Further Reading
If you’d like to dig into the topic of PHP security even further (and you do), check out these books that Mike recommends:
I’ve seen a lot of crazy, tortured interactions between PHP and databases in my career, but this particular solution to the problem of displaying future dates is one of the most tortured I’ve ever seen.
The short story is that the developer in question must have known SQL much better than any scripting language, chose to use SQL date math functions, queried the DB for persisted dates, and iterated over them in PHP. Wat?
For the whole story, you’ll need to head over to the always amazing The Daily WTF to read PHP Doesn’t Have Date Functions Either. You’ll be glad you did.
Thanks to reader Michael Greiling for the heads up on the Daily WTF Post.
I’m not sure what else you’d call this besides an open invitation to face roll your
database. How many times have we discussed SQL injection here at CSI: PHP? I guess we’ll have to discuss it a lot more.
“Read this today on a developer thread. I was horrified, but then I realized
he was working on his local machine – so no issue there1. But, then I read the
rest of his email/question, and in his signature, it said “Senior PHP Developer.”
NNNOOOOOO”
123456789101112131415161718
<?phpmysql_connect("localhost","uname","pass@3")ordie(mysql_error());echo"Connected to MySQL<br />";// working finemysql_select_db("dbname")ordie(mysql_error());echo"Connected to Database";// working fineextract($_POST);$result=mysql_query("SELECT * FROM admin_users WHERE admin_email='".$user_email."' AND password='".md5($password)."' AND status=1")ordie(mysql_error());// not working returns nullwhile($row=mysql_fetch_array($result)){print_r($row);}
This is a straight education problem. My suspicion is this “Senior” dev is the
only dev, has worked solo for many years, reads very little, and has no contact
with the PHP community. I wish there was a good way to reach out to those devs,
show them the error of their ways, and teach them the right way. Ah, but I digress
from my usual cynical snark.
WAT A BIG DUMMY.
With that out of the way, the right way to refactor the snippet above is to 1)
use PDO and 2) use prepared statements. Show us how it’s done, PHP: The Right Way.
Notes
1. I respectfully disagree with the contention that bad practices
are acceptable in local environments. “Practice like you’ll play” is one of
my favorite quotes. In this context it means that you should write every line of code as if it’s going
into production. Best practices are best practices everywhere.
CSI: PHP Investigator @jsundquist recently forwarded
the below incident report:
Yesterday a user posted in the php general mailing list asking why he needed
to set a default timezone now for php 5.3. It was explained to him why and how.
After doing some of his own research he decided to go the route of:
His only reasoning for wanting to go this route is because it is “deployed on servers all over the world and he does not know what timezone the server is in”.
All I can say to that is, wow. Set it to UTC and leave it at that.
I’m with Sundquist on this one. Wow.
Timezone is serious business
First, always set date.timezone. Always, always, always. It’s used by all of the
PHP date/time functions, and those are pretty damned important.
If the timezone is invalid, PHP generates E_NOTICE. You’ll get an E_WARNING if PHP has to guess your timezone
from the system settings or has to use the TZ environment variable. What’s more,
PHP no longer attempts to guess your timezone as of PHP 5.4.0. Setting your
timezone in your php.ini or using date_default_timezone_set() is a must.
The “shutup” operator
Don’t use it. Ever. It’s never OK to use the shutup operator. Those notices
and warnings are there for a reason, and should be dealt with immediately (you
always develop with error_reporting set to -1, right?).