3
\$\begingroup\$

Please critique my login and signup validation php files.. login.php;

<?php
session_start();
require("connect.php");
$email = $_POST['emaillogin'];
$password = $_POST['passwordlogin'];
$email = mysql_real_escape_string($email);
$password = mysql_real_escape_string($password);

    if(empty($email))
    {
        die('{status:2,txt:"Enter your email address."}');
    }

    if(!filter_var($email, FILTER_VALIDATE_EMAIL))
    {
        die('{status:2,txt:"Invalid email or password"}');
    }

    if(empty($password))
    {
        die('{status:2,txt:"Enter your password."}');
    }

    if(strlen($password)<6 || strlen($password)>16)
    {
        die('{status:2,txt:"Invalid email or password"}');
    }

$query = "SELECT password, salt FROM users WHERE Email = '$email';";
$result = mysql_query($query);

    if(mysql_num_rows($result) < 1) //no such user exists
    {
        die('{status:2,txt:"Invalid email or password"}');
    }

$userData = mysql_fetch_array($result, MYSQL_ASSOC);
$hash = hash('sha256', $userData['salt'] . hash('sha256', $password) );

    if($hash != $userData['password']) //incorrect password
    {
        die('{status:2,txt:"Invalid email or password"}');
    }
////////////////////////////////////////////////////////////////////////////////////
    if('{status:3}')
{   
    session_regenerate_id (); //this is a security measure
    $getMemDetails = "SELECT * FROM users WHERE Email = '$email'";
    $link = mysql_query($getMemDetails);
    $member = mysql_fetch_row($link);
    $_SESSION['valid'] = 1;
    $_SESSION['userid'] = $member[0];
    $_SESSION['name'] = $member[1];
    session_write_close();
    mysql_close($con);  
    echo '{status:3,txt:"success.php"}';
}
?>

Signup and validation PHP

<?php
$name = $_POST['name'];
$surname = $_POST['surname'];
$email = $_POST['email'];
$remail = $_POST['remail'];
$gender = $_POST['gender'];
$bdate = $_POST['year'].'-'.$_POST['month'].'-'.$_POST['day'];
$bday = $_POST['day'];
$bmon = $_POST['month'];
$byear = $_POST['year'];
$cdate = date("Y-n-j");
$password = $_POST['password'];
$hash = hash('sha256', $password);
$regdate = date("Y-m-d");


function createSalt()
{
    $string = md5(uniqid(rand(), true));
    return substr($string, 0, 3);
}

$salt = createSalt();
$hash = hash('sha256', $salt . $hash);

    if(empty($name) || empty($surname) || empty($email) || empty($remail) || empty($password) )
{
    die('{status:0,txt:"All the fields are required"}');
}


    if(!preg_match('/^[A-Za-z\s ]+$/', $name))
{
    die('{status:0,txt:"Please check your name"}');
}


    if(!preg_match('/^[A-Za-z\s ]+$/', $surname))
{
    die('{status:0,txt:"Please check your last name"}');
}


    if($bdate > $cdate)
{
    die('{status:0,txt:"Please check your birthday"}');
}


    if(!(int)$gender)
{
    die('{status:0,txt:"You have to select your sex"}');
}


    if(!(int)$bday || !(int)$bmon || !(int)$byear)
{
    die('{status:0,txt:"You have to fill in your birthday"}');
}


    if(!$email == $remail)
{
    die('{status:0,txt:"Emails doesn&sbquo;t match"}');
}


    if(!filter_var($email, FILTER_VALIDATE_EMAIL))
{
    die('{status:0,txt:"Enter a valid email"}');
}


    if(strlen($password)<6 || strlen($password)>16)
{
    die('{status:0,txt:"Password must be between 6-16 characters"}');
}

    if (!$_POST["recaptcha_challenge_field"]===$_POST["recaptcha_response_field"]) 
{
    die('{status:0,txt:"You entered incorrect security code"}');
}


    if('{status:1}') 
{   
    require("connect.php");
    function getRealIpAddr()
    {
        if (!empty($_SERVER['HTTP_CLIENT_IP']))
        {
          $ip=$_SERVER['HTTP_CLIENT_IP'];
        }
        elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
        {
          $ip=$_SERVER['HTTP_X_FORWARDED_FOR'];
        }
        else
        {
          $ip=$_SERVER['REMOTE_ADDR'];
        }
        return $ip;
    }
    $rip = getRealIpAddr();
    $ipn = inet_pton($rip);
    $checkuser = mysql_query("SELECT Email FROM users WHERE Email = '$email'"); 
    $username_exist = mysql_num_rows($checkuser);
    if ( $username_exist !== 0 )
    {
        mysql_close($con);
    die('{status:0,txt:"This email Address is already registered!"}');
    }
    else
    {

    $query = "INSERT INTO users (name, surname, date, Email, Gender, password, salt, RegistrationDate, IP) VALUES ('$name', '$surname', '$bdate', '$email', '$gender', '$hash', '$salt', '$cdate', '$ipn')";
    $link = mysql_query($query);
    if(!$link)
        {
        die('Becerilemedi: ' . mysql_error());
        }
        else
        {
        mysql_close($con);  
        echo '{status:1,txt:"afterreg.php"}';
        }
    }
}

?>
\$\endgroup\$
3
  • \$\begingroup\$ really,only improvement i can see on that is using PDO for communicating with db \$\endgroup\$ Jun 25 '11 at 15:05
  • 2
    \$\begingroup\$ what the hell is this: if('{status:3}') \$\endgroup\$
    – RobertPitt
    Jun 29 '11 at 21:00
  • \$\begingroup\$ why don't you only bind the variable when the POST is not empty, e.g. if(empty($_POST['email'])) { die('{status:2,txt:"Enter your email address."}'); } else { $email = $_POST['email']; } \$\endgroup\$ Dec 29 '16 at 4:38
2
\$\begingroup\$

You should delimit your SQL field and table names because reserved words (e.g. date) can cause problems. Also I would use LIKE for something like an email address so it's not case-sensitive.

$query = "SELECT `password`, `salt` FROM `users` WHERE `Email` LIKE '$email';";

You might want to use a encryption package like phpass. Some people argue that it's much more secure to use an open source crypt framework, since it will be updated if problems are found with the code or cryptographic algorithms.

You might want to use mysqli functions rather than mysql, just because they are "improved". :-)

require is a language construct, not a function so you don't need the parentheses. They are ok here but can cause problems in some cases.

RobertPitt is correct: if('{status:3}') will always return TRUE.

$cdate = date("Y-n-j"); should probably use d for day w/ leading zeros.

if(!(int)$gender) casting to int is confusing and will require an additional conversion. I'd let PHP figure this out on its own: if(!$gender)

Good job escaping user input (from $_POST) in the first file; be sure to escape all user input used in SQL queries in both files.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.