3
\$\begingroup\$

Please review the following code and list all the coding errors and poor coding practices that you can see in this code.

function output()
{
    // Check authorization
    if(is_authorized())
    {
        $authorized = true;
        include('/path/to/' . $_REQUEST['module'] . '.php');
    }
    echo "<ul>"
    $conn = mysql_connect( "dbserver.com:6789", "dbyuser", "dbpassWd" );
    mysql_select_db( "kum", $conn ); // selects a database
    $q = " SELECT * FROM main WHERE id > " . $_GET["id"]. ";";
    $res = mysql_query( $q, $conn);
    while( $row = mysql_fetch_assoc( $res ) )
    {
        echo "<li>".$row['description']."</li>";
    }
    echo "</ul><br><ul>";
    $q = " SELECT * FROM main WHERE id < " . $_GET["id"]. ";";
    $res = mysql_query( $q, $conn);
    while( $row = mysql_fetch_assoc( $res ) )
    {
        // Display the status if it is authorized, otherwise display N/A
        echo "<li>".$row['description']. "(" .
        $authorized ? $row['status'] : "N/A" . ")</li>";
    }
    echo "</ul>";
}
\$\endgroup\$
  • \$\begingroup\$ Will post a proper answer later, but a few things jump out at me. Namely, SQL injection, your include is open to cross directory including, you don't check that any of the keys are set in your array accesses, and you don't escape your HTML output. \$\endgroup\$ – Corbin Jun 18 '12 at 4:55
  • 2
    \$\begingroup\$ Can you please explain it little more ... ? \$\endgroup\$ – Miqdad Ali Jun 18 '12 at 4:56
  • \$\begingroup\$ Other than the first section, codereview.stackexchange.com/questions/12647/… should explain it for the most part. \$\endgroup\$ – Corbin Jun 18 '12 at 5:06
2
\$\begingroup\$

showerhead and haylem covered this question quite thoroughly, but there's a few more things I think should be added or expounded upon.

Be extremely careful when including based on user input

This was mentioned a few times by both people mentioned, but considering how big of security hole it can be, I feel that it deserves a lot more attention than anyone gave it.

You should NEVER include a file blindly based on user input.

What if I go to: page.php?module=../some/secret

Suddenly /path/some/secret.php just got included. The .php extension would limit a lot of the damage this could do (page.php?module=../path/to/some/config.ini), but the hole is still there.

I tend to be extremely strict with file name handling. I typically only allow a-z, A-Z, 0-9, -, and _. That way you know that it will be a semi-logical file name, and you know that it will not be able to traverse directories.

For example:

$module = (isset($_GET['module']) && is_string($_GET['module'])) ? $_GET['module'] : null;
if (!preg_match('/^[a-zA-Z0-9_-]$/', $module)) {
    $module = false;
}

You should also check if the file is readable before including it. Otherwise, a user may see an ugly error message, or, best case, you open up the possibility of a huge log file of meaningless errors. (Production servers should never have display_errors = On, but they should have error_reporting = E_ALL.)

Don't assume (user provided) array keys are set

showerhead touched on this:

Also, I would not immediately use any value from any user provided array directly in my program. It should be sanitized and validated first.

But I think one of the reasons behind this should be explained a bit more.

When you access an array, if the key does not exist, then a notice is triggered about the array key not existing. As such, you should always make sure you are 100% certain that an array key exists. The only way to do this on user controlled inputs is explicity checked. The filter_input method is a favorite of many, but if you wanted to it "raw", you can use isset.

$val = (isset($_GET['val'])) ? $_GET['val'] : null;

It's guaranteed that every value of a $_GET/$_POST/etc array will be either a string or an array, thus it's a good idea to check type too. Using an array as a string will trigger quite a few errors in most scripts.

$val = (isset($_GET['val']) && is_string($_GET['val'])) ? $_GET['val'] : null;

array_key_exists is worth mentioning here too. It does basically the same thing, except it can be used when a value in an array may be false.

$a = array('foo' => null);

if (isset($a)) { echo 'isset'; }
if (array_key_exists('foo', $a)) { echo 'exists'; }

exists will echo here, but isset will not since isset(null) === false.

Separation of concerns

showerhead explained the concept well, but I disagree with his execution a bit.

As part of having only one responsibility, a function should have no inner depencies. What I mean by this is that a function that is not related to direct database interaction should not even have to know that a database exists. (Obviously at some point in your code, this will have to be broken, but it's more a general idea than a code-law.)

In more technically, slightly more vague terms, if a function has any dependencies, they should be provided to a function rather than a function finding those dependencies itself. ("dependency injection" is typically what this is called.)

I would consider rewriting your code like:

function output(array $recordsA, array $recordsB, $authorized, $module)
{

    if($authorized)
    {
        include('/path/to/' . $module . '.php');
    }

    echo "<ul>"

    foreach ($recordsA as $record) {
        echo "<li>" . htmlspecialchars($row['description']) . "</li>";
    }

    echo "</ul><br><ul>";

    foreach ($recordsB as $record) {
        $status = ($authorized) ? htmlspecialchars($record['status']) : 'N/A';
        $description = htmlspecialchars($record['description']);
        echo "<li>{$description} ({$status})</li>";
    }

    echo "</ul>";

}

Then the usage of the function would be:

$recordsAQuery = mysql_query("...");
$recordsA = array();
while ($row = mysql_fetch_assoc($recordsAQuery)) {
    $recordsA[] = $row;
}

$recordsBQuery = mysql_query("...");
...

$module = ...;

output($recordsA, $recordsB, is_authorized(), $module);

I would also be tempted to abstract yet more. Specifically, the database stuff could be abstracted away a bit more depending on how carried away one was.

function queryAndFetchAll($query, $conn = null)
{
    $res = mysql_query($query);
    if (!$res) {
        throw new Exception("mysql_query failed.  Query: [{$query}]. Error: [" . mysql_error($conn) . "]");
    }
    $records = array();
    while ($row = mysql_fetch_assoc($res)) {
        $records[] = $row;
    }
    return $records;
}

$recordsA = queryAndFetchAll("SELECT ...");
$recordsB = queryAndFetchAll("SELECT ...");

Note that I would name recordsA and recordsB something more descriptive. (I also might consider breaking this function into two functions.)

And, depending on what exactly the module file is doing, I might try to abstract that out of the function.


On a side note, the PDOStatement is Iterable, so you can pass it and the function can use it silently as an array.

If you're not familiar with PDO this probably won't have much meaning, but hopefully the gist is there:

$recAStmt = $db->prepare("SELECT * FROM main WHERE id > ?");
$recAStmt->execute(array($id));
$recBstmt = ...;
output($recAStmt, $recBStmt, ...);

output

What is being output? I would consider naming this something like renderStatuses. (Though if you were to rename it render*, I would expect it to return content instead of echoing.)

echo vs return

That brings me to my next point :).

I avoid outputting to the client except for in my main view code. What if for some reason you want to store this code to a variable instead of echoing it immediately? If you want to echo immediately, it's easy enough to do echo f();, but storing and then echoing is quite a pain. (ob_start(); f(); $f = ob_get_flush();) This does tend to make functions take a bit longer to write, but it's typically worth it.

SELECT *

This one is being pretty picky, however, I favor explicitly listing the column name. There's a tiny performance benefit (even if selecting all columns), but more so than that, I would rather my queries break in the future than my PHP code silently mess up.

Consider if you decided to rename your status column. Your code would silently break. You'd start getting undefined index notices, and nothing would be output correctly.

However, if you had selected all of the columns individually, the query would fail, and you'd likely notice the error a lot quicker.

\$\endgroup\$
5
\$\begingroup\$
  • magic strings,
  • DB connection not closed,
  • DB errors not handled,
  • no prepared statements with safe query parameters,
  • injectable request and query parameters,
  • mix of UI and logic code,
  • no HTML encoding,
  • 2 of the 3 comments are very redundant with the line they comment.
\$\endgroup\$
  • \$\begingroup\$ +1. However: "no prepared statements with safe query parameters" Not strictly necessary, but yes, PDO or MySQLi is much better than mysql_*. And for "DB connection not closed" -- scripting languages are used for a reason :). (A much better critique might be that some random output method has no business making a DB connection in the first place. \$\endgroup\$ – Corbin Jun 18 '12 at 4:57
  • \$\begingroup\$ @Corbin: thanks. Regarding the better critique, that's what I mean by the mix of UI and logic code. That's utterly wrong. I'm not sure I get the point about scripting languages. If you mean that it's OK to whip up a script to query a DB and not care to close the DB or check errors, that's fair enough, but then I'm not sure the OP would ask for a review. \$\endgroup\$ – haylem Jun 18 '12 at 4:59
  • \$\begingroup\$ @Corbin: also, you're right, prepared statements are not necessary, it's more the lack of parameterized queries that bugged me at first sight. (also OK, I guess, if it's a script for personal use, but that coupled with the _GET/_REQUEST makes me think there's really potential for trouble :) ) \$\endgroup\$ – haylem Jun 18 '12 at 5:01
  • \$\begingroup\$ Checking for errors is definitely necessary, however, assuming the script is short lived, there's really no point in explicitly closing the connection. mysql_connect resources have a bit special handling, but most resources will be closed automatically when the scope of the function ends. (We should probably just accept this as opinion, really. There's definitely no down side to explicitly closing the connection. From a purely logical point of view, I can't particularly defend my laziness. I'm just lazy :p.) \$\endgroup\$ – Corbin Jun 18 '12 at 5:02
  • \$\begingroup\$ Even for personal use I would escape query parameters. Escaping isn't just about security; it's also about correctness. A user (or yourself if it's a personal script) may forget that O'Reilly is going to break the script. \$\endgroup\$ – Corbin Jun 18 '12 at 5:03
1
\$\begingroup\$

REQUEST / GET Array

Would suggest not using the REQUEST array. Because this data is so easy to hijack it is a poor choice. Better to simply use the right array (GET, POST, COOKIE, SESSION) and if it could be one of a number of them, then set up statement(s) to return the correct one. Also, I would not immediately use any value from any user provided array directly in my program. It should be sanitized and validated first. Check out PHP's filter_input() function. Here's an example of it.

$id = filter_input( INPUT_GET, 'id', INPUT_SANITIZE_NUMBER_INT );
//Equivalent to sanitizing and validating $id = $_GET['id'];

Variables

Make sure your variables are available for each instance a function might be run! Right now $authorized is only ever defined if the contents of that first if statement are processed. Because of this, if is_authorized() fails then your second mysql while loop would produce errors because $authorized was never defined. Also, that variable need not be separate from that function. is_authorized() obviously returns a boolean, or something that can be used as a boolean, should be the former rather than the later. Since $authorized needs to always be set, and its value is dependent upon that function, why not just set it to the return value of that function?

$authorized = is_authorized();
if( $authorized ) { }

Ternary Operations

Ternary operations are good for simple things, such as setting a variable. What they are not good for is long statements, string concatenation, and other such more complicated tasks. They can do it, but they lead to illegible code and mistakes. I mainly point this out because your ternary operation is flawed. Currently it will not end the parenthesis or end the list item because that is considered part of the "else" statement in the ternary operation. A better way to perform your ternary operation would be to remove it from the string and declare it beforehand. This will prevent all such mishaps in the future and is easier to read.

$status = $authorized ? $row['status'] : 'N/A';

PHP Strings

Once this is done then you can insert it into the string. Let me also point out that there are two types of PHP strings. Strings that are processed for variables and entities and strings that aren't. The first uses double quotes ("") to enclose a string and add a little bit of overhead to PHP, unnoticeable really, so that it knows to search that string for items to escape. The later uses single quotes ('') and is only ever used for holding static strings. There is no overhead to these strings because PHP ignores them. That being said, you are telling PHP that all of your string have variables and entities in them to be escaped, so PHP has to process them. As I said, the overhead isn't really noticeable, but it is there and unnecessary. So I would write that echo statement you had your ternary operation in like so:

echo "<li>{$row['description']}($status)</li>";
//OR
echo '<li>' . $row['description'] . "($status)</li>";
//OR EVEN
echo '<li>' . $row['description'] . '(' . $status . ')</li>';

Out of all of those I prefer the first implementation, but it is a matter of preference. Honestly this bit on strings might be considered preference as well, but I believe this optimization is standard and that my examples are also easier to read, minus the third which is more or less the same.

Haylem and Corbin

I mostly agree with them, but I believe an explanation is in order instead of just all these terms you might not have heard of before, or don't understand.

"no prepared statements with safe query parameters" Not strictly necessary, but yes, PDO or MySQLi is much better than mysql_*.

From my understanding mysql is also getting deprecated. But maybe I've heard wrong. Don't do much with MySQL to know, so keep your eye out for this. It is good for learning the basics on though. Continue what you are doing, just be aware that there are better, and safer (security wise), solutions out there that you should eventually look into. And that eventually you may have to change any mysql code.

DB connection not closed" -- scripting languages are used for a reason

I still agree that it should be closed. This will prevent the connection from being used after you are done with it. Not sure how it is done, but I still think it is a valid concern. Sure PHP is very forgiving and should close it for you, but I believe writing the extra line won't hurt and can only help.

some random output method has no business making a DB connection in the first place

What they are talking about here is called Separation of Concerns. In my opinion its the first step to learning OOP. Don't be concerned with OOP quite yet, but it is a good idea to start learning this concept. Basically it states that functions should only be concerned with one thing. A well written function can be described using its name only and should not do anything its name does not imply. From your output function I would expect it to only "output" information, never to connect to a database and retrieve it. I would instead expect to see a connectToDB() function, or something similar. And maybe a fetch() function for those mysql queries. So your output function would more closely resemble this (all suggestions implemented here):

connectToDB();

$id = filter_input( INPUT_GET, 'id', INPUT_SANITIZE_NUMBER_INT );
$res1 = fetch(" SELECT * FROM main WHERE id > $id;");
$res2 = fetch(" SELECT * FROM main WHERE id < $id;");

function output( $res1 , $res2 )
{
    // Check authorization
    $authorized = is_authorized();
    if( $authorized )
    {
        include('/path/to/' . $_REQUEST['module'] . '.php');
    }
    echo "<ul>"

    while( $row = $res1 )
    {
        echo "<li>".$row['description']."</li>";
    }

    echo "</ul><br><ul>";

    while( $row = $res2 )
    {
        // Display the status if it is authorized, othewise display N/A
        $status = $authorized ? $row['status'] : "N/A";
        echo "<li>{$row['description']}($status)</li>";
    }

    echo "</ul>";
}

Even this is not fully optimized, but it will give you an idea and a place to start. Further optimization might see those while statements turned into a single function so that it could be reused. Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ (Warning: mini-rant incoming :p) The single vs double quotes psuedo-myth needs to die. There is no meaningful performance impact at all. If 1ns really matters, then PHP is the wrong language. Single quotes should be used when they're more readable, and double quotes should be used when they're more readable. That's it. \$\endgroup\$ – Corbin Jun 19 '12 at 0:14
  • \$\begingroup\$ Oh, and the while( $row = $res1 ) will be an infinite loop. \$\endgroup\$ – Corbin Jun 19 '12 at 0:15
  • \$\begingroup\$ @Corbin: Not if it performs the mysql_fetch_assoc() as well. \$\endgroup\$ – mseancole Jun 19 '12 at 12:59
  • \$\begingroup\$ @Corbin: It's not really a "myth" if it does indeed effect performance. However, I did specifically say that it was negligible. I'm not really implying to do this for the speed advantage but because PHP offers a built-in string processor and it is not being used in this script. I gave reasons to use the processor version, which are sound if you realize that by using double quotes with no reason you will find that you need to escape characters you would not need to if you were just using single quotes. Sometimes it does not matter, then I say use whichever quotes you want. \$\endgroup\$ – mseancole Jun 19 '12 at 13:05
  • 1
    \$\begingroup\$ To anyone curious the mysql extension is definitely being deprecated: kr.php.net/manual/en/mysqlinfo.api.choosing.php. (I'm fairly sure this page just appeared in the past day or two.) \$\endgroup\$ – Corbin Jun 22 '12 at 8:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.