Rudimentary registration and login system

Question

I'd love for someone to review my code. It works fairly well, but I'm still getting the hang of PHP and want to make sure I've got an iron-clad system that isn't hilariously vulnerable. I'm also open to better ways to handle registering/logging in and out. I know the general answer is to not do this yourself, but I'm interested in learning to do this for myself rather than utilizing a framework. I definitely feel like I'm repeating code in several places, and would love to make this more concise.

index.php

<?php
error_reporting(E_ALL);
ini_set("display_errors", 1);
session_start();
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Project Neue World</title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php
if (isset($_SESSION["message"])) {
    echo "<p>" . $_SESSION["message"] . "</p>";
    unset($_SESSION["message"]);
}
if (isset($_SESSION["loggedin"]) && $_SESSION["loggedin"]) {
    echo "<p>Welcome back! Click <a href=\"profile.php\">here</a> to view your profile, " . $_SESSION["username"] . ".</p>";
} else {
    echo "<p>Welcome! Please <a href=\"register.php\">register</a> an account, or <a href=\"login.php\">login</a></p>";
}
?>
    <script src="main.js"></script>
</body>
</html>

register.php

<?php
error_reporting(E_ALL);
ini_set("display_errors", 1);
session_start();
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Project Neue World - Register</title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php
if (isset($_SESSION["loggedin"]) && $_SESSION["loggedin"]) {
    $_SESSION["message"] = "You're already logged into an account. Please <a href=\"?logout\">logout</a> to register a new account.";
    header("Location: profile.php");
    die();
}
if ($_SERVER["REQUEST_METHOD"] === "POST") {
    $errors = [];
    $username = "";
    $password = "";
    $email = "";

    if (isset($_POST["register"])) {
        if (isset($_POST["username"])) {
            $username = htmlspecialchars(trim($_POST["username"]));

            if (empty($username)) {
                $errors["username"] = "Please enter a username.";
            } elseif (strlen($username) > 32) {
                $errors["username"] = "Your username must be more than one character and less than 32 characters.";
            }
        }
        if (isset($_POST["password"])) {
            $password = trim($_POST["password"]);

            if (empty($password)) {
                $errors["password"] = "Please enter a password.";
            } elseif (strlen($password) < 8) {
                $errors["password"] = "Your password must be longer than eight characters.";
            }
        }
        if (isset($_POST["email"])) {
            $email = htmlspecialchars(trim($_POST["email"]));

            if (empty($email)) {
                $errors["email"] = "Please enter an e-mail address.";
            } elseif (strlen($email) > 64) {
                $errors["email"] = "Please enter an e-mail address shorter than 64 characters.";
            } elseif (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
                $errors["email"] = "Please enter a valid e-mail address.";
            }
        }
        try {
            $db = new PDO("sqlite:dev.sqlite3", null, null, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
            $stmt = $db->prepare("SELECT username FROM users WHERE username = :username");
            $stmt->execute([":username" => $username]);
            $username_exists = $stmt->fetchColumn();

            if ($username_exists) {
                $errors["username"] = "Username already exists. Please enter another username.";
            }
            $stmt = $db->prepare("SELECT email FROM users WHERE email = :email");
            $stmt->execute([":email" => $email]);
            $email_exists = $stmt->fetchColumn();

            if ($email_exists) {
                $errors["email"] = "This e-mail address has already been used. Please enter another e-mail address.";
            }
            if (!empty($errors)) {
                $_SESSION["errors"] = $errors;
                $_SESSION["temporary_username"] = htmlspecialchars($_POST["username"]);
                $_SESSION["temporary_email"] = htmlspecialchars($_POST["email"]);
                header("Location: register.php");
                die();
            }
            $stmt = $db->prepare("INSERT INTO users (username, password, email) VALUES (:username, :password, :email)");
            $password_hash = password_hash($password, PASSWORD_DEFAULT);
            $stmt->execute([
                ":username" => $username,
                ":password" => $password_hash,
                ":email" => $email
            ]);
            $_SESSION["loggedin"] = true;
            $_SESSION["username"] = $username;
            $_SESSION["message"] = "Registration complete!";
            header("Location: profile.php");
            die();
        }
        catch (PDOException $e) {
            echo "<p>" . $e->getMessage() . "</p>";
        }
    }
}
if (isset($_SESSION["errors"])) {
    foreach ($_SESSION["errors"] as $error) {
        echo "<p>" . $error . "</p>";
    }
    unset($_SESSION["errors"]);
}
?>
    <form action="register.php" method="post">
        <div><label for="username">Username</label></div>
        <div><input type="text" name="username" id="username" value="<?php echo isset($_SESSION["temporary_username"]) ? $_SESSION["temporary_username"] : "" ?>" required></div>
        <div><label for="password">Password</label></div>
        <div><input type="password" name="password" id="password" required></div>
        <div><label for="email">E-mail</label></div>
        <div><input type="email" name="email" id="email" value="<?php echo isset($_SESSION["temporary_email"]) ? $_SESSION["temporary_email"] : "" ?>" required></div>
        <div><input type="submit" name="register" value="Register Account"></div>
    </form>
<?php
unset($_SESSION["temporary_username"]);
unset($_SESSION["temporary_email"]);
?>
    <script src="main.js"></script>
</body>
</html>

login.php

<?php
error_reporting(E_ALL);
ini_set("display_errors", 1);
session_start();
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Project Neue World - Login</title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php
if (isset($_SESSION["message"])) {
    echo "<p>" . $_SESSION["message"] . "</p>";
    unset($_SESSION["message"]);
}
if (isset($_SESSION["loggedin"]) && $_SESSION["loggedin"]) {
    $_SESSION["message"] = "You're already logged in!";
    header("Location: profile.php");
    die();
}
if ($_SERVER["REQUEST_METHOD"] === "POST") {
    $errors = [];
    $username = "";
    $password = "";

    if (isset($_POST["login"])) {
        if (isset($_POST["username"])) {
            $username = $_POST["username"];

            if (empty($username)) {
                $errors["username"] = "Please enter a username.";
            }
        }
        if (isset($_POST["password"])) {
            $password = $_POST["password"];

            if (empty($password)) {
                $errors["password"] = "Please enter a password.";
            }
        }
        try {
            $db = new PDO("sqlite:dev.sqlite3", null, null, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
            $stmt = $db->prepare("SELECT * FROM users WHERE username = :username");
            $stmt->execute([":username" => $username]);
            $result = $stmt->fetch(PDO::FETCH_ASSOC);

            if ($result["username"] !== $username) {
                $errors["login"] = "Incorrect login credentials.";
            }
            if (!password_verify($password, $result["password"])) {
                $errors["login"] = "Incorrect login credentials.";
            }
            if (!empty($errors)) {
                if (!empty($errors)) {
                    $_SESSION["errors"] = $errors;
                    $_SESSION["temporary_username"] = htmlspecialchars($_POST["username"]);
                    header("Location: login.php");
                    die();
                }
            }
            $_SESSION["loggedin"] = true;
            $_SESSION["username"] = $username;
            $_SESSION["message"] = "Successfully logged in!";
            header("Location: profile.php");
            die();
        }
        catch (PDOException $e) {
            echo "<p>" . $e->getMessage() . "</p>";
        }
    }
}
if (isset($_SESSION["errors"])) {
    foreach ($_SESSION["errors"] as $error) {
        echo "<p>" . $error . "</p>";
    }
    unset($_SESSION["errors"]);
}
?>
    <form action="login.php" method="post" name="login-form">
        <div><label for="username">Username</label></div>
        <div><input type="text" name="username" id="username" value="<?php echo isset($_SESSION["temporary_username"]) ? $_SESSION["temporary_username"] : "" ?>" required></div>
        <div><label for="password">Password</label></div>
        <div><input type="password" name="password" id="password" required></div>
        <div><input type="submit" name="login" value="Login"></div>
    </form>
<?php
unset($_SESSION["temporary_username"]);
?>
    <script src="main.js"></script>
</body>
</html>

profile.php

<?php
error_reporting(E_ALL);
ini_set("display_errors", 1);
session_start();

if ($_SERVER["REQUEST_METHOD"] === "GET") {
    if (isset($_GET["logout"])) {
        session_unset();
        session_destroy();
        header("Location: index.php");
        die();
    }
}
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Project Neue World - Profile</title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php
    if (isset($_SESSION["message"])) {
        echo "<p>" . $_SESSION["message"] . "</p>";
        unset($_SESSION["message"]);
    }
    if (!isset($_SESSION["loggedin"]) || !$_SESSION["loggedin"]) {
        $_SESSION["message"] = "Please <a href=\"register.php\">register</a> an account to view your profile.";
        header("Location: index.php");
        die();
    }
?>
    <p>Welcome to your profile, <?php echo $_SESSION["username"]; ?>.</p>
<?php
try {
    $db = new PDO("sqlite:dev.sqlite3", null, null, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
    $stmt = $db->prepare("SELECT * FROM users WHERE username = :username");
    $stmt->execute([":username" => $_SESSION["username"]]);
    $result = $stmt->fetch(PDO::FETCH_ASSOC);

    echo "<p>User ID: " . $result["id"] . "</p>";
    echo "<p>E-mail Address: " . $result["email"] . "</p>";
}
catch (PDOException $e) {
    echo "<p>" . $e->getMessage() . "</p>";
}
?>
    <p><a href="?logout">Logout</a>.</p>
    <script src="main.js"></script>
</body>
</html>

You should remove the error_reporting(E_ALL); ini_set("display_errors", 1); in production. Any errors or warnings can give away information to a hacker. I would create a seperate 'start.php' file to include in each php file containing this, that way you only have to change it in one location. — KIKO Software, Commented Mar 31, 2019 at 7:55
If you remove an user from the database they will stay logged in as long as 'loggedin' stays 'true' in their session. This could be for days, or longer. It is better to check $_SESSION["username"] against the database each time. — KIKO Software, Commented Mar 31, 2019 at 8:01
@KIKOSoftware partially true for display_errors but absolutely wrong for error_reporting. You need your error reporting in production more than in dev. And surely you meant "change display_errors to 0", not remove. — Your Common Sense, Commented Mar 31, 2019 at 18:22
@YourCommonSense I would suggest to use your common sense in this case. :-) You're right that only removing 'display_errors' is enough (set to zero). Perhaps you should say why 'error_reporting' should stay? — KIKO Software, Commented Mar 31, 2019 at 18:24
Why would you trim the password? What if the user wishes to use a space? — Script47, Commented Apr 1, 2019 at 8:51

mickmackusa · Accepted Answer · 2019-03-31 20:55:06Z

You should not be presenting any errors in production and don't offer the caught $e->getMessage(). These details will only help malicious actors (and not your users).

When you want to check if a variable exists AND it is not zero-ish/falsey/null/empty, just use !empty() instead of doing isset(...) && !empty(...).

If you have already checked that a variable isset() then you want to check if it is zero-ish/falsey/null/empty, just use !$variable_name -- this has the same effect and avoids the unnecessary function call.

I do not support the initialization of $errors = [];, $username = "";, $password = "";, and $email = "";. Just don't declare them. Empty username, password, or email should prevent any processes that rely on them. !empty($errors) both checks if the variable was declared AND has a positive count.

You should not be bothering to make a trip to the database if the submission lacks a username, password, or email.

You are rushing right into processing $result["username"] from $result = $stmt->fetch(PDO::FETCH_ASSOC);, but you should check that that element was actually generated.

This simply looks careless, remove the redundant check:

if (!empty($errors)) {
    if (!empty($errors)) {
           ...
    }
}

You shouldn't be throwing htmlspecialchars() around so much. You should be calling that ONLY just before printing to screen ($_SESSION["temporary_username"]). Definitely don't foul with users' email account before processing the value. If you want to sanitize certain input values so that specific characters are filtered out, fine, just be clear about this value mutating when the user is filling the form so that no one has any surprises.

Minimize total trips to the database. Combine the logic in SELECT username FROM users WHERE username = :username with SELECT email FROM users WHERE email = :email to form SELECT email FROM users WHERE username = :username OR email = :email, then process the single result set to check for either pre-existing value.

In login.php, you know you are only processing the username and password values so update your query's SELECT clause to nominate those two columns only. Only ever ask the database for the values that you intend to use.

!isset($_SESSION["loggedin"]) || !$_SESSION["loggedin"] can be condensed to empty($_SESSION["loggedin"]).

I don't recommend using SESSION storage to hold any "identifiable" values. Just keep the id in there and use that for all subsequent interactions. I have mentioned this before at StackOverflow.

Surely you meant switch off display errors, not error reporting — Your Common Sense, Commented Mar 31, 2019 at 18:23
Yes. I've edit my poor wording. Welcome back to SO. Let's hope you stick around for a while. — mickmackusa, Commented Mar 31, 2019 at 20:56
Thank you for the in-depth answer! Lots of useful information here. In regards to $e->getMessage(), how should I go about handling the catch part of my try-catch block instead? Regarding variable initialization, are you saying I can just use those variables by declaring them when I need them, or I shouldn't be using variables at all in this case? Re: $result["some_var"]. Are you saying I need to make sure result contains... results before trying to use them? On the last bit about SESSIONs, would I just want to do something akin to $_SESSION["user_id"] = $id? — Robert Holman, Commented Apr 1, 2019 at 13:04
Regarding catching pdo errors, I'll recommend reading the link in YCS's answer. You can present a vague error message to the user which states that something went wrong and that the developer has been made aware. Just don't ever give any specific details that a baddie would find valuable. Rather than instantiating $username then conditionally overwriting it, I recommend refactoring the flow so that disqualifying inputs from the user do not declare a $username and instead just exit that section of code. — mickmackusa, Commented Apr 1, 2019 at 20:10
After executing your SELECT query, you should perform an if or while check to see if the result is null (lots of places to research this on Stackoverflow.). Finally, yes, I recommend using an arbitrary / autoincremented value as the user's id for SESSION storage. — mickmackusa, Commented Apr 1, 2019 at 20:13

Your Common Sense · Accepted Answer · 2019-03-31 19:10:39Z

1

I would say that this code is quite above the average at whole. Yet there are some issues as well.

Error reporting

To elaborate on what others said about error reporting. Actually I've got a comprehensive article on PHP error reporting basics, which I highly recommend to read, but here is a gist just to get it right:

Error reporting is essential. A good programmer always crave for the every error message they can get. And it should be at max all the time. The difference is only the destination: on a local dev server it is naturally should be your screen, but on a production server errors should be completely banned from screen, going to the log file instead. Hence the two different setups:

On a development server
- error_reporting should be set to E_ALL value;
- display_errors should be set to 1
On a production server
- error_reporting should be set to E_ALL value;
- display_errors should be set to 0
- log_errors should be set to 1

So this setup contradicts a bit with what you have been told before but it makes a perfect sense.

Database exposed

Another security issue is the Sqlite database location. Assuming it is on the same level with other files, it means it is accessible by site users, so anyone would be able to download your whole database. I don't think its a good idea to let is so. Consider moving the database file outside the site root. And also use the absolute path to the file. To help with this issue I also have an article on files and directories.

answered Mar 31, 2019 at 19:10

Your Common Sense

8,8231 gold badge21 silver badges51 bronze badges

\$\begingroup\$ As you have written extensively on PDO and you stress the importance of setting EMULATE_PREPARES to false, is there a reason why you haven't mentioned it? (P.S. Not bashing just genuinely curious why no-one has mentioned it, your resource on PDO has been invaluable). \$\endgroup\$
– Script47
Commented Apr 1, 2019 at 9:01
\$\begingroup\$ @Script47 it is definitely not me who stressed on the importance, I would rather say that security-wise emulated statements are no worse than real ones. It is for convenience in the first place I recommend to have the emulation off. Besides, I am no expert in sqlite, and just don't know how the emulation works for this driver. \$\endgroup\$
– Your Common Sense
Commented Apr 1, 2019 at 9:24
\$\begingroup\$ Fair enough, thanks for the clarity. \$\endgroup\$
– Script47
Commented Apr 1, 2019 at 9:25
\$\begingroup\$ Thank you for the answer! So would you recommend I move the database to a subfolder within the site root, or to a folder "above" the root? \$\endgroup\$
– Robert Holman
Commented Apr 1, 2019 at 13:05
1

\$\begingroup\$ @RobertHolman Definitely above the site root \$\endgroup\$
– Your Common Sense
Commented Apr 1, 2019 at 13:06

Add a comment |

Stack Exchange Network

Rudimentary registration and login system

2 Answers 2

Error reporting

Database exposed

Your Answer

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

Hot Network Questions

Rudimentary registration and login system

2 Answers 2

Error reporting

Database exposed

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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