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”

1
2
3
4
5
6
7
8
9
10
11
12
13
14

<?php
if($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:

1
2
3
4
5
6
7
8
9
10
11
12
13
14

<?php
if($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.

Happy Querying!

@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.

Here is the original code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43

<?php
#...
if (function_exists('strip_html_tags') === FALSE) {
    function strip_html_tags($str) {
        $replacements = array(
            '#<head[^>]*?>.*?</head>#siu',
            '#<style[^>]*?>.*?</style>#siu',
            '#<script[^>]*?>.*?</script>#siu',
            '#<noscript[^>]*?>.*?</noscript>#siu',
        );
        $str = preg_replace('/(<|>)\1{2}/is', '', $str);
        $str = preg_replace($replacements, "", $str);
        $str = replace_whitespace($str);
        $str = strip_tags($str);
        return $str;
    }
}
if (function_exists('replace_whitespace') === FALSE) {
    function replace_whitespace($str) {
        $result = $str;
        $empties = array(
            " \t",
            " \r",
            " \n",
            "\t\t",
            "\t ",
            "\t\r",
            "\t\n",
            "\r\r",
            "\r ",
            "\r\t",
            "\r\n",
            "\n\n",
            "\n ",
            "\n\t",
            "\n\r",
        );
        foreach ($empties as $replacement) {
            $result = str_replace($replacement, ' ', $result);
        }
        return ($str !== $result) ? replace_whitespace($result) : $result;
    }
}

The only hope I have for the original author is that they wrote this to fight back against some ridiculous encoding issues when it came to whitespace.

Assuming you don’t have ridiculous encoding issues here are some better alternatives for replacing whitespace:

Trim (PHP Docs)

1
2
3
4
5
6
7

<?php
if (function_exists('replace_whitespace') === FALSE) {
    function replace_whitespace($str) {

        return trim($str);
    }
}

You do not need a method to do this, you can simly trim($str) elsewhere in your application

As the docs state, without a second parameter trim() will strip these characters:

• “ ” (ASCII 32 (0x20)), an ordinary space.
• “\t” (ASCII 9 (0x09)), a tab.
• “\n” (ASCII 10 (0x0A)), a new line (line feed).
• “\r” (ASCII 13 (0x0D)), a carriage return.
• “\0” (ASCII 0 (0x00)), the NUL-byte.
• “\x0B” (ASCII 11 (0x0B)), a vertical tab.

String Replace (PHP Docs) or Preg Replace (PHP Docs)

Again with this method, you do not need to pass this into it’s own method.

To remove a space “ ” (ASCII 32 (0x20)):

1
2
3

<?php

$str = str_replace(' ', '', $str);

To remove all whitespace, we can use a regular expression:

1
2
3

<?php

$str = preg_replace('/\s+/', '', $str);

More discussion found at stackoverflow

Recap

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.

Thanks for Reading. Happy Coding!

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

<?php
function checkIfUserIsLoggedIn($user_logged_in) {
  // Do something to check if login is active / current
  if (!$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

1
2
3
4
5
6
7
8
9
10

<?php
include('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?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

<?php
include('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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17

<?php
function checkIfUserIsLoggedIn($user_logged_in) {
  // Do something to check if login is active / current
  if (!$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

        exit;
  }
}

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.

Here is the original code

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69

<?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 empty
  if(empty($email)){
      $status = "error";
      $message = "You did not enter an email address!";
  }
  else if(!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
  );
  
  echo json_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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29

<?php
$database_host = 'localhost';
$database_name = 'signup';
$database_user = 'user';
$database_password = 'I<3CSIPHP';
$conn = new PDO("mysql:host=$database_host;dbname=$database_name", $database_user, $database_password);

function CheckThisSignUp($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;
}
function AddSignUp($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 successful
        return array('status'=>'success','message'=>'You have been signed up!');
    }
    else { //if insert fails
        return array('status'=>'error','message'=>'Ooops, Theres been a technical error!');
    }
}

Better than original code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30

<?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 empty
if (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);
echo json_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
• Friends don’t let friends run bad code.

I thought that Graham was the most devious PHP code troll I’d ever met. Turns out I was wrong. Dead wrong.

This tweet:

led me to this post: “Creating a user from the web problem”, which includes this code:

1

shell_exec("sudo useradd -p $encpass -g groupname -s /bin/bash $username");

I honestly don’t think this post is for real. I think it’s a brilliant, amazing, downright epic troll. Kudos to you, sir or ma’am. Kudos.

WARNING: The code you’re about to view is intended for mature audiences and may not be suitable for all readers.

I saw this horrifying code snippet in my Twitter feed this afternoon.

To make this abomination easier to read, I’ve copied and formatted it here.

1
2
3
4

<?
foreach (get_defined_vars() as $var) {
    @extract($var);
}

What this nightmare does, in case you aren’t immediately able to grok it, is iterate over a multidimensional array containing a list of all defined variables (environment, server, and user defined) and import them into the current symbol table. If that weren’t bad enough, it uses the @ error control operator and (because this is extract’s default behavior) overwrites any existing defined variable it encounters.

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.

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!

A few examples to get your attention:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16

<?php

if (!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.

Getting a listing from MySQL:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34

<?php
/**
 * Example how to list items from a databas using PDO
 *
 * @author Michelangelo van Dam
 */

ini_set('display_errors',1);
error_reporting(-1);

$pdo = new PDO(
    '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>

    <?php while ($row = $result->fetch(PDO::FETCH_ASSOC)): ?>
    <tr>
        <td><?php echo htmlentities($row['id'], ENT_QUOTES, 'utf-8') ?></td>
        <td><?php echo htmlentities($row['item'], ENT_QUOTES, 'utf-8') ?></td>
        <td><?php echo htmlentities($row['price'], ENT_QUOTES, 'utf-8') ?></td>
    </tr>
    <?php endwhile ?>

</table>

Getting a single item from MySQL:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26

<?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 = new PDO('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><?php echo htmlentities($item['item'], ENT_QUOTES, 'utf-8') ?></dt>
    <dd><?php echo htmlentities($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:

• Essential PHP Security by Chris Shiflett
• Pro PHP Security
• php|architect’s Guide to PHP Security by Ilia Alshanetsky

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.

For more about PHP DateTime functionality, see the official DateTime documentation and the Date and Time chapter at PHP: The Right Way.

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.

I’ll let contributor @dilbert for life explain it in his own words:

“Read this today on a developer thread. I was horrified, but then I realized he was working on his local machine – so no issue there¹. But, then I read the rest of his email/question, and in his signature, it said “Senior PHP Developer.” NNNOOOOOO”

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

<?php

mysql_connect("localhost", "uname", "pass@3") or die(mysql_error());

echo "Connected to MySQL<br />"; // working fine

mysql_select_db("dbname") or die(mysql_error());

echo "Connected to Database"; // working fine

extract($_POST);

$result = mysql_query("SELECT * FROM admin_users WHERE admin_email='" . $user_email .
        "' AND password='" . md5($password) . "' AND status=1") or die(mysql_error()); // not working returns null

while($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:

1
2
3

<?php

date_default_timezone_set(@date_default_timezone_get());

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?).

Worse than silencing notices and warnings, the Error Control Operators documentation states,

“… the ”@“ error-control operator prefix will even disable error reporting for critical errors that will terminate script execution.”

Good luck troubleshooting that business.

So which timezone do I use?

Take Sundquist’s advice. If you’re not sure what to do with your timezone, set it to UTC and move on.

If you have any thoughts or advice you’d like to add, drop ‘em in the comments below.