My lost password function I'm trying to create a lost password function on my site. When a user enter their email it updates 2 columns in the users table with that email called: new_password_key and new_password_requested (int, using PHP time()), I then send them a link with that password key to a page where they may change their password.

It seems to work, and if they don't visit the link within 24 hours it's considered invalid and they have to request a new one.

Is it OK? Safe? Is there something else I should think of?

This is the https://mysite.com/password/key_goes_here page:

$value = isset($_GET['key']) ? $_GET['key'] : null;
$query = $db-&gt;prepare("SELECT new_password_key, new_password_requested FROM users WHERE new_password_key = ?");
$query-&gt;execute(array($value));
$row = $query-&gt;fetch();

if (!$row) {
    // not found
    header("Location: /password");
    die;
}

// if this key is older than 24 hours its invalid (expired)
if ($row['new_password_requested'] &lt;= time() - (60 * 60 * 24)) {
    $query = $db-&gt;prepare("UPDATE users SET new_password_key = NULL, new_password_requested = 0 WHERE new_password_key = ?");
    $query-&gt;execute(array($value));
    header("Refresh: 3;url='/password'");
    return false;
} 


if (count($_POST) &gt; 0) {

    $new_password1 = trim($_POST['new_password1']);
    $new_password2 = trim($_POST['new_password2']);

    if (strlen($new_password1) &lt; 4)
        $error = "passwords must be at least 4 characters long. please choose another (longer) password";
    else if ($new_password1 != $new_password2)
        $error = "passwords do not match";

    if (empty($error)) {
        $hash = password_hash($new_password1, PASSWORD_DEFAULT);
        $query = $db-&gt;prepare("UPDATE users SET password = ?, new_password_key = NULL WHERE new_password_key = ?");
        $query-&gt;execute(array($hash, $row['new_password_key']));

    }

}


This is the form where you enter your email to get a key:

if (count($_POST) &gt; 0) {

$email = trim($_POST['email']);

$query = $db-&gt;prepare("SELECT email, new_password_key, new_password_requested FROM users WHERE email = ?");
$query-&gt;execute(array($email));
$row = $query-&gt;fetch();

if (empty($email))
    $error = "this field can not be empty";
else if (!filter_var($email, FILTER_VALIDATE_EMAIL))
    $error = "not a valid email address";
else if (!empty($row['new_password_key']))
    $error = 'a link has already been sent';
elseif ($query-&gt;rowCount() &lt; 1)
    $confirmed = true;


if (empty($error)) {

    if ($row) {
        // email exists, send them mail
        $query = $db-&gt;prepare("UPDATE users SET new_password_key = ?, new_password_requested = ? WHERE email = ?");
        $query-&gt;execute(array(generateRandomString(25), time(), $row['email']));
        sendMail($email, 'lostpassword');
        $confirmed = true;
    } else {
       // email dont exist, tell them an email has been dispatched but dont actually send a mail we dont want them to know what emails are registered on this site
       $confirmed = true;
    }
}
}
This seems quite fine. It's good that you are using prepared statements for all database queries.

What bugs me the most is that the users table is reused for multiple purposes:


Store essential user information such as email and password
Store information about tracking reset password requests


It would be better to use another table for the 2nd purpose.
The reason is that the users table is an essential piece of your site and you wouldn't want anything to happen to it by accident.
So the less you touch that table, the better.
Besides, for most users the new_password_key and new_password_requested fields will be blank / unused most of the time.



The variable names are sometimes not so great. For example:


$value = isset($_GET['key']) ? $_GET['key'] : null;



$value is a key, so I suggest to name it accordingly.



One last small thing, I recommend to use braces with all if, else if statements.