mysql resetting password for user and script improvement

[Himself12794]

I'm trying to write a script that allows a user to reset their password if they've forgotten it. It works, but I want to know if there are any errors in the script I should fix, and if there is any way I can improve it or condense it to save space.

Here it is:

<?php
//Start session
session_start();

//Include database connection details
require_once('config.php');

//Array to store validation errors
$errmsg_arr = array();

//Validation error flag
$errflag = false;

//Connect to mysql server
$link = mysql_connect(DB_HOST, DB_USER, DB_PASSWORD);
if(!$link) {
	die('Failed to connect to server: ' . mysql_error());
}

//Select database
$db = mysql_select_db(DB_DATABASE);
if(!$db) {
	die("Unable to select database");
}

//Function to sanitize values received from the form. Prevents SQL injection
function clean($str) {
	$str = @trim($str);
	if(get_magic_quotes_gpc()) {
		$str = stripslashes($str);
	}
	return mysql_real_escape_string($str);
}

//Get form values and clean them
$login = clean($_POST['login']);
$email = clean($_POST['email']);
$newpassword = mt_rand().mt_rand();

//Get database values
$qry="SELECT * FROM members WHERE login='$login' AND email='$email'";
$result = @mysql_query($qry);

//Input Validations
if($login == '') {
	$errmsg_arr[] = 'Login ID missing';
	$errflag = true;
}
if($email == '') {
	$errmsg_arr[] = 'Email missing';
	$errflag = true;
}

//See if login exsists
if($login != '') {
	$qry = "SELECT * FROM members WHERE login='$login'";
	$result = mysql_query($qry);
	if($result) {
		if(mysql_num_rows($result) == 0) {
			$errmsg_arr[] = 'That Login ID does not exsist. Are you trying to register?';
			$errflag = true;
		}
		@mysql_free_result($result);
	}
	else {
		die("Query failed");
	}
}

//See if email exsists
if($email != '') {
	$qry = "SELECT * FROM members WHERE email='$email'";
	$result = mysql_query($qry);
	if($result) {
		if(mysql_num_rows($result) == 0) {
			$errmsg_arr[] = 'That email address is not registered on our website.';
			$errflag = true;
		}
		@mysql_free_result($result);
	}
	else {
		die("Query failed");
	}
}

//See if login exists. This is one of the main things I want to condense
if($login != '' && $email != '') {
	$qry1 = "SELECT * FROM members WHERE login='$login'";
	$qry2 = "SELECT * FROM members WHERE email='$email'";
	$result1 = mysql_query($qry1);
	$result2 = mysql_query($qry2);
	$mems = mysql_fetch_assoc($result1);
	$memlog = $mems['member_id'];
	$mems1 = mysql_fetch_assoc($result2);
	$memem = $mems1['member_id'];
	if($result1 && $result2) {
		if($memlog != $memem) {
			$errmsg_arr[] = 'That Login ID and email do not match.';
			$errflag = true;
		}
		@mysql_free_result($result1);
		@mysql_free_result($result2);
	}
	else {
		die("Query failed");
	}
}

//If there are input validations, redirect back to the login form
if($errflag) {
	$_SESSION['ERRMSG_ARR'] = $errmsg_arr;
	session_write_close();
	header("location: reset.php");
	exit();
}

//Check whether the query was successful or not
//if($result) {
		//Change new password
		$qry2 = "UPDATE members SET passwd='".md5($newpassword)."' WHERE login='$login' AND email='$email'";
		@mysql_query($qry2);

//Send new password email. I want to improve this part of the code also.
$to      = $email;
$subject = "  New Password";
$message = "New password.\r\rYou, or someone using your email address, has requested a new password.\r\rLogin: $login\r\rNew Password: $newpassword\r\rRegards, Me";
$headers = 'From: noreply@ mywebsite.com' . "\r\n" .
    'Reply-To: noreply@ mywebsite.com' . "\r\n" . 
    'X-Mailer: PHP/' . phpversion();

mail($to, $subject, $message, $headers);
	header("location: reset-success.php");
	exit();

?>

Any help would be appreciated.

[Psycho]

1. You have two variable: $errmsg_arr & $errflag. In the validation, if an error is found you add an error description to $errmsg_arr and set $errflag to true. Then at the end you use $errflag to determine if errors occurred. $errflag is completely unnecessary. Just use count$errmsg_arr) to determine if any errors occurred. You can then get rid of the line that creates $errflag and all the lines that set it to true.

 

2. you are running the first query using $login & $email THEN you are validating the values? You should validate that the values are valid before running the query.

 

3. What is the purpose of the first query, anyway?

 

4. Don't do separate queries to check login and email. Do just one query searching for a record matching both values. That provides better security. If someone stumbles upon one right value they could continue to try alternates for the second value.

 

5. You have many, many queries and you only need one.

 

I could go on, but it would just be easier to show you how I might do it

<?php
//Start session
    session_start();

    //Include database connection details
    require_once('config.php');

    //Connect to mysql server
    if(!mysql_connect(DB_HOST, DB_USER, DB_PASSWORD))
    {
        die('Failed to connect to server: ' . mysql_error());
    }

    //Select database
    if(!mysql_select_db(DB_DATABASE))
    {
        die('Unable to select database: ' . mysql_error());
    }

    //Function to sanitize values received from the form. Prevents SQL injection
    function clean($str)
    {
        if(get_magic_quotes_gpc())
        {
            $str = stripslashes($str);
        }
        return mysql_real_escape_string(trim($str));
    }

   	//Array to store errors
    $errmsg_arr = array();

    //Get form values and clean them
    $login = clean($_POST['login']);
    $email = clean($_POST['email']);
    $newpassword = mt_rand().mt_rand();

    //Input Validations
    if(empty($login))
    {
        $errmsg_arr[] = 'Login ID missing';
    }
    if(empty($email))
    {
        $errmsg_arr[] = 'Email missing';
    }

    //Attempt to set new password value (only run if no previous errors)
    if(count($errmsg_arr)==0)
    {
        $pwHash = md5($newpassword);
        $qry = "UPDATE members
                SET passwd='$pwHash'
                WHERE login='$login' AND email='$email'";
        $result = mysql_query($qry);
        if(!$result)
        {
            die("Error running query: " . mysql_error());
        }
   
        //If there were no affected rows then there was not matching value
        if(mysql_affected_rows($result)==0)
        {
            $errmsg_arr[] = 'That Login ID and/or Email do not exsist. Are you trying to register?';
        }
        else
        {
            //Password was uipdated, send new password email.
            $to      = $email;
            $subject = "New Password";
            $message = "New password.\r\r
                        You, or someone using your email address, has requested a new password.\r\r
                        Login: $login\r\r
                        New Password: $newpassword\r\r
                        Regards, Me";
            $headers = "From: noreply@mywebsite.com\r\n" .
                       "Reply-To: noreply@ mywebsite.com\r\n" . 
                       "X-Mailer: PHP/" . phpversion();
            if(!mail($to, $subject, $message, $headers))
            {
                $errmsg_arr[] = 'There was a problem sending the email';
            }
        }
    }

    //If there are error, redirect back to the login form
    if(count($errmsg_arr)>0)
    {
	$_SESSION['ERRMSG_ARR'] = $errmsg_arr;
	session_write_close();
	header("location: reset.php");
	exit();
}

    //There were no errors
    header("location: reset-success.php");

?>

[Himself12794]

It all works except for the mysql_affected_rows() function.

I've found that you don't need to specify the query, because it just checks the last query. Thanks for all your help!

[Psycho]

Ah yes, it takes an options parameter for the resource link (i.e. the db connection variable) not the query identifier.