Posted by Michael Fesser on 07/22/07 19:09

..oO(Martin Adams)

>Just thought that it might be of use to someone starting out in PHP. A
>short while ago I created some video tutorials on PHP development (on
>Windows XP Pro) using Eclipse and the PHP plugin (those are free to
>download).
>
>It covers some basic PHP concepts such as form posting, etc and some MySQL
>concepts such as writing an address book application.

I can't resist to write some comments about the MySQL address book
tutorial. There are some things in it that are really ugly, others
should _never_ be done, especially not in a beginner's tutorial. I admit
that some of them might not be an issue in this simple example, but
nevertheless things should be done properly right from the start.

In fact, this tutorial shows a beginner how to write an extremely
insecure application!

critical:
---------
* using the 'root' account for normal work
* not preventing SQL injection, doubling apostrophes with str_replace()
is no protection, the minimum would be mysql_real_escape_string()

dangerous:
----------
* using "SELECT MAX(id) + 1 ..." to get the next ID for a record insert:
That's a race condition.
* printing out plain MySQL error messages: This can reveal internal
details to an attacker.
* not using htmlspecialchars() when printing out DB data to a page: This
can be abused for cross-site-scripting.

ugly:
-----
* using the proprietary backticks around each identifier
* using $_REQUEST instead of $_GET or $_POST
* relying on the existance of a field value in $_REQUEST just because
it's in the HTML: Every(!) value has to be checked if it exists.
* using mysql_fetch_array() to retrieve values from a result set: This
returns all fields twice by default, which not only wastes memory, but
might also lead to errors in some cases.
* mixing all kinds of quotes and concatenation: In another tutorial you
said that you don't like embedding variables into a double-quoted
string because it might confuse you. Well, that's what syntax high-
lighting is for - it exactly shows you what is a string and what is a
variable. Instead you always use concatenation. But now tell me -
what is more confusing for a beginner:

"INSERT INTO ..." .
"`id` = " . $id . ", " .
"`name` = '" . $name . "', " .
"`email` = '" . $email . "', "

or a simple

"INSERT INTO ...
id = $id,
name = '$name',
email = '$email'"

general notes:
--------------
* braces when using print or require_once: They're not needed.

Micha

[Back to original message]