views:

80

answers:

2

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

A: 

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.

Daniel Egeberg
well abt indentation it was there but positing it here caused it to break,. thnx of correction of htmlentities. About Mysqli and PDO will be discussing later. thnx
KoolKabin
To whoever downvoted me: I would appreciate a comment. Thanks.
Daniel Egeberg
+1  A: 

Never use GET/POST variables DIRECTLY in your queries

Use:

  • Proper type casting with intval
  • Use mysql_real_escape_string

Better:

You must read:

Sarfraz
*Never*, huh? You might want to let the developers of phpMyAdmin know that they shouldn't allow the users to execute queries :P
Daniel Egeberg
@Daniel Egeberg: Hun ! phpMyAdmin is under your control not a website out publicly :)
Sarfraz
is prepared statments available for old mysql extensions? I think type casting intval and mysql_real_escape_string is covered by point no. 2
KoolKabin
Well, then it's not "never", is it?
Daniel Egeberg
The OP actually is aware of that already. See *#2 Bad practise of using $_GET variable without checking it for sql injection.* So why answer it again?
Gordon
@Daniel Egeberg: I would consider it **never** for public site and probably **not** for your phpmyadmin.
Sarfraz
@Gordon: That's true but specifically wanted to suggest about prepared statements :)
Sarfraz
@sAc: I am aware of prepared statements but wonder are they available in old mysql extension. I guess they are only from mysqli
KoolKabin
@KoolKabin: Yes they are available in mysqli.
Sarfraz