Question

Ok so my login script recognises a user that does not exist - its recognises when the password is incorrect - it moves forward when the correct information is submitted but not to the correct page despite me telling it too...

login form:

<form name="login" action="login.php" method="post">
    Username: <input type="text" name="username" />
    Password: <input type="password" name="password" />
    <input type="submit" value="Login" />
</form>

Login script:

<?
session_start(); //must call session_start before using any $_SESSION variables
$username = $_POST['username'];
$password = $_POST['password'];

$dbhost = 'localhost';
$dbname = '******';
$dbuser = '******';
$dbpass = '******'; //not really
$conn = mysql_connect($dbhost, $dbuser, $dbpass);
mysql_select_db($dbname, $conn);


$username = mysql_real_escape_string($username);
$query = "SELECT password, salt
        FROM users
        WHERE username = '$username';";
$result = mysql_query($query);
if(mysql_num_rows($result) < 1) //no such user exists
{
    header('Location: login_form.php');
    die();
}
$userData = mysql_fetch_array($result, MYSQL_ASSOC);
$hash = hash('sha256', $userData['salt'] . hash('sha256', $password) );
if($hash != $userData['password']) //incorrect password
{
    header('Location: login_form.php');
    die();
}
else
{
    validateUser(); //sets the session data for this user
}


header('Location: membersonly.php');


?>
Was it helpful?

Solution

So I rewrote your code a little bit. I did a few things:

  1. I converted your MySQL to MySQLi. MySQL is depreciated and will be removed. I used OOP style MySQLi, but you can do procedural style.
  2. I changed your query string. Sometimes SQL can get picky about formatting and sql really likes when you use the '`' character.
  3. Next I separated out your large hash statement. This was for readability.
  4. I also added an "isset" check for the salt and the password. This check just confirms that these values actually exist.
  5. I've added the "error" variable for "login_form.php" I would remove it at release, but it gives you as the programmer a good chance to see where your code seems to be going wrong.
  6. I've now also added "true" and "303" to your header area where you set the location. true is a boolean about whether or not it should override a previous set header value for Location. 303 tells the browser that it should "see other"
  7. Finally I added some extra headers designed to remove any chance of caching. While I do not know of any browsers that would cache your page that requests a redirect, it could happen seeing as it's all about how the browser handles it.

    $username = $_POST['username'];
    $password = $_POST['password'];
    
    $dbhost = 'localhost';
    $dbname = '******';
    $dbuser = '******';
    $dbpass = '******'; //not really
    
    $sql_connection = new mysqli($dbhost, $dbuser, $dbpass, $dbname);
    
    $username = mysql_real_escape_string($username);
    $query = "SELECT password, salt FROM users WHERE username = '$username';";
    $result = $sql_connection->query($query);
    if($result->num_rows != 1) //no such user exists{
        header("Location: login_form.php");
        die();
    }
    $userData = $result->fetch_assoc();
    
    if(!isset($userData['salt']) || !isset($userData['password'])){
        header("Location: login_form.php?error=MissingInformation");
        die();
    }
    
    $salt = $userData['salt'];
    $password_hash = hash('sha256', $password);
    $hash = hash('sha256', $salt . $password_hash);
    $correct_password = $userData['password'];
    if($hash != $correct_password){ // incorrect
        header('Location: login_form.php');
        die();
    }else{
        validateUser(); //sets the session data for this user
    }
    
    header("Location: membersonly.php", true, 303);
    

Hope it helps!

Edit: I would also recommend removing the redirect and looking at the result to make sure it's not returning a warning. If it's just a warning, your script will still execute and it will redirect, but you'll never see the error.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top