6
\$\begingroup\$

What I try to achieve I need to create a secure forgot password reset system. I am especially worrying about the security of this code and how to further improve it. Here's my code:

d32b6f67a9d2ff687d9b7d.php: maintenance placed one level above the root folder

<?php
define('WEB_MASTER_EMAIL', 'mail@example.com');
define('CONTACT_EMAIL', "mail@example.com");
define('ERROR_LOG_EMAIL', 'mail@example.com');

date_default_timezone_set('UTC');
$maintenance = false;

$home = 'http://localhost/projectturmeric/';
$webpageTitle = "Project Turmeric";
$t_users = "users";
$t_activationKeys = "activation_keys";
$t_passwordReset = "password_reset_requests";
if ($maintenance) {
    header('location:' . $home . 'under-maintenance.php');
    exit();
}
?>

dbconnect placed one level above the root folder c6ef0226276fb386d4a7a7dcb4df38ee9df1105a2244b7bd.php:

<?php
define('DB_HOST', 'localhost');
define('DB_USERNAME', 'root');
define('DB_PASSWORD', '');
define('DB_NAME', "project_turmeric");
?>

system/utils.php utils.php: contains mostly used functions

<?php
function create_db_linkage_instance()
{
    try {
        $greenf = new PDO("mysql:host=" . DB_HOST . ";dbname=" . DB_NAME . ";charset=utf8mb4;", DB_USERNAME, DB_PASSWORD);
        return $greenf;
    } catch (Exception $e) {
        $date = '[' . date('d-M-Y h-i-s a') . ']';
        $msg = $e->getMessage();
        $err_msg = $date . ' | Exception Error | ' . $msg  . ' | \n';
        error_log($err_msg, 3, ERROR_LOG_BOOK);
        // error_log("Date/time: $date, Exception Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Exception Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");
    } catch (Error $e) {
        $date = '[' . date('d-M-Y h-i-s a') . ']';
        $msg = $e->getMessage();
        $err_msg = $date . ' | Error | ' . $msg  . ' | \n';
        error_log($err_msg, 1, ERROR_LOG_BOOK);
        // error_log("Date/time: $date, Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");
    }
    return null;
}



function findIP()
{
    if (!empty($_SERVER['HTTP_CLIENT_IP'])) {
        //check ip from share internet
        $ip = $_SERVER['HTTP_CLIENT_IP'];
    } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
        //to check ip is pass from proxy
        $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
    } else {
        $ip = $_SERVER['REMOTE_ADDR'];
    }
    return $ip;
}



function sendMail($to, $name, $subject, $message): bool
{
    $developmentMode = true;
    $mail = new PHPMailer\PHPMailer\PHPMailer($developmentMode);
    $mail->SMTPDebug = 0; // disables SMTP debug information (for testing)
    $mail->IsSMTP();

    if ($developmentMode) {
        $mail->SMTPOptions = [
            'ssl' => [
                'verify_peer' => false,
                'verify_peer_name' => false,
                'allow_self_signed' => true
            ]
        ];
    }

    $mail->Host = "smtp.gmail.com";
    $mail->SMTPAuth = true;
    $mail->Username = "mail@example.com";
    $mail->Password = "********";
    $mail->SMTPSecure = "tls";
    $mail->Port = 587;
    $mail->setFrom('no-reply@projectturmeric.com', 'Project Turmeric'); // sender email and name
    $mail->AddAddress($to, $name);
    $mail->IsHTML(true);
    $mail->Subject = $subject;
    $mail->Body = $message;

    $mail->CharSet = "utf-8";
    $mail->addReplyTo('mail@example.com', 'Information');
    $mail->wordwrap = 70;

    if ($mail->send()) {
        $mail->ClearAllRecipients();
        $mail->clearAttachments();
        return true;
    }
    $mail->ClearAllRecipients();
    $mail->clearAttachments();
    return false;
}
?>

users/forgot-password.php forgot-password.php: this script sends the account recovery email

<?php
if (session_status() === PHP_SESSION_NONE) {
    session_start();
}

// Maintenance
require '../../d32b6f67a9d2ff687d9b7d.php';
require '../system/utils.php';

if (isset($_SESSION['user_id'])) {
    // user is already verified and logged in
    // so redirect to index page
    header('location:' . $home);
    exit();
}

// error log
define('ERROR_LOG_BOOK', '../../error.log');

// functions
function endTheWebpage()
{
    echo '</div>';
    echo '</main>';
    echo '</div>';
    echo '<footer id="footer">';
    include '../includes/footer.php';
    echo '</footer>';

    echo '<!-- SCRIPTS -->';
    echo '<script type="text/javascript" src="..\js\jquery-3.4.1.min.js"></script>';
    echo '<script type="text/javascript" src="..\css\bootstrap-4.3.1\js\bootstrap.min.js"></script>';
    echo '</body>';

    echo '</html>';
    exit();
}
?>
<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="description" content="">
    <meta name="author" content="">
    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
    <link rel="stylesheet" href="..\css\bootstrap-4.3.1\css\bootstrap.min.css" type="text/css">
    <link rel="stylesheet" href="..\css\main.css" type="text/css">
    <title>Forgot Password | <?php echo htmlspecialchars($webpageTitle, ENT_QUOTES); ?></title>
</head>

<body>
    <header id="header">
        <?php
        $menu = 4; // activation.php
        include '../includes/header.php';
        ?>
    </header>
    <div class="container">
        <main>
            <div id="reset-password-area">
                <h1 class="text-center">Reset Password</h1>
                <?php
                if ($_SERVER['REQUEST_METHOD'] === 'POST') {
                    $email = isset($_POST['email']) ? filter_var($_POST['email'], FILTER_SANITIZE_EMAIL) : "";
                    $email = trim($email);
                    if (!empty($email)) {
                        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
                            $errors['email'] = 'Please enter a valid email.';
                        }
                    } else {
                        $errors['email'] = 'please enter your email.';
                    }



                    if (empty($errors)) {
                        $requestTime = date("Y-m-d H:i:s"); // current date / time

                        try {
                            require '../../c6ef0226276fb386d4a7a7dcb4df38ee9df1105a2244b7bd.php';
                            $greenf = create_db_linkage_instance();
                            // check if the user exists in the database
                            // if yes, pull the display name associated with that email
                            $qry = 'SELECT display_name FROM ' . $t_users . ' WHERE email=:email';
                            $stmt = $greenf->prepare($qry);
                            $stmt->bindParam(':email', $email, PDO::PARAM_STR);
                            $stmt->execute();
                            if ($stmt->rowcount() === 1) {
                                $data = $stmt->fetch(PDO::FETCH_OBJ);
                                $displayName = $data->display_name;

                                // delete the old token from the database if present
                                $qry = 'DELETE FROM ' . $t_passwordReset . ' WHERE email=:email';
                                $stmt = $greenf->prepare($qry);
                                $stmt->bindParam(':email', $email, PDO::PARAM_STR);
                                $stmt->execute();
                                $token = hash('sha256', mt_rand(1000000000, 9999999999) . date("Y-m-d H:i:s"));
                                $token_id = hash('sha256', mt_rand(1000000000, 9999999999) . date("Y-m-d H:i:s"));
                                $url = $home . "users/reset-password.php?id=" . $token_id . "&token=" . $token;
                                // hash the token to store it in the database
                                $hashedToken = password_hash($token, PASSWORD_DEFAULT);
                                $qry = 'INSERT INTO ' . $t_passwordReset . ' (email, token_id, token, request_time) VALUES (:email, :token_id, :token, :request_time)';
                                $stmt = $greenf->prepare($qry);
                                $stmt->bindParam(':email', $email, PDO::PARAM_STR);
                                $stmt->bindParam(':token_id', $token_id, PDO::PARAM_STR);
                                $stmt->bindParam(':token', $hashedToken, PDO::PARAM_STR);
                                $stmt->bindParam(':request_time', $requestTime, PDO::PARAM_STR);
                                $stmt->execute();
                                if ($stmt->rowcount() === 1) {
                                    // send the password reset email
                                    $to = $email;
                                    $subject = "Reset Password - " . $webpageTitle;
                                    $mailContent = file('../../reset-password-with-account-email-template.html');
                                    $expires = date("d-m-Y H:i:s", strtotime($requestTime . " +30 minute")); // current date / time + 30 minutes
                                    $temp = findIP();
                                    if (filter_var($temp, FILTER_VALIDATE_IP) === false) {
                                        // invalid ip
                                        // set it to 0.0.0.0
                                        $ip = "0.0.0.0";
                                    } else {
                                        $ip = $temp; // valid ip
                                    }


                                    $mailContent = str_replace("{{HomePage}}", $home, $mailContent);
                                    $mailContent = str_replace("{{Logo}}", $home . "images/logo.png", $mailContent);
                                    $mailContent = str_replace("{{WebpageTitle}}", $webpageTitle, $mailContent);
                                    $mailContent = str_replace("{{Name}}", $displayName, $mailContent);
                                    $mailContent = str_replace("{{PasswordResetLink}}", $url, $mailContent);
                                    $mailContent = str_replace("{{Expires}}", $expires, $mailContent);
                                    $mailContent = str_replace("{{ForgetPasswordResetLink}}", $home . "users/cancel-password-reset-request.php", $mailContent);
                                    $mailContent = str_replace("{{IP}}", $ip, $mailContent);
                                    $mailContent = str_replace("{{AccountRecoveryLink}}", $home . "users/forgot-password.php", $mailContent);
                                    $mailContent = str_replace("{{ContactUs}}", $home . "contact-us.php", $mailContent);
                                    $body = "";
                                    if (is_array($mailContent)) {
                                        foreach ($mailContent as $mc) {
                                            $body .= $mc;
                                        }
                                    } else {
                                        $body = $mailContent;
                                    }

                                    require_once '../libs/PHPMailer-master/src/Exception.php';
                                    require_once '../libs/PHPMailer-master/src/PHPMailer.php';
                                    require_once '../libs/PHPMailer-master/src/SMTP.php';

                                    if (sendMail($to, $displayName, $subject, $body)) {
                                        $stmt = $greenf = null;
                                        echo '<p class="text-center" style="color: green;">Account recovery email sent to ' . $to . '. Please open this email to reset your password.</p>';
                                        echo '<p class="text-center" class="m_info">If you don’t see this email in your inbox within 15 minutes, look for it in your junk mail folder. If you find it there, please mark it as “Not Junk”.</p>';
                                        echo '<p class="text-center" class="m_info">If you are still experiencing any problems, contact <a href="../contact-us.php" target="_blank">support</a>.</p>';
                                    } else {
                                        $stmt = $greenf = null;
                                        echo '<p class="text-center">The system is busy please try later.</p>';
                                    }
                                    endTheWebpage(); // close div, main, include footer, close body and html tags
                                } else {
                                    // cannot insert the password reset token into the database
                                    // display the error message and end the webpage
                                    $stmt = $greenf = null;
                                    echo '<p class="text-center">The system is busy please try later.</p>';
                                    endTheWebpage(); // close div, main, include footer, close body and html tags
                                } // end of inserting password_reset_token into the database if condition block
                            } else {
                                // the email entered to reset the password is not found  in the database.
                                // so send the account not found. Sign up instead email
                                $stmt = $greenf = null;
                                $to = $email;
                                $subject = "Reset Password - " . $webpageTitle;
                                $mailContent = file('../../reset-password-without-account-email-template.html');

                                $temp = findIP();
                                if (filter_var($temp, FILTER_VALIDATE_IP) === false) {
                                    // invalid ip
                                    // set it to 0.0.0.0
                                    $ip = "0.0.0.0";
                                } else {
                                    $ip = $temp; // valid ip
                                }


                                $mailContent = str_replace("{{HomePage}}", $home, $mailContent);
                                $mailContent = str_replace("{{Logo}}", $home . "images/logo.png", $mailContent);
                                $mailContent = str_replace("{{WebpageTitle}}", $webpageTitle, $mailContent);
                                $mailContent = str_replace("{{Email}}", $to, $mailContent);
                                $mailContent = str_replace("{{WebpageTitle}}", $webpageTitle, $mailContent);
                                $mailContent = str_replace("{{SignUpEmail}}", $home . "users/signup.php", $mailContent);
                                $mailContent = str_replace("{{IP}}", $ip, $mailContent);
                                $mailContent = str_replace("{{ContactUs}}", $home . "contact-us.php", $mailContent);

                                $body = "";
                                if (is_array($mailContent)) {
                                    foreach ($mailContent as $mc) {
                                        $body .= $mc;
                                    }
                                } else {
                                    $body = $mailContent;
                                }

                                require_once '../libs/PHPMailer-master/src/Exception.php';
                                require_once '../libs/PHPMailer-master/src/PHPMailer.php';
                                require_once '../libs/PHPMailer-master/src/SMTP.php';

                                if (sendMail($to, "", $subject, $body)) {
                                    $stmt = $greenf = null;
                                    echo '<p class="text-center" style="color: green;">Account recovery email sent to ' . $to . '. Please open this email to reset your password.</p>';
                                    echo '<p class="text-center" class="m_info">If you don’t see this email in your inbox within 15 minutes, look for it in your junk mail folder. If you find it there, please mark it as “Not Junk”.</p>';
                                    echo '<p class="text-center" class="m_info">If you are still experiencing any problems, contact <a href="../contact-us.php" target="_blank">support</a>.</p>';
                                } else {
                                    $stmt = $greenf = null;
                                    echo '<p class="text-center">The system is busy please try later.</p>';
                                }
                                endTheWebpage(); // close div, main, include footer, close body and html tags

                            } // end of existsInDatabase() if condition block
                        } catch (Exception $e) {
                            $date = '[' . date('d-M-Y h-i-s a') . ']';
                            $msg = $e->getMessage();
                            $err_msg = $date . ' | Exception Error | ' . $msg  . ' | \n';
                            error_log($err_msg, 3, ERROR_LOG_BOOK);
                            // error_log("Date/time: $date, Exception Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Exception Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");
                        } catch (Error $e) {
                            $date = '[' . date('d-M-Y h-i-s a') . ']';
                            $msg = $e->getMessage();
                            $err_msg = $date . ' | Error | ' . $msg  . ' | \n';
                            error_log($err_msg, 3, ERROR_LOG_BOOK);
                            // error_log("Date/time: $date, Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");
                        } // end of try catch blocks
                    } // end of empty($errors) if condition block without else block. The errors will be displayed next to the input field.
                }
                ?>
                <form id="reset-password-form" class="m_form" name="reset-password-form" method="post" action="<?php echo htmlspecialchars($_SERVER['PHP_SELF']); ?>">
                    <p class="text-center">Forgot your account’s password or having trouble logging into your account? Enter your email address and we’ll send you a recovery link.</p>
                    <div class="mb-3"></div>
                    <div class="row form-group">
                        <label class="col-sm-4 text-left col-form-label font-weight-bold" for="email">Email<span style="color: red;">&nbsp;*</span></label>
                        <div class="col-sm-8">
                            <input class="form-control" id="email" type="email" name="email" required title="Enter your email" value="<?php if (isset($_POST['email'])) {
                                                                                                                                            echo htmlspecialchars($_POST['email'], ENT_QUOTES);
                                                                                                                                        } ?>">
                            <label class="error"><?php if (isset($errors['email'])) {
                                                        echo htmlspecialchars($errors['email'], ENT_QUOTES);
                                                    } ?></label>
                        </div>
                    </div>

                    <div class="row form-group">
                        <label class="col-sm-4 col-form-label text-left"></label>
                        <div class="col-sm-8 text-center">
                            <button class="btn btn-success M_button" id="reset-password-button" type="submit" name="reset-password-button">Send recovery email</button>
                        </div>
                    </div>
                </form>

            </div>
        </main>
    </div>
    <footer id="footer">
        <?php include '../includes/footer.php'; ?>
    </footer>

    <!-- SCRIPTS -->
    <script type="text/javascript" src="..\js\jquery-3.4.1.min.js"></script>
    <script type="text/javascript" src="..\css\bootstrap-4.3.1\js\bootstrap.min.js"></script>
</body>

</html>

users/reset-password.php reset-password.php: this script validates the token_id & token, allows the user to reset the password

<?php
if (session_status() === PHP_SESSION_NONE) {
    session_start();
}

// Maintenance
require '../../d32b6f67a9d2ff687d9b7d.php';
require '../system/utils.php';

if (isset($_SESSION['user_id'])) {
    // user is already verified and logged in
    // so redirect to index page
    header('location:' . $home);
    exit();
}

// error log
define('ERROR_LOG_BOOK', '../../error.log');

// functions
function endTheWebpage()
{
    echo '</div>';
    echo '</main>';
    echo '</div>';
    echo '<footer id="footer">';
    include '../includes/footer.php';
    echo '</footer>';

    echo '<!-- SCRIPTS -->';
    echo '<script type="text/javascript" src="..\js\jquery-3.4.1.min.js"></script>';
    echo '<script type="text/javascript" src="..\css\bootstrap-4.3.1\js\bootstrap.min.js"></script>';
    echo '</body>';

    echo '</html>';
    exit();
}
?>
<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="description" content="">
    <meta name="author" content="">
    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
    <link rel="stylesheet" href="..\css\bootstrap-4.3.1\css\bootstrap.min.css" type="text/css">
    <link rel="stylesheet" href="..\css\main.css" type="text/css">
    <title>Reset Password | <?php echo htmlspecialchars($webpageTitle, ENT_QUOTES); ?></title>
</head>

<body>
    <header id="header">
        <?php
        $menu = 4; // activation.php
        include '../includes/header.php';
        ?>
    </header>
    <div class="container">
        <main>
            <div id="reset-password-area">
                <h1 class="text-center">Reset Password</h1>
                <?php
                if ($_SERVER['REQUEST_METHOD'] === 'POST') {
                    $token_id = isset($_SESSION['id']) ? filter_var($_SESSION['id'], FILTER_SANITIZE_STRING) : "";
                    $token = isset($_SESSION['token']) ? filter_var($_SESSION['token'], FILTER_SANITIZE_STRING) : "";
                } else {
                    $token_id = isset($_GET['id']) ? filter_var($_GET['id'], FILTER_SANITIZE_STRING) : "";
                    $token = isset($_GET['token']) ? filter_var($_GET['token'], FILTER_SANITIZE_STRING) : "";
                    $_SESSION['id'] = $token_id;
                    $_SESSION['token'] = $token;
                }

                if (!empty($token_id) && strlen($token_id) === 64 && !empty($token) && strlen($token) === 64) {
                    // token and token_id seems to be valid
                    // proceed to check inside the database

                    try {
                        require_once '../../c6ef0226276fb386d4a7a7dcb4df38ee9df1105a2244b7bd.php';
                        $greenf = create_db_linkage_instance();

                        $qry = 'SELECT * FROM ' . $t_passwordReset . ' WHERE token_id=:token_id';
                        $stmt = $greenf->prepare($qry);
                        $stmt->bindParam(':token_id', $token_id, PDO::PARAM_STR);
                        $stmt->execute();

                        if ($stmt->rowcount() === 1) {
                            $data = $stmt->fetch(PDO::FETCH_OBJ);
                            $email = htmlspecialchars($data->email, ENT_QUOTES);
                            $hashed_token = $data->token;
                            $requestTime = htmlspecialchars($data->request_time, ENT_QUOTES);
                            $expiresDate = date("Y-m-d H:i:s", strtotime($requestTime . " +30 minute"));
                            $expires = DateTime::createFromFormat("Y-m-d H:i:s", $expiresDate);
                            $currentDateTime = DateTime::createFromFormat("Y-m-d H:i:s", date("Y-m-d H:i:s"));

                            if ($currentDateTime < $expires) {
                                // the token is not expired
                                if (password_verify($token, $hashed_token)) {
                                    // display the password reset form
                ?>
                                    <form class="m_form" id="reset-password-form" name="reset-password-form" action="<?php echo htmlspecialchars($_SERVER['PHP_SELF']); ?>" method="POST">
                                        <div class="mb-3"></div>
                                        <?php
                                        // validate the password submitted to reset
                                        if ($_SERVER['REQUEST_METHOD'] === 'POST') {
                                            $newPassword = isset($_POST['new_password']) ? $_POST['new_password'] : "";
                                            $confirmPassword = isset($_POST['confirm_password']) ? $_POST['confirm_password'] : "";
                                            if (!empty($newPassword)) {
                                                if ($newPassword !== $confirmPassword) {
                                                    $errors['password'] = 'Your new password and confirm password do not match.';
                                                } else if (!preg_match("/(?=.*[a-z])(?=.*[A-Z])(?=.*[\d]).{8,}/", $newPassword)) {
                                                    $errors['password'] = 'Password must contain at least eight characters, including at least 1 uppercase letter, one lowercase letter and 1 number.';
                                                }
                                            } else {
                                                $errors['password'] = 'Please enter your new password.';
                                            }

                                            if (empty($errors)) {
                                                $hashed_password = password_hash($newPassword, PASSWORD_DEFAULT);
                                                $temp = findIP();
                                                if (filter_var($temp, FILTER_VALIDATE_IP) === false) {
                                                    // invalid ip
                                                    // set it to 0.0.0.0
                                                    $ip = "0.0.0.0";
                                                } else {
                                                    $ip = $temp; // valid ip
                                                }

                                                $qry = 'UPDATE ' . $t_users . ' SET password=:password, last_login_ip=:last_login_ip WHERE email=:email';
                                                $stmt = $greenf->prepare($qry);
                                                $stmt->bindParam(':password', $hashed_password, PDO::PARAM_STR);
                                                $stmt->bindParam(':last_login_ip', $ip, PDO::PARAM_STR);
                                                $stmt->bindParam(':email', $email, PDO::PARAM_STR);
                                                $stmt->execute();

                                                if ($stmt->rowcount() === 1) {
                                                    // new password updated successfully
                                                    $qry = 'DELETE FROM ' . $t_passwordReset . ' WHERE token_id=:token_id';
                                                    $stmt = $greenf->prepare($qry);
                                                    $stmt->bindParam(':token_id', $token_id, PDO::PARAM_STR);
                                                    $stmt->execute();

                                                    unset($_SESSION['id']);
                                                    unset($_SESSION['token']);
                                                    $stmt = $greenf = null;
                                                    echo '<p class="text-center" style="color: green;">Congratulations! Your password has been changed successfully. Please <a href="login.php">click here</a> to login and enjoy.';
                                                    endTheWebpage();
                                                } else {
                                                    $stmt = $greenf = null;
                                                    echo '<p class="text-center">The system is busy please try later.</p>';
                                                    endTheWebpage();
                                                } // end of rowcount() === 1 if condition block (Update query);
                                            } // end of empty($errors) if condition block without else block (errors will be displayed next to the textboxes
                                        } // end of $_SERVER['REQUEST_METHOD'] === 'POST if condition block to validate the password and update the database with the new password
                                        ?>
                                        <p class="text-center">Please use the form below to reset your password.</p>
                                        <div class="mb-3"></div>
                                        <div class="row form-group">
                                            <label for="new_password" class="col-sm-4 col-form-label text-left font-weight-bold">New password<span style="color: darkred;">&nbsp;*</span></label>
                                            <div class="col-sm-8">
                                                <input class="form-control m_text" id="new-password" type="password" name="new_password" required value="<?php if (isset($_POST['new_password'])) {
                                                                                                                                                                echo htmlspecialchars($_POST['new_password']);
                                                                                                                                                            } ?>" pattern="(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9]).{8,}" title="Password must contain at least eight characters, including at least 1 uppercase letter, one lowercase letter and 1 number.">
                                                <label class="error"><?php if (isset($errors['password'])) {
                                                                            echo htmlspecialchars($errors['password'], ENT_QUOTES);
                                                                        } ?>Password must contain at least eight characters, including at least 1 uppercase letter, one lowercase letter and 1 number.</label>
                                            </div>
                                        </div>

                                        <div class="row form-group">
                                            <label for="confirm_password" class="col-sm-4 col-form-label text-left font-weight-bold">Confirm password<span style="color: darkred;">&nbsp;*</span></label>
                                            <div class="col-sm-8">
                                                <input class="form-control m_text" id="confirm-password" type="password" name="confirm_password" required value="<?php if (isset($_POST['confirm_password'])) {
                                                                                                                                                                        echo htmlspecialchars($_POST['confirm_password']);
                                                                                                                                                                    } ?>">
                                            </div>
                                        </div>


                                        <div class="row form-group">
                                            <label class="col-sm-4 col-form-label text-left"></label>
                                            <div class="col-sm-8 text-center">
                                                <button class="btn btn-success M_button" id="reset-password-button" type="submit" name="reset-password-button">Reset password</button>
                                            </div>
                                        </div>
                                    </form>
                <?php
                                } else {
                                    // the token hash not matched with the token hash in the database
                                    $stmt = $greenf = null;
                                    echo '<p class="text-center" style="color: ed;">The link is invalid.</p>';
                                    echo '<p class="text-center">Please <a href="forgot-password.php">click here</a> to get a new link to reset your password.</p>';
                                    endTheWebpage();
                                }
                            } else {
                                // the token has been expired
                                $stmt = $greenf = null;
                                echo '<p class="text-center" style="color: ed;">The link has been expired.</p>';
                                echo '<p class="text-center">Please <a href="forgot-password.php">click here</a> to get a new link to reset your password.</p>';
                                endTheWebpage();
                            }
                        } else {
                            // no data returned from the database
                            $stmt = $greenf = null;
                            echo '<p class="text-center" style="color: red;">The link is invalid.</p>';
                            echo '<p class="text-center">Please <a href="forgot-password.php">click here</a> to get a new link to reset your password.</p>';
                            endTheWebpage();
                        } // end of $stmt->rowcount() === 1 if condition block ('SELECT QUERY')
                    } catch (Exception $e) {
                        $date = '[' . date('d-M-Y h-i-s a') . ']';
                        $msg = $e->getMessage();
                        $err_msg = $date . ' | Exception Error | ' . $msg  . ' | \n';
                        error_log($err_msg, 3, ERROR_LOG_BOOK);
                        // error_log("Date/time: $date, Exception Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Exception Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");
                    } catch (Error $e) {
                        $date = '[' . date('d-M-Y h-i-s a') . ']';
                        $msg = $e->getMessage();
                        $err_msg = $date . ' | Error | ' . $msg  . ' | \n';
                        error_log($err_msg, 3, ERROR_LOG_BOOK);
                        // error_log("Date/time: $date, Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");
                    } // end of try catch blocks
                } else {
                    $stmt = $greenf = null;
                    echo '<p class="text-center" style="color: red;">The link is invalid.</p>';
                    echo '<p class="text-center">Please <a href="forgot-password.php">click here</a> to get a new link to reset your password.</p>';
                    endTheWebpage();
                } // end of !empty($token_id) && strlen($token_id) === 64 && !empty($token) && strlen($token) === 64 if condition block
                ?>
            </div>
        </main>
    </div>
    <footer id="footer">
        <?php include '../includes/footer.php'; ?>
    </footer>

    <!-- SCRIPTS -->
    <script type="text/javascript" src="..\js\jquery-3.4.1.min.js"></script>
    <script type="text/javascript" src="..\css\bootstrap-4.3.1\js\bootstrap.min.js"></script>
</body>

</html>

I am a beginner to PHP. Is this the correct way to implement password reset system? Are there any security issues in my code? If yes, what are they? How to overcome them?

\$\endgroup\$
6
\$\begingroup\$

Naming things

I love that you take the time to name things properly. Names like create_db_linkage_instance(), endTheWebpage() immediately make clear what these functions do. It's not db_conn() or finish(). Of course there is the problem that you haven't stuck to one naming convenstion, choose either snake_case or or camelCase and stick to it. Yes, I know that PHP itself is, regrettably, not very consistent. Later, while reading through your code, I came across less well-chosen names: $greenf, which is a database instance, and $qry for, you guessed it right, a MySQL query.

Another really weird thing are the d32b6f67a9d2ff687d9b7d.php and c6ef0226276fb386d4a7a7dcb4df38ee9df1105a2244b7bd.php file names. My best guess is that you try to obsure the location of these files, however, this is not the way to do it. Security through obscurity is generally discouraged especially if it makes the job of understanding what a file is for harder. Simply put the file out of reach of any potential hacker, as you already do.

Configuration

You're using "a lot" of constants to set up your database connection. Have a look at this question, that was just answered. The fewer names you define yourself, global to the whole of PHP, the less change of a naming conflict (in the future).

Repetition of code

What we, programmers, don't like is endless repetition of the same pieces of code. We're allergic to it. We even have a name for code like that: It isn't DRY. In the create_db_linkage_instance() function you catch exceptions and errors in exactly the same way. And this is repeated four times more in the rest of your code. This is shouting for a separate function.

There's also code like this:

$mailContent = str_replace("{{HomePage}}", $home, $mailContent);
$mailContent = str_replace("{{Logo}}", $home . "images/logo.png", $mailContent);
$mailContent = str_replace("{{WebpageTitle}}", $webpageTitle, $mailContent);
$mailContent = str_replace("{{Name}}", $displayName, $mailContent);
$mailContent = str_replace("{{PasswordResetLink}}", $url, $mailContent);
$mailContent = str_replace("{{Expires}}", $expires, $mailContent);
$mailContent = str_replace("{{ForgetPasswordResetLink}}", $home . "users/cancel-password-reset-request.php", $mailContent);
$mailContent = str_replace("{{IP}}", $ip, $mailContent);
$mailContent = str_replace("{{AccountRecoveryLink}}", $home . "users/forgot-password.php", $mailContent);
$mailContent = str_replace("{{ContactUs}}", $home . "contact-us.php", $mailContent);

This could be:

$tokens = ["HomePage"                => $home,
           "Logo"                    => $home . "images/logo.png",
           "WebpageTitle"            => $webpageTitle,
           "Name"                    => $displayName,
           "PasswordResetLink"       => $url,
           "Expires"                 => $expires,
           "ForgetPasswordResetLink" => $home . "users/cancel-password-reset-request.php",
           "IP"                      => $ip,
           "AccountRecoveryLink"     => $home . "users/forgot-password.php",
           "ContactUs"               => $home . "contact-us.php"];

foreach ($tokens as $token => $value) {
   $mailContent = str_replace("{{" . $token . "}}", $value, $mailContent);
}

This code is not faster, but it is easier to read, and the repetition is now done by a loop.

Nesting

Your code uses very few functions, no classes, and relies heavily on nesting one bit of code inside another. Let me take your reset-password.php file and write out the main control structure:

if (...) {
    try {
        if (...) {
            if (...) {
                if (...) {
                    if (...) {
                        if (...) {
                            if (...) {
                            } else if (...) {
                            }
                        } else {
                        }
                        if (...) {
                            if (...) {
                            } else {
                            }
                            if (...) {
                            } else {
                            } 
                        } 
                    } 
                    if (...) {
                    }
                    if (...) {
                    }
                } else {
                }
            } else {
            }
        } else {
        } 
    } catch (...) {
    } catch (...) {
    } 
} else {
} 

That looks complex, and it is. Now it is only 36 lines long, but in reality it is more than 150 lines of code. This feels like a maze. It's a wonder it works at all. This is not a good way to write secure code, because you quickly lose sight of what is happening.

So what to do? For starters: Use functions, as I mentioned before. Divide the code into logical chunks. In this case that would be a good alternative to all this nesting. The control structure might even stay the same, but because it is wrapped in a few, clearly named, functions, it stays understandable. Something like:

if (isValidUserToken($token_id)) {
   if (isValidPasswordHash($token) {
      ChangePassword();
   }
}

I know, this is far from complete, it might not even be correct, but it is just an illustration of how you can keep your code the same but divide it up in functional chunks. This is, of course the same as:

if (isValidUserToken($token_id) &&
    isValidPasswordHash($token)) {
   ChangePassword();
}

Which shows you that structuring your code this way opens the way to simplify it further. The isValidUserToken($token_id) && isValidPasswordHash($token) could probably be combined into a function called isValidUser($token_id, $token). There's nothing wrong with a one-line function, as long as it is used multiple times.

Is this code secure

I really cannot tell. Given the problems mentioned above, it is very difficult to find bugs in the code. There are many globals floating around, a complex control flow, mixing of PHP processing and HTML output, and generally difficult to read code.

This is further complicated because you haven't really told us how your "forgot password reset system" is supposed to work. From what I can see, you've given us more than you implied. I think there's a "forgot password" and a "reset password" piece of code. Then again, the latter could be part of the first: When An user tells your system they forgot their password, it emails something that allows them to reset their password. That makes sense. It hinges on the user being in control of their mail address.

Something that has happened to me quite a lot: I lost my email address, for instance when changing company, ISP, or when selling a domain. Sites often identify you only by your email address. This is understandable, but there will be users who do not possess their email addresses anymore. Big websites have to deal with this somehow, although I know quite a few that simply refuse that and still take your money (I can name Adobe and Vimeo from experience).

Sorry about that, it has little to do with your code. However, as I said, I cannot verify that your code is secure. It would require a long study of your code, or a complete rewrite, and that just takes too much time. There are many good things about your code: You correctly use parameter binding in your PDO queries, and you take care to check a lot of things, but in the end the code is a jumble which is hard to read and understand.

My advice would be to look at the queries you have and think about what they do. You can then put that in tidy functions. For instance, this piece of code has a two clear functions:

$hashed_password = password_hash($newPassword, PASSWORD_DEFAULT);
$temp = findIP();
if (filter_var($temp, FILTER_VALIDATE_IP) === false) {
    // invalid ip
    // set it to 0.0.0.0
    $ip = "0.0.0.0";
} else {
    $ip = $temp; // valid ip
}

$qry = 'UPDATE ' . $t_users . ' SET password=:password, last_login_ip=:last_login_ip WHERE email=:email';
$stmt = $greenf->prepare($qry);
$stmt->bindParam(':password', $hashed_password, PDO::PARAM_STR);
$stmt->bindParam(':last_login_ip', $ip, PDO::PARAM_STR);
$stmt->bindParam(':email', $email, PDO::PARAM_STR);
$stmt->execute();

It updates the user table with the new password and it stores the IP address of this login. So, the two functions would become:

function updateUserPasswordByEmail($database, $email, $newPassword)
{
    $hashedPassword = password_hash($newPassword, PASSWORD_DEFAULT);
    $query = "UPDATE users SET password = :password WHERE email = :email";
    $statement = $database->prepare($query);
    $statement->bindValue(':password', $hashedPassword, PDO::PARAM_STR);
    $statement->bindValue(':email', $email, PDO::PARAM_STR);
    return $statement->execute();
}

function storeUserIpAddressByEmail($database, $email)
{
    $IpAddress = findIP();
    if (!filter_var($temp, FILTER_VALIDATE_IP)) {
        $IpAddress = "0.0.0.0"; // invalid ip
    }
    $query = "UPDATE users SET last_login_ip= :last_login_ip WHERE email = :email";
    $statement = $database->prepare($query);
    $statement->bindValue(':last_login_ip', $IpAddress, PDO::PARAM_STR);
    $statement->bindValue(':email', $email, PDO::PARAM_STR);
    return $statement->execute();
}

These are nice and short pieces of code which you can wrap your mind around. You can state definitely that you know what they are doing. You can also test them, over and over again, independent of the other code.

Notice that I use bindValue() instead of bindParam(). This makes sense because I only need to associate the value with the placeholder once.

You might have noticed that the two update queries, in the above functions, look very similar. If we make more similar functions, all updating one field in the user table, it would not be very DRY. So let's work on that:

function updateUserFieldByEmail($database, $email, $column, $value, $type)
{
    $query = "UPDATE users SET $column = :value WHERE email = :email";
    $statement = $database->prepare($query);
    $statement->bindValue(':value', $value, $type);
    $statement->bindValue(':email', $email, PDO::PARAM_STR);
    return $statement->execute();
}

function updateUserPasswordByEmail($database, $email, $newPassword)
{
    $hashedPassword = password_hash($newPassword, PASSWORD_DEFAULT);
    return updateUserFieldByEmail($database, $email, 'password', $hashedPassword, PDO::PARAM_STR);
}

function storeUserIpAddressByEmail($database, $email)
{
    $IpAddress = findIP();
    if (!filter_var($temp, FILTER_VALIDATE_IP)) {
        $IpAddress = "0.0.0.0"; // invalid ip
    }
    return updateUserFieldByEmail($database, $email, 'last_login_ip', $IpAddress, PDO::PARAM_STR);
}

As you can see these functions get shorter and easier to understand. The chance that you might have missed something that could be a security issue is therefore smaller. This is (almost) the whole point of structured programming, it exists to help the reader, that's you, to understand the code.

This discussion is far from exhaustive, but I hope it is useful to you.

Here's a link you might find useful: https://phptherightway.com

\$\endgroup\$
3
  • 1
    \$\begingroup\$ psst! str_replace already works with arrays, though separate ones. And strtr works with the exact array you made \$\endgroup\$ – Your Common Sense Feb 1 at 5:48
  • 1
    \$\begingroup\$ Wow, I was just going to say the same thing. If you are going to construct an associative array, just call strtr(). (It just took me ~15 minutes to read this review) I found trim() to be unnecessary after sanitizing the email. empty() is not necessary when a variable is guaranteed to be declared -- just use a falsey check. \$\endgroup\$ – mickmackusa Feb 1 at 6:05
  • \$\begingroup\$ @YourCommonSense That's true, but you're ignoring the {{ and }}, these would have to be added in the array, I chose this method. \$\endgroup\$ – KIKO Software Feb 1 at 8:22
4
\$\begingroup\$

In addition to the magnificent answer from KIKO Software, a few pointers...

Getting IP

First of all there is at least one issue connected to security; namely the findIP() function. Many beginners are using a similar approach and feeling smart. In reality, this function makes your code less reliable and secure as having this very function actually allows the IP to be easily spoofed. See my other answer for the details. Stick to REMOTE_ADDR only and you're good to go.

Error reporting / Code repetition

The amount code repetition (mentioned in the other answer) is just unacceptable (however, it takes some experience to get an eye for the code repetitions and nobody really expects a beginner to catch these). The two code blocks differ by one symbol (or so I can tell because finding differences in the identical code blocks is a job by itself).

$date = '[' . date('d-M-Y h-i-s a') . ']';
$msg = $e->getMessage();
$err_msg = $date . ' | Exception Error | ' . $msg  . ' | \n';
error_log($err_msg, 3, ERROR_LOG_BOOK);
// error_log("Date/time: $date, Exception Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Exception Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");

$date = '[' . date('d-M-Y h-i-s a') . ']';
$msg = $e->getMessage();
$err_msg = $date . ' | Error | ' . $msg  . ' | \n';
error_log($err_msg, 1, ERROR_LOG_BOOK);
// error_log("Date/time: $date, Error check error log for more details", 1, WEB_MASTER_EMAIL, "Subject: Error \nfrom: Error Log <" . ERROR_LOG_EMAIL . "> \r\n");

Moreover, these almost identical blocks are repeated in the code again!

But what I really don't understand is why there is so much attention to the database connection errors as compared to a complete neglect towards many other errors in the code!

Say, connecting to a mail server may result in none the less illustrious error, but we don't see even a hint of the error handling, least such a hairy one.

Or, including a file may result in the error, when a file is missing or has a permission problem. Or sending a header almost certainly at some point will produce the infamous "Headers already sent" error. Or literally any line of any code may produce an error. But for some reason, only some code blocks are wrapped in a try catch.

At least wrap the entire code in a single try catch, so that you won't have to repeat the handling code and also it will be able to handle all possible errors, not just a subset of them.

And of course make this code to catch a single \Throwable class exception with some condition inside to distinguish the type (in case you really need it, which I highly doubt).

Also, think of your code form this point of view: it is only good for a programmer, while a site visitor doesn't get a clear idea what something goes wrong. For a site user you have to provide a generic excuse page.

The separation of the business logic and presentation logic

As you already learned (from the fact you are using endTheWebpage()), intertwining the application logic with the presentation logic makes your life incredibly hard. And you must separate them completely:

  • make your application code do all of its job first
  • only then start sending HTML to the client

This simple approach will make both parts far clearer and easier to understand.

Following the good practices

Right ow I am working on the article where I am trying to sum up my experience in both the web development and answering questions on Stack Overflow. It's till work in progress, but it already has a lot of important information on the very topics that have been highlighted here, and you may find it useful: The most important basic principles of web programming. In particular it explains the correct principles of error reporting and the application/presentation logic separation.

\$\endgroup\$
0

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.