2
\$\begingroup\$

I'm learning more about PHP, so I've decided to create a simple login/create account system. Entering information for creating a new account sends the data to my localhost machine, using MySQLi and the web server MySQL. I'm looking for feedback about security, efficiency, and overall code. I would like to kick old habits to the curb before it's too late. Any and all help is appreciated and considered. Thank you in advance!

create.html

<!DOCTYPE html>
<html lang="en-US">
    <head>
        <meta charset="UTF-8">
        <title>create.html</title>
        <script src="script.js"></script>
        <link rel="stylesheet" type="text/css" href="style.css">
    </head>
    <body bgcolor="pink">
        <center>
            <form action="create.php" method="post">
                <label>Username</label>
                <input type="text" name="username"><br>
                <label>Password</label>
                <input type="password" name="password"><br>
                <label>Re-enter Password</label>
                <input type="password" name="confirm_password"><br>
                <button type="submit">Create Account</button>
            </form>
        </center>
    </body>
</html>

create.php

<!DOCTYPE html>
<html lang="en-US">
    <head>
        <meta charset="UTF-8">
        <title>create.php</title>
    </head>
    <body bgcolor="pink">
        <?php

            $servername = "localhost";
            $username = "XXXXXXXXXX"; // Not shown
            $password = "XXXXXXXXXX"; // Not shown
            $dbname = "Database";

            //Create connection
            $mysqli = new mysqli($servername, $username, $password, $dbname);

            //Test connection
            if ($mysqli->connect_error) {
                die("Connection failed: " . $mysqli->connect_error);
            }

            $new_user_usr = filter_input(INPUT_POST, 'username');
            $new_user_pwd = filter_input(INPUT_POST, 'password');
            $new_user_pwd_conf = filter_input(INPUT_POST, 'confirm_password');

            $sql = "SELECT usr, pwd FROM Users";
            $result = $mysqli->query($sql);

            if($result->num_rows > 0) {

                /* If passwords don't match */
                if($new_user_pwd !== $new_user_pwd_conf) {
                    die("Passwords don't match");
                }

                /* If password isn't between bounds */
                if(strlen($new_user_pwd) <= 7 || strlen($new_user_pwd) >= 13) {
                    die("Password not long enough! Must be at least 8 characters long, but not greater than 12 characters");
                }

                /* If username is the same as password*/
                if($new_user_usr === $new_user_pwd) {
                    die("Username cannot equal password!");
                }

                while($row = $result->fetch_assoc()) {
                    if($row['usr'] === $new_user_usr) {
                        die("Username already taken");
                    }
                }

                $add = "INSERT INTO Users (usr, pwd) VALUES ('$new_user_usr', '$new_user_pwd')";

                echo $mysqli->query($add) ? "user created successfully" : "Error: " . $add . "<br>" . $mysqli->error;

            }

        ?>
    </body>
</html>
\$\endgroup\$
2
\$\begingroup\$

filter_input(INPUT_POST) does nothing

Although filter_input() can be very useful for validating and sanitizing data, when you use filter_var(INPUT_POST) you are actually not sanitizing anything. It just passes the raw data through and any invalid content you were hoping to remove, like HTML, will still be there. I believe what you are looking for is to use the FILTER_SANITIZE_STRING flag which will strip HTML from those values:

$new_user_usr = filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING);
$new_user_pwd = filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING);
$new_user_pwd_conf = filter_input(INPUT_POST, 'confirm_password', FILTER_SANITIZE_STRING);

Do your data validation and error handling sooner

In this code you go to the database to get users and then you do data validation that could cause your script to exit before you use that data. That's a waste of time and processing power. Don't do anything until you have to. So get your after you validate the other data.

Don't die()

It's good that you check for errors, but you handle them poorly. Terminating the executation of your code and dumping an error message out to the user is a bad user experience. You should capture those errors, probably in an array, and then display them in a user-friendly way so they have the opportunity to correct their mistakes and try again.

Your user check is extremely inefficient

You check to see if a user exists by iterating through every row in your table! That's just very slow and won't scale at all. You can easily narrow this down to a simple query that checks to see if anyone is using the same username. If you get any results back, you know someone is using that username.

// This code is insecure and I will fix that next
$sql = "SELECT usr FROM Users WHERE usr = '{$new_user_usr}'");
$result = $mysqli->query($sql);
if ($result->num_rows) {
    die("Username already taken");
}

You are wide open to SQL injections

our code is wide open to [SQL Injection][1] which is the [most dangerous web vulnerability][2]. This occurs when you execute SQL queries with unsanitized user data. By placing the raw $_POST variable directly in your query you are allowing an attacker to inject their own SQL into your query and execute it. They can do anything from stealing data to deleting your database.

To combat this you must use parameterized queries. Stack Overflow covers this very well but here's what your code would like if you use mysqli with prepared statements:

$stmt = $mysqli->prepare('SELECT usr FROM Users WHERE usr = ?');
$stmt->bind_param('s', $new_user_usr); // 's' specifies the variable type => 'string'
$stmt->execute();
$result = $stmt->get_result();
if ($result->num_rows) {
    die("Username already taken");
}

And:

$stmt = $mysqli->prepare('INSERT INTO Users (usr, pwd) VALUES (?, ?)');
$stmt->bind_param('ss', $new_user_usr, $new_user_pwd); 
$stmt->execute();
$result = $stmt->get_result();
if (!$result->affected_rows) {
    die("Username already taken");
}

Never store plain text passwords

Never, ever, store plain text passwords. It is a huge security nightmare and there is no excuse to do it. Please use PHP's built-in functions to handle password security.

// Do this *before* you insert into the database
$new_user_pwd = password_hash($new_user_pwd, PASSWORD_BCRYPT, $options);

// Do this when you want to verify a password
if (password_verify($new_user_pwd, $row['pwd'])) {
    echo 'Password is valid!';
} else {
    echo 'Invalid password.';
}

Make sure you don't escape passwords or use any other cleansing mechanism on them before hashing. Doing so changes the password and causes unnecessary additional coding.

Put your database credentials some more secure

Your database credentials are considered sensitive information and should be treated as such. Putting it in your PHP code potentially leaves it open to discovery. If your web server has an issue and instead of rendering your PHP code it displays it as plain text, your credentials will be publicly available for all to see. You should consider moving it to its own configuration file which is placed outside of your web root so it cannot be accessed through a browser at any time.

Separate concerns

Try to separate your HTML and business logic. It's okay to have PHP in your HTML templates for dynamic content, but business logic is best separated from your HTML as coupling them together makes your code less flexible and potentially more difficult to maintain.

|improve this answer|||||
\$\endgroup\$
2
\$\begingroup\$

I would like to kick old habits to the curb

That's a very good intention. Especially given that the present code, sadly, is rather a display of such old habits.

In order to improve, consider learning the following essentials:

  • PDO prepared statements (be advised that PDO is superior to mysqli for learners). Basically all your queries involving variables must be run not via query() but via prepare()/execute(), with all variables in the query substituted with placeholders.
  • Password hashing is essential. Just follow the simple example and store in the database not the original password but the result of password_hash() function's call.
    • there is no reason to limit the maximum password length. Let a user have the password as big as they wish.
  • the proper error reporting. first of all, always configure PHP to report errors by itself. And then never write a single line of code that intentionally outputs the error message. It's your server's configuration should be responsible for the errors' output.
  • Use a better code structure. Consider having both the HTML form and the handler sharing the same address, making the file with the form not called directly but included by the PHP file on demand. It will let you to utilize the POST/Redirect/Get pattern. Among other things it will let you to show errors nicely instead of bluntly aborting the script execution to show one. Collect all errors in the array and then run your query only if it is empty. Show the form back along the errors and the entered data otherwise.
    • the code used for the database connection is better to be stored in a separate file and then just included into every script that will need it.
  • avoid the code that does nothing useful. For example, running a select query to get all users from the database in order to create a user just makes no sense. You can ditch both the query and the condition that follows.

Taking all the above into consideration your code could be rewritten like this

db.php:

$host = '127.0.0.1';
$db   = 'Database';
$user = 'XXX';
$pass = 'XXX';
$charset = 'utf8mb4';

$options = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::ATTR_EMULATE_PREPARES   => false,
];
$dsn = "mysql:host=$host;dbname=$db;charset=$charset";
try {
     $pdo = new PDO($dsn, $user, $pass, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

create_user.php

$errors = [];
if ($_POST)
{
    require 'db.php';

    $username = filter_input(INPUT_POST, 'username');
    $password = filter_input(INPUT_POST, 'password');
    $pwd_confirm = filter_input(INPUT_POST, 'confirm_password');

    if ($password !== $pwd_confirm) {
        $errors[] = "Passwords don't match";
    }
    if (strlen($password) < 8) {
        $errors[] = "Password not long enough! Must be at least 8 characters long";
    }
    if ($username === $password) {
        $errors[] = "Username cannot equal password!";
    }

    $stmt = $pdo->query("SELECT 1 FROM Users WHERE usr = ?");
    $stmt->execute([$username]);
    $user_found = $stmt->fetchColumn();
    if ($user_found) {
        $errors[] = "Username already taken";
    }
    if (!$errors)
    {
        $hashed_password = password_hash($password, PASSWORD_DEFAULT);
        $stmt = $pdo->prepare("INSERT INTO Users (usr, pwd) VALUES (?, ?)");
        $stmt->execute([$username, $hashed_password]);
        header("Location: ."); // consider redirecting to the user profile
        exit;
    }
} else {
    $username = "";
}

include 'form.php';

form.php

<!DOCTYPE html>
<html lang="en-US">
<head>
    <meta charset="UTF-8">
    <title>create.html</title>
    <script src="script.js"></script>
    <link rel="stylesheet" type="text/css" href="style.css">
</head>
<body bgcolor="pink">
<center>
    <?php foreach ($errors as $error): ?>
        <p><?= $error ?></p>
    <?php endforeach ?>
    <form method="post">
        <label>Username</label>
        <input type="text" name="username" value="<?= htmlspecialchars($username) ?>"><br>
        <label>Password</label>
        <input type="password" name="password"><br>
        <label>Re-enter Password</label>
        <input type="password" name="confirm_password"><br>
        <button type="submit">Create Account</button>
    </form>
</center>
</body>
</html>
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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