0

I need help with my PHP registration form. I can't figure out what's wrong. Maybe you can see it. Data sent from my registration form are written to database even if submitted username or password is too short or long and when passwords don't match. What to change in my code? I've been trying to correct it for hours.
Thanks in advance.

Registration form in index.php including <?php session_start(); ?> above HTML

<form class="sign-up" action="users.php" method="post">
    <p class="sign-up-title">Username:</p> <input class="sign-up-input" type="text" name="username" min="5" max="25">
    <p class="sign-up-title">Password:</p> <input class="sign-up-input" type="password" name="pass" min="6" max="35">
    <p class="sign-up-title">Confirm password:</p> <input class="sign-up-input" type="password" name="pass_check" min="6" max="35">
    <p class="sign-up-title">E-mail:</p> <input class="sign-up-input" type="email" name="email">
    <input id="sign-up-input-submit" class="sign-up-input" type="submit" value="Sign Up">
</form>
<?php
if (isset($_SESSION["username_error_short"])) {
    echo $_SESSION["username_error_short"];
}
elseif (isset($_SESSION["username_error_long"])) {
    echo $_SESSION["username_error_long"];
}
elseif (isset($_SESSION["username_error_exists"])) {
    echo $_SESSION["username_error_exists"];
}
elseif (isset($_SESSION["pass_error_short"])) {
    echo $_SESSION["pass_error_short"];
}
elseif (isset($_SESSION["pass_error_long"])) {
    echo $_SESSION["pass_error_long"];
}
elseif (isset($_SESSION["pass_error_mismatch"])) {
    echo $_SESSION["pass_error_mismatch"];
}
elseif (isset($_SESSION["email_error_exists"])) {
    echo $_SESSION["email_error_exists"];
}
elseif (isset($_SESSION["registration_success"])) {
    echo $_SESSION["registration_success"];
}
elseif (isset($_SESSION["registration_fail"])) {
    echo $_SESSION["registration_fail"];
}
?>


Script in users.php

<?php

session_start();
include "connect.php";
global $db;

if (isset($_POST["username"]) || isset($_POST["pass"]) || isset($_POST["pass_check"]) 
    || isset($_POST["email"])) {

    $username = $_POST["username"];
    $password = $_POST["pass"];
    $password_check = $_POST["pass_check"];
    $email = $_POST["email"];

    // check if username is too short/long

    if (strlen($username) < 5) {

        $_SESSION["username_error_short"] = "Username too short. Username should contain at least 5 characters.";

    }
    elseif (strlen($username) > 25) {

        $_SESSION["username_error_long"] = "Username too long. Username should contain max. 25 characters.";

    }

    // check if username already exists in DB

    elseif (strlen($password) >= 5 || strlen($password) <= 25) {

        $sql_User_Duplicate = $db->prepare('SELECT * FROM users WHERE username = :username');
        $sql_User_Duplicate->bindParam(':username', $username);
        $sql_User_Duplicate->execute();

        if ($sql_User_Duplicate->rowCount() > 0) {

            $_SESSION["username_error_exists"] = "This username already exists. Select another one.";

        }
        else {

            $usernameCheck = 1;

        }

    }

    // check if password is too short/long

    if (strlen($password) < 6) {

        $_SESSION["pass_error_short"] = "Password too short. Password should contain at least 6 characters.";
        $passwordCheck_length = 0;

    }
    elseif (strlen($password) > 35) {

        $_SESSION["pass_error_long"] = "Password too long. Password should contain max. 35 characters.";
        $passwordCheck_length = 0;

    }

    // check if $password matches $password_check

    elseif (strlen($password) >= 6 || strlen($password) <= 35) {

        if ($password == $password_check) {

        $passwordCheck = 1;

        }
        else {

        $_SESSION["pass_error_mismatch"] = "Passwords don't match. Try again.";

        }

    }

    // check if email already exists in DB

    $sql_Email_Duplicate = $db->prepare('SELECT * FROM users WHERE email = :email');
    $sql_Email_Duplicate->bindParam(':email', $email);
    $sql_Email_Duplicate->execute();

    if ($sql_Email_Duplicate->rowCount() > 0) {

        $_SESSION["email_error_exists"] = "This e-mail is already registered.";

    }
    else {

        $emailCheck = 1;

    }

    // create new account

    if ($usernameCheck == 1 || $passwordCheck == 1 || $emailCheck == 1) {

        $sql_Account_Create = $db->prepare('INSERT INTO users (username, password, email) VALUES (:username, :password, :email)');
        $sql_Account_Create->execute(array(":username" => $username, ":password" => $password, ":email" => $email));

        // check if account (username) has been created in DB

        $sql_Account_Create_Check = $db->prepare('SELECT * FROM users WHERE username = :username');
        $sql_Account_Create_Check->bindParam(':username', $username);
        $sql_Account_Create_Check->execute();

        if ($sql_Account_Create_Check->rowCount() > 0) {

            $_SESSION["registration_success"] = "Account registered successfully.";

        }
        else {

            $_SESSION["registration_fail"] = "Something went wrong. Please check submitted data and try again later.";

        }

    }

}

header('Location: index.php');

?>
encrypted21
  • 194
  • 11
  • *In order to prevent SQL injection and server errors I need to use the PDO statements only. Without using mysqli functions etc.* - mysqli is just as safe as PDO. It's the mysql_* functions (without the 'i') that have been deprecated. – Mike May 02 '16 at 17:09
  • 1
    Allow users to use the [passwords / phrases](https://xkcd.com/936/) they desire. [Don't limit passwords.](http://jayblanchard.net/security_fail_passwords.html) – Jay Blanchard May 02 '16 at 17:11
  • So, it's really safe to use mysqli? I thought only PDO statements are safe. Thanks for correcting me. – encrypted21 May 02 '16 at 17:12
  • 1
    **Never store plain text passwords!** Please use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). Make sure that you [don't escape passwords](http://stackoverflow.com/q/36628418/1011527) or use any other cleansing mechanism on them before hashing. Doing so *changes* the password and causes unnecessary additional coding. – Jay Blanchard May 02 '16 at 17:12
  • You should also [enable error reporting](http://stackoverflow.com/questions/1053424/how-do-i-get-php-errors-to-display). Your script should produce an undefined variable warnings under certain conditions. – Mike May 02 '16 at 17:13
  • 1
    Both PDO and MySQLi are safe if used correctly with prepared statements. – Jay Blanchard May 02 '16 at 17:13
  • 1
    you could start by using an `&&` for `elseif (strlen($password) >= 5 || strlen($password) <= 25)` and the same for `if ($usernameCheck == 1 || $passwordCheck == 1 || $emailCheck == 1)` you're only checking for `OR` rather than `AND|&&`. So if the latter only fulfills "one" of those conditions rather than "all", then your INSERT happens. I feel you have just way too many conditions in there. – Funk Forty Niner May 02 '16 at 17:13
  • 1
    @encrypted21 PDO and MySQLi can both be completely unsafe if used incorrectly. Just simply using these functions is not enough to prevent your site from being hacked. As JayBlanchard mentioned above, use prepared statements with bound parameters. – Mike May 02 '16 at 17:19
  • @Fred -ii- But what if one of these variables aren't set because username or email already exist in the database or length of username or password is incorrect? PHP will perform SQL query even if these mistakes occur if OR is used. Same as it's happening now. Or am I wrong? I'm confused :(@ – encrypted21 May 02 '16 at 17:23
  • 2
    and now we are all headed towards the espresso and popcorn machine @JayBlanchard - it's about that time of day too. – Funk Forty Niner May 02 '16 at 17:24
  • @JayBlanchard I understand. I wanted to add hashing after finding out what to do with the current problem. – encrypted21 May 02 '16 at 17:27
  • I have a feeling about having too many conditions, too. But that's what I could create. I'm a beginner with PHP. – encrypted21 May 02 '16 at 17:30
  • 1
    Try cutting those conditionals down to 2-3 to start, then add from there until something breaks. You're setting all those errors using sessions so we don't know if your code is wanting to continue to execute even if one/some of them don't fill the bill. Add `exit;` after each of those until and again something either gives or breaks. That's the best advice I can offer here. P.s.: Start off small and build bigger until something falls. I feel you're biting off a bit too much than you can chew "starting off" as a new coder. – Funk Forty Niner May 02 '16 at 17:30
  • Addendum to above: Believe it or not, even I still work that way to this day ;-) it helps in debugging too without tripping over something I don't know made it break in the first place. – Funk Forty Niner May 02 '16 at 17:33
  • @Fred-ii- Using `exit;` after the first error would prevent your form from detecting multiple errors though. – Mike May 02 '16 at 17:35
  • @Mike *"until something breaks"* --- *"and again something either gives or breaks"* ;-) – Funk Forty Niner May 02 '16 at 17:36
  • 1
    @encrypted21 On the line `elseif (strlen($password) >= 5 || strlen($password) <= 25) {` (besides the fact you are incorrectly checking the password length instead of the username), can be simplified to just `else {` since that is the only other possibility. Same as below when you check the password length. Including extra checks is redundant, makes your code nanoseconds slower and most importantly, makes it less readable. – Mike May 02 '16 at 17:37
  • OK, thanks :). I'll try to write my code again and test it step by step. Right now I have no further idea what else to do with it. I had 'else' instead of 'elseif' before. I was experimenting with other possibilities. Good one. Have to change it back. Thank you all. – encrypted21 May 02 '16 at 17:42
  • 1
    @encrypted21 Also instead of checking all these specific errors in `$_SESSION`, why not just add all errors to `$_SESSION['errors']['fieldname']` (where 'fieldname' is the name of the field the error belongs to, e.g. 'username'? That way you can just do `if (isset($_SESSION['errors'])) { foreach ($_SESSION['errors'] as $err) { echo $err; } }`. – Mike May 02 '16 at 17:42
  • Oh, I never did that before. I'm not sure if I understand. Or maybe I'm starting to. That really looks like a better idea to use $_SESSION. Thanks! – encrypted21 May 02 '16 at 17:45
  • @Fred-ii- Maybe I'm just dumb, but I'm *still* not understanding what you mean. Anyway I added a community wiki answer below. Feel free to make any changes if you want. – Mike May 02 '16 at 18:22

1 Answers1

2

These are my recommendations:

  • Don't limit your users' passwords. I just checked to make sure it's not empty. It's faster to query SELECT COUNT(*) than SELECT * and count the results.
  • PDOStatement::execute returns true on success so you don't have to execute another query to see if it inserted correctly
  • The trailing ?> at the end of the file is unnecessary and actually discouraged in most cases.
  • Add all of your errors to a single array so that you don't have to test for each type of array in your index.php file. Doing it like that is not only prone to error, it's extremely tedious. You usually want to try to make things as automatic as possible, looping over things instead of trying to remember all of the test conditions you had previously used.
  • You want to store the hash of the user's password, not the password in plain text. Make sure your password column type is long enough to store bcrypt hashes (see: What column type/length should I use for storing a Bcrypt hashed password in a Database?).
  • Make sure you unset $_SESSION['errors'] or your users will continue to have the error message displayed to them even after they have successfully filled out the form.

users.php:

<?php

session_start();
include "connect.php";
global $db;

if (isset($_POST["username"]) || isset($_POST["pass"]) || isset($_POST["pass_check"]) 
    || isset($_POST["email"])) {

    $username = $_POST["username"];
    $password = $_POST["pass"];
    $password_check = $_POST["pass_check"];
    $email = $_POST["email"];

    // check if username is too short/long

    if (strlen($username) < 5) {

        $_SESSION["errors"]["username"] = "Username too short. Username should contain at least 5 characters.";

    }
    elseif (strlen($username) > 25) {

        $_SESSION["errors"]["username"] = "Username too long. Username should contain max. 25 characters.";

    }

    // check if username already exists in DB

    elseif (strlen($username) >= 5 || strlen($username) <= 25) {

        $sql_User_Duplicate = $db->prepare('SELECT COUNT(*) FROM users WHERE username = :username');
        $sql_User_Duplicate->bindParam(':username', $username);
        $sql_User_Duplicate->execute();

        if ($sql_User_Duplicate->fetchColumn() > 0) {

            $_SESSION["errors"]["username"] = "This username already exists. Select another one.";

        }

    }

    // check if password is too short/long

    if (empty($password)) {

        $_SESSION["errors"]["password"] = "Your password is empty";

    }
    // check if $password matches $password_check

    elseif ($password != $password_check) {

        $_SESSION["errors"]["password"] = "Passwords don't match. Try again.";

    }

    // check if email already exists in DB

    $sql_Email_Duplicate = $db->prepare('SELECT COUNT(*) FROM users WHERE email = :email');
    $sql_Email_Duplicate->bindParam(':email', $email);
    $sql_Email_Duplicate->execute();

    if ($sql_Email_Duplicate->fetchColumn() > 0) {

        $_SESSION["errors"]["email"] = "This e-mail is already registered.";

    }

    // create new account

    if (!isset($_SESSION['errors']) {
        $hash = password_hash($password, PASSWORD_BCRYPT, ['cost' => 12]);

        $sql_Account_Create = $db->prepare('INSERT INTO users (username, password, email) VALUES (:username, :password, :email)');
        $inserted = $sql_Account_Create->execute(array(":username" => $username, ":password" => $hash, ":email" => $email));

        if ($inserted !== true) {
            $_SESSION["errors"]["registration_fail"] = "Something went wrong. Please check submitted data and try again later.";
        }

    }

}

header('Location: index.php');

And index.php:

<form class="sign-up" action="users.php" method="post">
    <p class="sign-up-title">Username:</p> <input class="sign-up-input" type="text" name="username" min="5" max="25">
    <p class="sign-up-title">Password:</p> <input class="sign-up-input" type="password" name="pass" min="6" max="35">
    <p class="sign-up-title">Confirm password:</p> <input class="sign-up-input" type="password" name="pass_check" min="6" max="35">
    <p class="sign-up-title">E-mail:</p> <input class="sign-up-input" type="email" name="email">
    <input id="sign-up-input-submit" class="sign-up-input" type="submit" value="Sign Up">
</form>


<?php
if (isset($_SESSION["errors"])) {
    ?>
    There was a problem signing up:
    <ul>
    <?php
    foreach ($_SESSION['errors'] as $err) {
        echo "<li>$err</li>";
    }
    echo '</ul>';
    unset($_SESSION['errors']);
}
else {
    echo "Account registered successfully.";
}
Community
  • 1
  • 1
Mike
  • 23,542
  • 14
  • 76
  • 87
  • It works! Thank you so much ;). There are only 2 things to fix here: missing ')' and "Account registered successfully" message as it's always displayed. Fixed with $_SESSION created if data have been inserted. – encrypted21 May 02 '16 at 21:02