Database report in PHP

Just a small PHP exercise to write some simple reports, I thought I'd do it without a framework for fun and profit. I'm particularly concerned about the potential for SQL injection, since the user could pass their own values to $_GET['status'].

I also understand that mysql_connect() et. al are deprecated in PHP since 5.5, but I assume better equivalents work the same way.

Error performing query: " . mysql_error() . "");
        exit();
}

mysql_close($conn);

?>

        RM Rep
        
                body { font-family: Arial, sans-serif; }
                td { padding: 0px 5px; }
        

        
                
                ' . $row['name'] . '');
                        }
                ?>
                
                
        

        
        
        

        

        ");
                        echo("" . $row['id'] . "");
                        echo("" . $row['subject'] . "");
                        echo("");
                }
        ?>
        

I also understand that mysql_connect() et. al are deprecated in PHP since 5.5, but I assume better equivalents work the same way.

Sort of. MySQLi can work a lot like the the mysql extension, but personally I prefer PDO. Whereas MySQLi has both an OO and a procedural API, PDO has only an OO one. PDO can interact with more than just MySQL though.

Due to differences in SQL dialects, it's rare for it to be possible to just change out the driver and have everything work. It is a lot closer to cross compatibility than a specific RDBMS's API. Also, you can use your knowledge of the PDO API across multiple database systems. Whether it's MySQL, sqlite, SQL Server, etc, the general API is the same.

Anyway, whether you choose MySQLi or PDO, you should definitely stop using ext/mysql.

As for SQL injection: your code is basically a poster child for it.

$choice = isset($_GET['status']) ? $_GET['status'] : '7';
$result = mysql_query("SELECT id, subject FROM issues WHERE status_id = " . $choice, $conn);

What if I go to page.php?status=0 OR 1=1? The query becomes:

SELECT id, subject FROM issues WHERE status_id = 0 OR 1=1

This is a fairly benevolent example since all this will do is ignore the status filter. I've seen some very clever exploits though. Given enough time and determination, this small exploit could be ripped open into horrible, horrible things.

The easiest solution to SQL injection is to always stay aware that SQL is a language like any other (well, not on a technical level, but on a superficial level).

In particular, just remember to either escape content if it's being put into the query as a string, or cast it if it's being put in as something else.

For example:

$choice = isset($_GET['status']) ? (int) $_GET['status'] : 7;

$choice is now safe to put into an SQL statement.

This StackOverflow thread gives tons of options for properly handling SQL queries.

if (!$options || !$result || !$chosen_status)
{
    echo("Error performing query: " . mysql_error() . "");
    exit();
}

If more than one query fails, this will only output the error for the last one. Additionally, using exit for rendering errors is bad practice. In a file this simple, there's really not much of a better option without going way out of your way. What I tend to do in larger applications though is let uncaught exceptions (in other words, either the ones that I expect to never happen or the ones that are fatal) bubble up to the top at which point the request is transparently piped to an error controller (basically, I've copied the approach of most PHP MVC frameworks). If you wanted to use a similar approach, you could basically wrap exceptions over the MySQL API. That get's ugly fast though.

Oh, also, the mysql_error should be passed through htmlspecialchars. I'll add more to this note in a minute.

I'm not sure if it's technically valid to close a MySQL connection and then read the result sets. I suspect that this is not valid, and that if it works, it's just by luck.

Even when not going full-blown MVC, it can be very beneficial to separate logic or manipulation from the view. In particular, all of the mysql calls in the HTML parts, I would put in the PHP parts. If your view is only looping over an array, the source of the array can be changed without the view having to know about it. (Side note: depending on how strictly we're using terminology, the HTML part is probably better not called a view.)

while ($row = mysql_fetch_array($options, MYSQL_ASSOC))

You might as well us mysql_fetch_assoc.

echo('' . $row['name'] . '');

Anything being put into HTML needs to be run through htmlspecialchars. This comes back to being aware of context. It's just the rules of HTML. Certain things must be escaped in the same way that things in SQL or PHP must be escaped.

(I'm also not a fan of the echo() form of echo, but that's mostly opinion.)

echo '' . htmlspecialchars($row['name']) . '';

A lot of people will say that the htmlspecialchars on the id is pointless since it's going to be an int. While that's true, that's only for now. With a primary key column it's not likely to happen (in fact it probably shouldn't happen), but I've been in situations before where an int column has been changed to a varchar and we've had problems with the HTML escaping after that. Considering the overhead of htmlspecialchars is tiny, might as well call it on everything being put into HTML.

There's a few more comments and suggestions I have. I was originally going to add them as comments to the code, but I got a bit carried away and decided to rewrite it with PDO.

```
PDO::ERRMODE_EXCEPTION
));

//The error mode being exception means that exceptions will be thrown if a query fails
//(or some other kind of error happens). This tends to be more convenient since you don't
//have to explicitly check for unexpected errors -- either everything executes fine, or the script
//crashes

StackExchange Code Review Q#20283, answer score: 3