Basic user registration code

My first attempt at a user registration code.

Login.php defines database log in variables (e.g. Database name, Table name, etc)
Header.php connects to the database.

I used ctype_alum for security of inputs. I also know that I shouldn't use mysql_error() for security issues but I've used the function as a placeholder.

Would love some feedback on possible improvements:

```

Register an account:
Usernames and passwords must be at least 6 letters in length comprising of only alphanumeric characters.

Username
Password
Retype Password




_REGFORM;

if ($_POST['clicked']){
if ($_POST['regusername'] && $_POST['regpassword']){
if (ctype_alnum($_POST['regpassword']) && ctype_alnum($_POST['regusername']) && (strlen($_POST['regusername']) > 5) && (strlen($_POST['regpassword']) > 5)){
if ($_POST['regpassword'] === $_POST['checkregpassword']) {
// Insert code to check if the username exists
// Insert code to finally create the account!

$username = $_POST['regusername'];
$password = md5($_POST['regpassword']);

mysql_select_db($db_database) or die("Unable to select database: ".mysql_error());

$query = "SELECT * FROM userlogin WHERE username='$username'";
$result = mysql_query($query) or die("Database access failed: ".mysql_error);

if (!mysql_num_rows($result)) {
$query = "INSERT INTO userlogin VALUES('','$username','$password')";
$result = mysql_query($query) or die("Unable to create account: ".mysql_error());

echo "Thanks ".$username.", your account has been created!";
}
else echo"Username Unavailable, please try another username";

}

I think this is better than about 99% of first tries I've ever seen!

Anyway, I've tried my best to address as many things as I saw, whether major or extremely minor. If you have any questions on any points or want to clarify or defend any claims, let me know and I will expand the answer.

Assuming array indexes exist

Any array that comes from user input should never be directly accessed. $_POST['key'] is not safe since you can't know for sure whether or not the user supplied the $_POST['key'].

With arrays that can contain null values, you must use array_key_exists, but since input arrays like $_POST and $_GET will always have either strings or arrays, you can safely use isset.

The following is how I tend to access request driven arrays:

$username = (isset($_POST['username']) && is_string($_POST['username'])) ? $_POST['username'] : null;
if (strlen($username) = 5 charactes
} else if (strlen($username > 32)) {
    //username's must be < 32 characters
}

//If you wanted to check if the user even provided a username:
if ($username === null) {
    //A username element of the form was not provided.  Depending on specific context, this may imply form tampering.
}

Note that the is_string check is necessary so that the user cannot maliciously pass an array to the script. If that were to happen, then the strlen call would complain about not expecting an array. Unfortunately you have to be rather paranoid about anything coming from users.

There's a longer discussion of this here in the last section.

It's also worth noting that some people prefer a filter_input approach.

SQL injection

There is no SQL injection possible in your code, but that may be by accident. When putting strings into queries, it's good practice to always run them through mysql_real_escape_string. That way if you change your business rules in a way that does make SQL injection possible, you don't have to worry about the change cascading down to your queries and opening a security hole.

More information about SQL injection can be found in a post I wrote the other day, here.

(Sorry if I'm assuming that you don't know about SQL injection and you do.)

MySQL Extension

The mysql_* extension is quite dated, and PDO offers many advantages.

• Offers a cleaner API

• Offers better error handling (the ability to have failed queries throw exceptions is wonderful)

• Handles transactions SQL-dialect agnostically (for the most part)

• Different RDBMS can be changed out relatively easily (switching from mysql_* to PostreSQL is extremely painful. Switching from PDO with MySQL to PDO with PostreSQL isn't pleasant, but is care is taken to stay as dialect-agnostic as possible, it's reasonably easy)

• Prepared statements

You can find more information here under the PDO section.

`

pre can be used to arrange spacing, however you should be aware that it typically uses a fixed width font, and there's a few other quirks with it (you can probably style it to use a different font though). I suppose there's technically nothing wrong with it, but pre doesn't strike me as a good long term method for arranging elements.

ctype_alnum for a password

Why restrict users to a a-zA-Z0-9 password? I'm a firm believe that passwords should be allowed to be just about anything. Some restrictions may be in order, such as ASCII only or something of that nature, but if a user wants his password to be this is my password, then why not? You're hashing it anyway, so the length doesn't matter. By the same logic, why not allow non alpha numeric characters? It's not like HTTP or PHP will have any issues handling things like !@#$%^&*()|_+=- and so on.

list column fields in inserts

$query = "INSERT INTO userlogin VALUES('','$username','$password')";

Listing columns explicitly allows for better maintainability and clarity. What if in the future you add a birthday column after the username column? Suddenly you're trying to insert passwords into the birthday column. Also, if you're going to put an auto incrementing column in a query, it should be null.

I would write the query like this:

$query = "INSERT INTO userlogin (username, password) VALUES ('$username','$password')";

userlogin

This sounds like a logging table to me. This sounds like it contains records of users logging in, not of actual user records. I would consider renaming this table to something like users or user_accounts.

escape html where necessary

echo "Thanks ".$username.", your account has been created!";

Once again, since it's limited to alphanumeric, this is definitely harmless, however, preparing for the future never hurts. If at some point you allow users to create names that contain &`, this could output invalid html.

(Once again, sorry if I'm telling you something you already know.)

always use brackets

It's hard to see on a short script with fairly simple logic, but I'm a firm believer that braces should always

StackExchange Code Review Q#12757, answer score: 14