Basic user registration code

Question

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:

<?php
    require_once 'login.php';
    include 'header.php';

    echo <<<_REGFORM
    <form action="registration.php" method ="post"><pre>

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

    Username          <input type="text" name="regusername"/>
    Password          <input type="password" name="regpassword"/>
    Retype Password   <input type="password" name ="checkregpassword"/>
               <input type="submit" value="Register"/>
               <input type="hidden" name="clicked" value=yes/>

    </pre></form>
    _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";


                }
                else echo "Your passwords do not match";
            }

            else echo "Username and password must be atleast 6 letters in length and letters a-z, A-Z and numbers 0-9 are accepted <br />";    
        }
        else echo "Please fill in a valid username and password";
    }



    ?>

Answer 1

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) {
    //username's must be >= 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>

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 be used.

if {} else { statement }

Looks clearer and is less error prone than:

if {} else statement

html element's attributes should always be in quotes

value=yes should be value="yes"

MD5

Hashing algorithms should be computationally expensive enough that a brute force attack is infeasbile. Due to rainbow tables and other factors, a single MD5 run of an alphanumeric password is not considered secure.

Consider using something like bcrypt that is a bit heavier algorithm, or at least use something like 512 bit SHA repeated a thousand (or more!) times.

Let f be your hashing function. Let S = the set of all possible passwords. The absolute worst case time to brute force a password, p, such that p is in S is size of S * time for f(x)

If f = MD5 then the time is very small. MD5 and even SHA are considered fairly fast hashes. If you use a an algorithm such as one of those, you should at least repeat it. This way f becomes a much longer operation.

There's a few really good posts on StackOverflow about this, so I'll try to find some and link them.

This is a pretty good explanation (thanks to luiscubal for finding the link).

More graceful error handling

For the errors, I would probably handle them a bit better than the old or die construct that tends to go along with the mysql_ functions.

The idiom I would follow (though I would use PDO with exceptions), would be something like:

$query = "...";
$resource = mysql_query($query);
if (!$resource) {
    //log the error including all necessary details
    //output a fairly vague error the user
}

trigger_error can be useful for this.

Error handling suggestion

I like to validate everything separately and independently such that all errors can be output at once. For example, if my username is only 4 characters and my passwords don't match, it would be nice to see both errors at once instead of having to fix one error, resubmit the form, fix the second error then resubmit the form.

This is by no means perfect, but the structure I typically use for error handling is:

$errors = array();
$username = $password = null;

if ($_SERVER['REQUEST_METHOD'] === "POST") {
    $username = ...;
    $password = ...;
    if (strlen($username) < 5) {
        $errors['username'][] = "Usernames must be 5 or more characters";
    }
    if (strlen($username) && !ctype_alnum($username)) {
        $errors['username'][] = "Usernames must be alphanumeric";
    }
    if (strlen($password) < 5) {
        $errors['password'][] = "Passwords must be 5 or more characters";
    }
    //More validations
    if (!count($errors)) {
        //No errors, so $username and $password are valid
        $insertionQuery = sprintf("INSERT INTO users (username, password) VALUES ('%s', '%s')", mysql_real_escape_string($username), mysql_real_escape_string($password));
        $insertionResult = mysql_query($insertionQuery);
        if (!$insertionResult) {
            trigger_error("MySQL query failed.  Query: [{$insertionQuery}].  Error: [" . mysql_error() . "]", E_USER_WARNING);
            $errors['db'][] = "There was an unexpected error. Please try again.";
        }
    }
}

Then, to render the errors, I typically just create a <ul> with each error as a <li>:

if (count($errors)) {
    echo '<ul>';
    foreach ($errors as $element => $errs) {
        foreach ($errs as $err) {
            echo '<li>' . htmlspecialchars($err) . '</li>'
        }
    }
    echo '</ul>';
}

The element name being the array key is useful because it means you can do things like style elements a certain way if they had an error:

<input type="text" name="username" value="<?php echo htmlspecialchars($username);"<?php if (array_key_exists('username', $errors)) { echo ' class="error"'; } ?>>

You could even take the idea farther and render errors around elements.

In general, it's a lot more flexible to aggregate the errors instead of immediately outputting them.

This philosophy touches into the area of MVC and separation of concerns. MVC isn't particularly important for now, but the general idea of it (and in part SoC) is that each area of code should be concerned with one specific thing and nothing else.

A slight extension of this is the idea that logic and presentation should be separate. Doing this allows many more degrees of freedom, and typically better code. Consider for example if you wanted to have an AJAX interface for submitting the form, but still have an HTML fallback for users with JavaScript disabled. If you were storing errors, you could do something simple like:

if (!empty($_POST['ajax'])) {
    echo json_encode(array('result' => 'errors', 'errors' => $errors));
    //set some kind of flag so that html output will not happen
    //(or, if it's a small script, I might just use `exit;`)
}

//Then later, if it's not AJAX
//render the errors as HTML inline in the form

If you intermix your logic and presentation code, the code is forever stuck rendering in exactly one way. There's no way to reuse the same logic with a different presentation. If they are separated, however, then the same logic can be used with infinite presentations.

Variable reuse

I'm definitely in the minority on this one, but I don't like to reuse variable names. I would call the repeated query and result variables something like $existsQuery and $insertionQuery. Nothing wrong at all with reusing the same name, just a personal preference (though the more descriptive a name, the better [within reasonable limits]).

header.php

This is not a very descriptive name. I would consider calling it something like database.php or connect.php or something. When I think of headers, I think content, not code.

Also, since the database connection is required for the page to function, require should be used instead of include. A failed include will allow execution to continue, whereas a failed require will kill execution.

mysql_select_db

This should be with your database connection code.

login.php

No idea what "Login.php defines database log in numbers" means? I'm a bit confused though about what this could be doing. Also not sure why require_once instead of require?

Answer 2

Agree entirely with Corbin's post, these are just additions.

A point on preference rather than efficiency, but I find it odd to have plain, unaltered HTML echoed in PHP. I personally would escape it, but again, preference. At least you are using heredoc :)

I'm assuming $_POST['clicked'] is the same concept as checking for a submit button press. This is not very reliable due to the fact that most users do not click those buttons and instead press enter. A form submitted in this manor will not register the "submit" because it was not pressed. If this is what you are doing, I'd be wary of it. There are two better ways to do this. The first is the "right" way, while the second is a hack that works just as well.

if( $_SERVER[ 'REQUEST_METHOD' ] === 'POST' ) { }
//OR
if( $_POST ) { }

Checking for the boolean value of $_POST['regusername'] and $_POST['regpassword'] is not the same as ensuring that they are set. This method will return errors in your code if those keys are not set. It might still work, but any error is a bad error and should be fixed. Better to use isset() and verify they exist before trying to do anything with them.

Long statements can become difficult to read, especially when combined with numerous operations. To lessen the length and make code more legible you can start by removing some of the functions from the statement.

$passwordAlphaNum = ctype_alnum($_POST['regpassword']);
$usernameAlphaNum = ctype_alnum($_POST['regusername']);
$usernameLength = strlen($_POST['regusername']);
$passwordLength = strlen($_POST['regpassword']);
if ($passwordAlphaNum && $usernameAlphaNum && ($usernameLength > 5) && ($passwordLength > 5)){}

You can reduce line length even more by breaking up statements logically. Currently the above statement does two things. First it confirms that a username and password contains only alpha-numeric characters and that their lengths are greater than five. So...

if ($passwordAlphaNum && $usernameAlphaNum){
    if(($usernameLength > 5) && ($passwordLength > 5)) {}
}

This is much easier to read. Of course you can break this down ever further, as Corbin has done in his demonstration for error reporting. I actually prefer this method as it allows for the custom error messages. But it all depends upon your implementation.

Now, in my opinion you have set the username and password to variables a tad too late. You've performed all these validation checks on them alreday, having to rewrite the post array and key for each element. Better to set it after ensuring that it is set then do all the validation on it. Another way you can go about this is to use PHP's filter_input() method. Corbin mentioned this but did not go into detail. This removes the necessity for checking if a value is set because that function will return FALSE in this instance. This also automatically sanitizes input based on the defined flag. So you can remove a number of those if statements and those ctype_alnum() functions and at the same time reduce the amoutn of times you need to use that post array. (All for one down payment of $19.95!) So before even the first if statement you can do this.

$username = filter_input( INPUT_POST, 'regusername', FILTER_SANITIZE_STRING );
$password = filter_input( INPUT_POST, 'regusername', FILTER_SANITIZE_STRING );
if( $username && $password ) {
    $usernameLength = strlen( $username );
    $passwordLength = strlen( $password );
    if( ( $usernameLength > 5 ) && ( $passwordLength > 5 ) ) { }
}

Answer 3

+1 to @showerhead and @Corbin, and:

1. Flattening arrow code would improve readability a lot.

2. Create local variables for $_POST['regusername'] and $_POST['regpassword']. These two statements are repeating too much.

3. I'd create an isValidPassword() and an isValidUsername() instead of the long condition.

   function isValidUsername($username) {
       if (!ctype_alnum($username)) {
           return false;
       }
       if (strlen($username]) <= 5)) {
           return false;
       }
       return true;
   }
   
   function isValidPassword($password) {
       if (!ctype_alnum($password])) {
           return false;
       }
       if (strlen($password]) <= 5)) {
           return false;
       }
       return true;
   }
   

   It's easier to follow and read.