my "remember me"-feature

Question

Finally I have got this function to work. It does its job but it looks real messy, just wanna hear your thoughts and maybe theirs something I could improve?

Thanks alot!

Login

$result = mysql_query("SELECT * FROM users WHERE username = '".mysql_real_escape_string($_POST['username'])."' AND password = '".md5($_POST['password'])."'");
$row = mysql_fetch_object($result);

    if (mysql_num_rows($result) == 0) {
        exit('Bad Login');
    }

    $_SESSION['id'] = mysql_result($result, 0, 'id');

    # The user wants to be remembered
    if (isset($_POST['remember'])) {
        $key = md5(uniqid());
        setcookie('remember', $key, time()+900000);  /* expire in 10 days */
        mysql_query("UPDATE users SET sessionkey = '$key' WHERE id = ".mysql_result($result, 0, 'id'));

    }

And on each page I check:

if (isset($_SESSION['id'])) {
header("Location: welcome.php");
}

elseif (isset($_COOKIE['remember'])) {

$rs = mysql_query("SELECT * FROM users WHERE sessionkey = '$_COOKIE[remember]'");

if (mysql_num_rows($rs) == 1) {
    $_SESSION['id'] = 1;
header("Location: welcome.php");
}

}

Answer 1

I'm no PHP expert, but there's probably an off-the-shelf solution for session management - I would suggest using it over rolling your own.

Answer 2

Put the code that checks if the number of rows is zero before the mysql_fetch_object($result) statement. That way, you don't waste that extra CPU cycle if the user doesn't exist.

Change this (in both places):

mysql_result($result, 0, 'id');

to

$row->id;

Also, if your id column isn't sanitary (i.e.: the user has entered some data for it at some point), you're going to want to escape it in your UPDATE query.

Just a matter of preference on this one, but when I check URL parameters existance, I like to use !empty() instead of isset. The reason is that if the parameter is set but empty, it will still return false:

!empty($_POST['remember'])

Also on that note, be sure to sanitize $_COOKIE['remember']. Cookie values can be changed by the user.

mysql_query("SELECT * FROM users WHERE sessionkey = '" . mysql_real_escape_string($_COOKIE[remember]) . "'");

Lastly, it might be a good idea not to select * in your query, as this can bump you up against a performance wall later on in your app. Consider just selecting, say, the ID of the user:

mysql_query("SELECT id FROM users ...

Everything else looks pretty good!