Forgotten password / password reset

I have hand-coded a forgotten-password reset system. I am apprehensive about security issues/vulnerabilities. What part of this can be exploited or could be made more secure?

forgot-password.php

```
0) {

$hash = sha1(md5(mt_rand(0, 1000000000)));

$forgot_password_hash_query = "UPDATE users SET forgot_password_hash = '{$hash}'
WHERE email = '{$email}'";

$do_forgot_password_hash_query = mysqli_query($connection, $forgot_password_hash_query);

if($do_forgot_password_hash_query) {

$data = array("result" => 1, "message" => "Link To Reset Your Password Has Been Successfully
Sent To $email");

sendEmail($email, $hash);

}
else {
$data = array("result" => -5, "message" => "Error Encountered! Try Again Later.");
}

}
else {
$data = array("result" => -4, "message" => "Non-Existant Email Address! Perhaps You Need To Register
For An Account.");
}

}
else {
$data = array("result" => -3, "message" => "Encountered A Problem Resetting Your Password!
Please Try Again Later.");
}

}
else {
$data = array("result" => -2, "message" => "Invalid Email Address! yourname@domain.com is an example
of the correct format.");
}

}
else {
$data = array("result" => -1, "message" => "No Email Address Provided!");
}

}
else {
$data = array("result" => 0, "message" => "Incorrect Request Method!");
}

function sendEmail ($recipient, $hash) {

$to = $recipient;
$subject = "Reset Your Password | DOMAIN.COM";
$message = '

Did you just forget your password ?

Security

Your code should be secure.

But just putting all user input through htmlentities and mysqli_real_escape_string is not really the recommended approach.

The recommended approach against SQL injection are prepared statements.

The recommended approach against XSS is htmlentities, but it should be applied when echoing data, not when reading it for the first time. There are two reasons for this:

• It is hard to keep track which variables are save to echo and which are not. This can very easily lead to bugs.

• You get dirty data, which can hurt usability. For example, what if my email address is foo&bar'foo@example.com? I would get foo&bar\'foo@example.com, and would thus not get an email.

Apart from that, it is customary to expire forgotten-password-links. The reason for this is that an attacker who can read data from the database can get into users accounts without having to decrypt the passwords.

Nesting

Your code is too deeply nested, making it hard to read. If you have 6 closing elses in a row, it is really hard to see which if they actually close.

You can reduce nesting by reversing your ifs:

if($_SERVER['REQUEST_METHOD'] === 'GET') {
    $data = array("result" => 0, "message" => "Incorrect Request Method!"); 
} else {
    [...]
}

I think it's nicer to just introduce functions and return:

function emailFormSubmittion($email) {
    if($_SERVER['REQUEST_METHOD'] === 'GET') {
        return array("result" => 0, "message" => "Incorrect Request Method!"); 
    } 

    if(!empty($_GET['email'])) {
        return array("result" => -1, "message" => "No Email Address Provided!");    
    }

    [...]
}

Misc

• You don't need to check isset if you use empty.

• You have quite a lot of unnecessary newlines.

• I think the structure of your code is a bit confusing. The naming doesn't really reveal what the responsibility of each file is (eg what's the difference between reset and do reset?), and each file does a lot of different things (send emails, read userdata, create forms, read from database, write to database, manage session, output data, etc).

StackExchange Code Review Q#111105, answer score: 3