Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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";
    }



    ?>
share|improve this question
    
I'll write a proper response later if no one else beats me to it, but: You're missing the () after one of the mysql_error calls. Also, pre should typically be avoided for styling. Also, everything other than the first section of this applies to your question: codereview.stackexchange.com/questions/12647/… –  Corbin Jun 20 '12 at 4:25
add comment

3 Answers

up vote 13 down vote accepted

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?

share|improve this answer
3  
+1 for awesome explanation for newbies –  drupality Jun 20 '12 at 9:52
    
As for hashing, bcrypt seems to be a very popular method to handle passwords. See stackoverflow.com/a/401684/32775 Also, md5 without salt is a big NO (even if it's obviously better than plain text) –  luiscubal Jun 20 '12 at 17:51
    
@luiscubal Ah! Good link! Have edited it into the answer. –  Corbin Jun 21 '12 at 1:09
    
And @palacsint thanks for fixing my lazy formatting :p –  Corbin Jun 21 '12 at 1:10
1  
Haven't had time to read this in detail yet, but THANK YOU! Great response. The encouragement is very motivating as well. With regards to the SQL injection, I knew a little about it, that's why I made the inputs only alphanumeric (as a temporary solutions until I learn more). I didn't want to think about security yet, that's a lesson for another day... probably. Amount of feedback is amazing...will review all feedback in detail once I get time. –  J Y Jun 21 '12 at 17:28
show 2 more comments

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 ) ) { }
}
share|improve this answer
1  
(I know I've mentioned this before, and I hate to keep bringing this up, but I'm quite curious about this...) A form submitted in this manor will not register the "submit" because it was not pressed. As far as I'm aware, an input of type submit does not have to be pressed to be considered "successful" (the term used to describe which elements are to be sent as form data). There is a clause that states that only the activated submit button should be considered successful for a form with more than 1, but no such clause about a form with 1. –  Corbin Jun 21 '12 at 1:07
    
In short, I'd be curious if you can provide an example of a recent browser that exhibits your mentioned behavior. Also, it's worth noting that checking if the request method is post, and checking a button was pressed (or if a hidden input is present) are not the same logical thing (though practically equivalent unless a page is handling multiple forms) (Potentially relevant: w3.org/TR/html401/interact/forms.html#successful-controls) (Also, for what it's worth, +1 :) ) –  Corbin Jun 21 '12 at 1:09
    
@Corbin: This is something I came upon recently, so I can't provide an example. But I have read enough posts where its claimed that this is a poor choice, even go so far as to claim bad programming, and gave that reason for it. I see that you are stressing "recent", however you can still find people using old browsers. While not common, it is still a possibility, and one that some people like to be aware of and include support for. I phrased that the way I did for this reason. –  mseancole Jun 21 '12 at 2:45
    
No these aren't the same logically. But, as you stated, they do practically the same thing. And multiple forms on the same page IS bad. Better to separate that logic. –  mseancole Jun 21 '12 at 2:45
1  
To be blunt, I'm not sure I believe that claim that there are any even remotely recent browsers that display this behavior. Hitting enter should result in the submit button being sent along. Though, for the simple fact that I can't seem to find any reference that that is required behavior, I suppose it may be better to avoid it. And if you meant that one section of PHP code handling multiple forms is bad, I would agree with that. "multiple forms on the same page IS bad", I however do not agree with. (And on that note, I promise I'll stop being an annoying pedant now :p) –  Corbin Jun 21 '12 at 3:10
add comment

+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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.