CSI: PHP

"Looking at your tweets I cannot even fathom what your job is. CSI:PHP?" — @grmpyprogrammer

No Way That’s Real

| Comments

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.

Artisan Level Code Trolling

| Comments

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.

I Am Repeating Myself

| Comments

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:

DateTime What?

| Comments

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.

Senior Dev Invites Application Destruction

| Comments

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

date.timezone WTF?

| Comments

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.

Interview Coding Disasters – FizzBuzz (+ TDD) Edition

| Comments

Today’s anonymous submitter is shocked to discover that FizzBuzz really works (and TDD isn’t as alive and well as he’d hoped).

A local company asked me for assistance recruiting a new PHP developer. I helped them come up with a job description, a few interview questions, and I threw in the FizzBuzz test for good measure. Since they’re working on implementing Test Driven Development, I added “ … bonus points for writing FizzBuzz using TDD.” I was horrified to see the massive fail that were the FizzBuzz submissions, but the single TDD submission took the cake.

Interestingly, it looks like the FizzBuzz portion of the submission probably works. It would have been a good idea if the submitter had read something, anything, about TDD before trying his hand at it in an interview situation.

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
70
71
72
73
74
75
76
77
<?php

class FizzBuzzTest {
    //build array that contains inputs 1-100
    public function totalInput() {

        $results = array();

        for( $i=1; $i<=100; $i++ ) {
            $results[] = $i;
        }

        return $results;
    }

    //build array that controls ttd test inputs 1-15
    public function tddInput() {

        $results = array(1,2,3,4,5,6,7,8,9,10,11,12,13,14,15);

        return $results;
    }

    //compare tddInput with the expected results
    public function testInput($source) {

        if($source == 'short') {
            $input = array(1,2,3,4,5,6,7,8,9,10,11,12,13,14,15);
        } else if($source == 'long') {
            $input = $this->totalInput();
        }

        $test = new runTest();

        $expected = array(1,2,'Fizz',4,'Buzz','Fizz',7,8,'Fizz','Buzz',11,'Fizz',13,14,'FizzBuzz');

        $testResults = $test->process($input);

        if($source == 'long') {
            return $testResults;
        } else {
            $this->assertEquals($expected, $testResults);
        }
    }
}

//replace multiples of 3 with FIZZ
//replace multiples of 5 with BUZZ
//replace multiples of 3 & 5 with FIZZBUZZ
//if none apply, show number
class runTest {
    public function process($input) {

        $results = array();

        foreach($input as $value) {

            $output = '';

            if($value % 3 == 0 && $value % 5 == 0) {
                $output = 'FizzBuzz';
            } elseif($value % 3 == 0) {
                $output = 'Fizz';
            } elseif($value % 5 == 0) {
                $output = 'Buzz';
            } else {
                $output = $value;
            }

            $results[] = $output;
        }

        return $results;
    }
}

?>

Words Escape Me

| Comments

Look, I know this is going to be tough to read, and I usually try to post short, snappy code snippets, but this is just too good. Or sad. Whatever. Words truly escape me.

Take it away anonymous submitter:

“Look at all these standalone ternaries for validating form input. What a mess! They’re embedded in what would normally be controller logic. These should be if() statements or, preferably, a separate validating class.”

Indeed.

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
<?php

(!is_numeric($input['raw_gpa']) || $input['raw_gpa'] < 0 || $input['raw_gpa'] > 4) ? ($errors[] = "Your unweighted GPA must be between 0 and 4. You must indicate your unweighted GPA, and this must be between 0 and 4.") : ('');
($input['gpa_max'] < 0 || $input['gpa_max'] > 200) ? ($errors[] = "Your school's max possible GPA must be between 0 and 100.") : ('');
($input['gpa'] < 0 || $input['gpa'] > $input['gpa_max']) ? ($errors[] = "Your weighted GPA must be between 0 and your school's max weighted GPA.") : ('');

(($input['act'] == "") && (($input['sat_verb'] == "") || ($input['sat_math'] == ""))) ? ($errors[] = "To use this site, you must indicate your SAT scores, or your ACT scores, or both.") : ('');

(!is_numeric($input['act']) && ($input['act'] != "")) ? ($errors[] = "Your ACT score must be a numeric value, or blank.") : ('');
(!is_numeric($input['act_eng']) && (!empty($input['act_eng']))) ? ($errors[] = "Your ACT English score must be a numeric value, or blank.") : ('');
(!is_numeric($input['act_math']) && (!empty($input['act_math']))) ? ($errors[] = "Your ACT Math score must be a numeric value, or blank.") : ('');
(!is_numeric($input['act_read']) && (!empty($input['act_read']))) ? ($errors[] = "Your ACT Reading score must be a numeric value, or blank.") : ('');
(!is_numeric($input['act_sci']) && (!empty($input['act_sci']))) ? ($errors[] = "Your ACT Science score must be a numeric value, or blank.") : ('');

(!is_numeric($input['sat_math']) && ($input['sat_math'] != "")) ? ($errors[] = "Your SAT math must be a numeric value, or blank.") : ('');
(!is_numeric($input['sat_verb']) && ($input['sat_verb'] != "")) ? ($errors[] = "Your SAT verbal must be a numeric value, or blank.") : ('');
(!is_numeric($input['sat_writ']) && ($input['sat_writ'] != "")) ? ($errors[] = "Your SAT writing must be a numeric value, or blank.") : ('');

((($input['act'] > 36) || ($input['act'] < 1)) && ($input['act'] != "")) ? ($errors[] = "Your ACT score is out of the valid range.") : ('');
((($input['act_eng'] > 36) || ($input['act_eng'] < 1)) && ($input['act_eng'] != "")) ? ($errors[] = "Your ACT English score is out of the valid range.") : ('');
((($input['act_math'] > 36) || ($input['act_math'] < 1)) && ($input['act_math'] != "")) ? ($errors[] = "Your ACT Math score is out of the valid range.") : ('');
((($input['act_read'] > 36) || ($input['act_read'] < 1)) && ($input['act_read'] != "")) ? ($errors[] = "Your ACT Reading score is out of the valid range.") : ('');
((($input['act_sci'] > 36) || ($input['act_sci'] < 1)) && ($input['act_sci'] != "")) ? ($errors[] = "Your ACT Science score is out of the valid range.") : ('');
((($input['sat_math'] > 800) || ($input['sat_math'] < 200)) && ($input['sat_math'] != "")) ? ($errors[] = "Your SAT math score is out of the valid range.") : ('');
((($input['sat_verb'] > 800) || ($input['sat_verb'] < 200)) && ($input['sat_verb'] != "")) ? ($errors[] = "Your SAT verbal score is out of the valid range.") : ('');
((($input['sat_writ'] > 800) || ($input['sat_writ'] < 200)) && ($input['sat_writ'] != "")) ? ($errors[] = "Your SAT writing score is out of the valid range.") : ('');

((($input['psat'] > 240) || ($input['psat'] < 60)) && ($input['psat'] != "")) ? ($errors[] = "Your PSAT is out of the valid range.") : ('');

(!array_key_exists($input['nat_merit'], $nationalMeritArray)) ? ($errors[] = "Please indicate whether or not you were awarded National Merit or National Achievement awards.") : ('');
(!array_key_exists($input['ap'], $apCountArray)) ? ($errors[] = "Please estimate how many APs your school offers.") : ('');

(!array_key_exists($input['hs_type'], $hs_type_array)) ? ($errors[] = "Please indicate what type of high school you attend (public, private, magnet, etc.).") : ('');
(!array_key_exists($input['hs_state'], $state_array)) ? ($errors[] = "Please indicate the state that your high school is in.") : ('');
($input['hs_city'] == "") ? ($errors[] = "Please indicate the city that your high school is in.") : ('');

(array_search($input['decile'], $decile_array) === false) ? ($errors[] = "Please indicate your class rank.") : ('');

((!is_numeric($input['class_size']) || $input['class_size'] < 1 || $input['class_size'] > 10000) && $input['class_size'] != "") ? ($errors[] = "If you indicate your class size, it must be numeric (without words, periods, or commas).") : ('');

Too Few DEFINEs for My Taste

| Comments

The Crime

It takes a big man to admit when he’s wrong, and @dilbert4life is one of those men. Here’s a snippet of horror that he wrote around a year-and-a-half ago.

Too many defines? Nay! I say let the site grow and see just how many we can stuff in there.

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
<?php
$slug = str_replace('.php', '', $_SERVER['REQUEST_URI']);
$slug = preg_replace('/\//', '', $slug, 1);
$slug = explode('?', $slug);
$slug = $slug[0];
if($slug == 'index' || $slug == '') {
    $slug = "/";
    define(IS_HOME, true);
    define(IS_ADMIN, false);
    define(IS_NEWS, false);
    define(IS_CONTACT, false);
} elseif(preg_match('/admin/', $slug)) {
    define(IS_HOME, false);
    define(IS_ADMIN, true);
    define(IS_NEWS, false);
    define(IS_CONTACT, false);
} elseif(preg_match('/news/', $slug)) {
    define(IS_HOME, false);
    define(IS_ADMIN, false);
    define(IS_NEWS, true);
    define(IS_CONTACT, false);
} elseif(preg_match('/contact/', $slug)) {
    define(IS_HOME, false);
    define(IS_ADMIN, false);
    define(IS_NEWS, false);
    define(IS_CONTACT, true);
} else {
    define(IS_HOME, false);
    define(IS_ADMIN, false);
    define(IS_NEWS, false);
    define(IS_CONTACT, false);
}

Rehabilitation

Normally, a submitter, whether anonymous or not, will simply send in an incident report and leave it at that. @dilbert4life has gone above and beyond, however. Not only did he submit his crime, but he submitted evidence of his rehabilitation.

1
2
3
4
5
6
7
8
<?php
$uri = parse_url($_SERVER['REQUEST_URI']);
$path = explode('/', $uri['path']);

define('IS_HOME', (empty($path[1])));
define('IS_ADMIN', ($path[1] === 'admin'));
define('IS_NEWS', ($path[1] === 'news'));
define('IS_CONTACT', ($path[1] === 'contact'));

I especially like how he replaced three lines of garbage with parse_url.

What Idiot Wrote This? Oh, Wait, It Was Me.

| Comments

Starting with this post, we have a new category here at CSI: PHP. It’s called ‘mea culpa’, and it’s criminal code written by yours truly.

Behold the awesome that is error(), an unholy concoction of PHP, JavaScript, and HTML. It’s the only function in a file called common.php, which makes perfect sense because shut up. Yes, this was in production use, and would dislpay a JavaScript alert with an error message whenever I remembered to use it.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
<?php // common.php

function error($msg) {
    ?>
    <html>
    <head>
    <script language="JavaScript">
    <!--
        alert("<?=$msg?>");
        history.back();
    //-->
    </script>
    </head>
    <body>
    </body>
    </html>
    <?
    exit;
}