What are the bad things present in following code?

Question

What are the bad things present in flowing code:

print "<ul>"
$conn = mysql_connect( "localhost:8080", "root", "admin" );
mysql_select_db( "testdb", $conn ); #selects a database
$q = " SELECT * FROM table1 WHERE id > " . $_GET["id"]. ";";
$res = mysql_query( $q, $conn);
while( $row = mysql_fetch_assoc($res) )
{
print "<li>".$row['description']."</li>";
}
print "</ul><br><ul>";
$q = " SELECT * FROM table1 WHERE id < " . $_GET["id"]. ";";
$res = mysql_query( $q, $conn);
while( $row = mysql_fetch_assoc($res) )
{
print "<li>".$row['description']."</li>";
}
print "</ul>";

as per me the bad things are:

1.) Using hardcoded values for database connection

2.) Bad practise of using $_GET variable without checking it for sql injection.

3.) Should print the database value useing htmlencode

4.) should use <br/> instead of <br>

5.) bad practise of mixing html presentation layer and database interaction layer something like controller

Answer 1

I'll agree with 1, 2, 3 (though it's htmlentities) and 5. Number 4 is bogus though, using HTML is entirely legitimate.

In addition to those points, horrible formatting; there is no indentation whatsoever.

Also, use MySQLi or PDO instead of the old mysql extension.

Answer 2

Never use GET/POST variables DIRECTLY in your queries

Use:

• Proper type casting with intval
• Use mysql_real_escape_string

Better:

• Use prepared statements

You must read:

• PHP Security Guide
• Top 7 PHP Security Blunders
• Programming PHP with Security in Mind
• PHP Security