current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

Edit1: Edited in response to Corbin's feedback

This is an update of my user registration code. This is continuation and implementation of feedback found in this thread: Previous codereview post.

I would love some feedback! Just began to use the PDO object, I'm not sure if I'm using it efficiently. Are there any security issues? Is the myEncrypt function suitable enough or still a bit lacking?

initialise.php which is:

$db = new PDO("mysql:host=$db_hostname;dbname=$db_database", $db_username, $db_password) or 
die();

function myEncrypt($password){
    $i = "$password"."$salt1";
    $hash1 = md5($i);
    $j = "$salt2"."$hash1";
    $hash2 = md5($j);
    return $hash2;
}

Registration.php:

<?
require 'topmenu.php';

//View
echo <<<_REGFORM
<form align=center action="registration.php" method ="post"><pre>

Register an account:

Username          <input type="text" name="regusername"/>
Password          <input type="password" name="regpassword"/>
Retype Password   <input type="password" name ="checkregpassword"/>

  <input type="submit" value="Register"/>

</pre></form>
_REGFORM;




//Checks inputs for length requirements, if two passwords are the same, if filter was successful
if ($_SERVER['REQUEST_METHOD'] === "POST") {

    $usernameclean = mysql_escape_string($_POST['regusername']);
    $reg_password = mysql_escape_string($_POST['regpassword']);
    $reg_checkpassword = mysql_escape_string($_POST['checkregpassword']);

    if (isset($usernameclean, $reg_password, $reg_checkpassword)) 
     {
        if (strlen($usernameclean) < 5) {
            $errors['username'][] = "Usernames must be at least 5 characters in length.";
        }
        if (strlen($usernameclean) > 32) {
            $errors['username'][] = "Usernames have a maximum limit of 32 characters.";
        }
        if (strlen($reg_password) < 5) {
            $errors['password'][] = "Passwords must be at least 5 characters in length.";
        }
        if ($reg_password <> $reg_checkpassword) {
            $errors['password'][] = "Your passwords must be the same.";
        }
    }
    else
    {
        $errors['values'][] = "Please fill out all the fields.";
    }


    if (!count($errors)) {

        $hashedpassword = myEncrypt($reg_password);       //function defined in initialise.php
        $checkUsername_query = "SELECT user_id FROM users WHERE username='$usernameclean'";
        $checkUsername_result = $db->query($checkUsername_query)->fetch();


        if (!$checkUsername_result) 
            {
            //SETS UP USER ACCOUNTS, DEFINES 5 FILE SLOTS          
                try {
                    $db->beginTransaction();
                    $createUser_query = "INSERT INTO users(username,password) VALUES('$usernameclean','$hashedpassword')";
                    $createUser_result = $db->exec($createUser_query);
                    $getuserid_query = "SELECT user_id FROM users WHERE username='$usernameclean'";
                    $getuserid = $db->query($getuserid_query);
                    $user_id = $getuserid->fetch();

                    //ASSIGNS 5 File slots into "files" (File-information)
                    for ($i = 1; $i <= 5; $i++) {
                        $assignfileslots = "INSERT INTO files(user_id) VALUES('$user_id[0]')";
                        $db->exec($assignfileslots);
                    }

                    $db->commit();

                    echo "Thanks " . htmlspecialchars($usernameclean) . ", your account has been created! Please login.<br/>";
                } catch (PDOException $e) 
                {
                    $db->rollback();
                    $errors['database'][] = "You account could not be created. Please try again later.";
                }
            } else 
            {
                $errors['username'][] = "Your username is taken. Please enter another.";
            }
    }
    if (count($errors)) {
        echo '<ul>';
        foreach ($errors as $elements => $errs) {
            foreach ($errs as $err) {
                echo '<li>' . htmlspecialchars($err) . '</li>';
            }
        }
        echo '<ul>';
    }
}
?>
share|improve this question

2 Answers 2

up vote 3 down vote

Will edit this when I have time, but for now a few brief notes:

Assuming POST keys exist

Review the section in my other post.

Exception handling

Catching an exception, outputting a message and letting the script continue does not make sense. If an exception happens at a high level like that, it means that the page should probably be halted and the user informed that something went horribly wrong. If you continue to use $db after that exception, you'll get a fatal error because $db will not be an object.

filter_input

Your use of filter_input is wrong (as is your assumption that it somehow alters $_POST).

isset logic

Your isset logic is flawed. An empty post request will manage to pass through with an empty $errors array. $username, $password and $checkpassword would all be null.

username existence handling

Why not treat this as any other validation?

Use a structure like:

if (valid username provided) {
    //check if the username exists
    if (user name exists) {
        $errors['username'][] = "Your username is taken";
    }
}

The valid username provided could just be !isset($errors['username']) if you were positive that all username errors were put into the username key (this is currently the case, so the snippet in this sentence would work).

share|improve this answer
    
Thanks again Corbin! I edited the code to include some of your comments.. 1. Assuming POST keys exist-> So Array_key_exists checks whether a variable is null, and isset checks if isset is empty? Do I need to use both array_key_exists and isset to access a possibly null variable? Also like you said that $_POST variables are never null (from your previous answer, unless I'm misinterpreting) is it okay to access "empty" $_POST variables? For example In the updated code I used mysql_escape_string on a possibly empty $_POST without checking isset. –  J Y Jun 28 '12 at 2:47
    
filter_input -> You're right. I had no idea how to use this. I'll stick to mysql_escape_string (is this enough to prevent sql injection?) for sql, htmlspecialchars for html, (and possibly some other things for javascript). –  J Y Jun 28 '12 at 2:48
    
isset logic -> I swear I had an else clause...Possibly deleted it. Either way I've edited it in the code. Awesome pick up! –  J Y Jun 28 '12 at 2:49
    
Exception handling -> If you're refering to the catch and try statement on db connect. Yeah you're right, there no need to try and catch there. I've just set it to or die(), although that might be redundant because I have require(). For the second try and catch I assume that's fine as is –  J Y Jun 28 '12 at 2:51
    
username existence handling -> Yeah definitely. The more elegant error handling as a whole doesn't come naturally to me yet, need to work on it actively. Also I've just posted a number of comments, please let me know if this is inappropriate/another method/ or something along the lines of "google XXXXX" if its quite trivial. –  J Y Jun 28 '12 at 2:53
up vote 3 down vote

In addition to @Corbin's reply:

1) $salt1 and $salt2 variables are undefined in myEncrypt() function.

2) Double hashing in myEncrypt() probably makes sense if you store salts in different storage (at least not in one table), or if hashing algorithm is unknown (though it is a bit obvious in this case).

3) My general concern is that code logic goes together with view (HTML code). I'd refactor the code according the MVC pattern (as a most common one) or some other (MVP, for example).

share|improve this answer
    
1) Yeah I did have those defined, not in the function itself, I didn't want to paste them in but essentially they were a static string like 134FdgE3. 2. I stored the salts in the .php files where should they be stored exactly? 3. I'm thinking of learning cakePHP, my concern is it's a steep learning curve and also I might learn more from building from the ground up, do you think I'm ready to learn a framework, or should I just refactor into an MVC pattern without a framework? –  J Y Jul 1 '12 at 18:44

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.