2
\$\begingroup\$

This is continuation and implementation of feedback found in this post: Basic user registration code

I 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:

try {
    $db = new PDO("mysql:host=$db_hostname;dbname=$db_database", $db_username, $db_password);
}
catch(PDOException $e)
{
echo "Failed to connect to server";
}

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

Registration.php:

require 'initialise.php';

//View - Leaving this as is for now. Haven't learnt css/html in depth yet.
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") {

    filter_input(INPUT_POST, $_POST, FILTER_SANITIZE_STRING, FILTER_NULL_ON_FAILURE);
    $usernameclean = $_POST['regusername'];
    $password = $_POST['regpassword'];
    $checkpassword = $_POST['checkregpassword'];

    if (isset($username, $password, $checkpassword)) {
        if (strlen($username) < 5) {
            $errors['username'][] = "Usernames must be at least 5 characters in length.";
        }
        if (strlen($username) > 32) {
            $errors['username'][] = "Usernames have a maximum limit of 32 characters.";
        }
        if (strlen($password) < 5) {
            $errors['password'][] = "Passwords must be at least 5 characters in length.";
        }
        if ($password <> $checkpassword) {
            $errors['password'][] = "Your passwords must be the same.";
        }
    }


    if (!count($errors)) {

        $hashedpassword = myEncrypt($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();
                    echo "Your account could not be created, please try again later.";
                    }
            } else 
            {
            echo "Username Unavailable, please try another username";
            }
    }
    if (count($errors)) {
        echo '<ul>';
        foreach ($errors as $elements => $errs) {
            foreach ($errs as $err) {
                echo '<li>' . htmlspecialchars($err) . '</li>';
            }
        }
        echo '<ul>';
    }
}
?>
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

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

\$\endgroup\$
7
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jun 28, 2012 at 2:47
  • \$\begingroup\$ 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). \$\endgroup\$ Commented Jun 28, 2012 at 2:48
  • \$\begingroup\$ isset logic -> I swear I had an else clause...Possibly deleted it. Either way I've edited it in the code. Awesome pick up! \$\endgroup\$ Commented Jun 28, 2012 at 2:49
  • \$\begingroup\$ 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 \$\endgroup\$ Commented Jun 28, 2012 at 2:51
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jun 28, 2012 at 2:53
3
\$\begingroup\$

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

\$\endgroup\$
1
  • \$\begingroup\$ 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? \$\endgroup\$ Commented Jul 1, 2012 at 18:44

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.