HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

User register script PHP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptuserregisterphp

Problem

So, I needed to make me a register script for my website so pepole can register, and just wanted to know if It's okey.
So, is there any vulns in this script i made?It works perfectly, just that i wanted to have someone to look over it here.
Or if there could be some inprovments that could make it better.

``
connect_error){ echo $Sql->connect_error; }
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));
if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }
$CheckUser = $Sql->query("SELECT Username FROM
users WHERE Username = '".$sUser."'");
$CheckMail = $Sql->query("SELECT
E-Mail FROM users WHERE E-Mail = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }
$xCode = GenerateCode();
$Insert = "
INSERT INTO
teamgamersnet.users (ID, Username, Password, E-Mail, IP, Activated, ActivateId`)
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');
";
$Create = $Sql->query($Insert);
if (!$Create) { echo "S**t, a error happened D:";}
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
Thank you for registering at Team Gamers Net
Just one more step and that is to activate your account
You registred with username '.$Username.'
And your password you selected (not shown)
To activate and contiune, please click HERE
';
$headers = "From: DoNotReply@TeamGamers.Net \r\n";
$headers .= "Reply-To: Admin@heisteknikk.com \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-88

Solution

Ok, there are a couple of things wrong with your code and your method of organizing the code. I've pointed them out to you in the code below within comments. The comments preceded with # are my comments.

connect_error){ echo $Sql->connect_error; }

/* Let's add a comment here: Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

# Let's move this to the top, right after these are set from _POST, to avoid all the 
# prior computation before running into this. Also, use empty() or isset() instead of ! checking
# See: http://www.phpbench.com/
#OLD::
#if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }

#NEW::
# Moved to top, also - syntax errors fixed: it's || not | 

/* Let's add a comment here: Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Let's add a comment here: Generate an activation code */
$xCode = GenerateCode();

/* Let's add a comment here: Write out the Insert statement  */ 
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Let's add a comment here: Insert the SQL statement */
$Create = $Sql->query($Insert);
# Why are we echoing? Shouldn't we be dying?
#OLD::
#if (!$Create) { echo "S**t, a error happened D:";}

#NEW::
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Let's add a comment here: Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
Thank you for registering at Team Gamers Net
Just one more step and that is to activate your account
You registred with username '.$Username.'
And your password you selected (not shown)
To activate and contiune, please click HERE
';
$headers = "From: DoNotReply@TeamGamers.Net \r\n";
$headers .= "Reply-To: Admin@heisteknikk.com \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");    
?>


Here's the same code, with my comments removed:

connect_error){ echo $Sql->connect_error; }

/* Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

/* Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Generate an activation code */
$xCode = GenerateCode();

/* Write out the Insert statement  */   
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Insert the SQL statement */
$Create = $Sql->query($Insert);

/* Check if SQL insert went well, otherwise die */
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
Thank you for registering at Team Gamers Net
Just one more step and that is to activate your account
You registred with username '.$Username.'
And your password you selected (not shown)
To activate and contiune, please click HERE
';
$headers = "From: DoNotReply@TeamGamers.Net \r\n";
$headers .= "Reply-To: Admin@heisteknikk.com \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");    
?>

Code Snippets

<?php

/* Mysql data */
$MysqlUsername = "root";
$MysqlPassword = "**********";
$MysqlHostname = "localhost";
$MysqlDatabase = "teamgamersnet";

$Username = $_POST['Username'];
$Password = $_POST['Password'];
$Email    = $_POST['Email'];

# Check for empty variables 
if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }

# We're only using $IP once, so why set it above? Let's do a direct comparison here
// Beta, Restrict users from using it until it's done
#OLD:: 
#if ($IP != "84.***.***.***") { die(); }

#NEW::
if ($_SERVER['REMOTE_ADDR'] != "84.***.***.**") { die(); } 

/* Random Activate code generator */
function GenerateCode() {
    # You're setting 7 variables with like variables, and returning them
    #OLD::
    #$R1 = rand(1,9);
    #$R2 = rand(1,9);
    #$R3 = rand(1,9);
    #$R4 = rand(1,9);
    #$R5 = rand(1,9);
    #$R6 = rand(1,9);
    #$C1 = "$R1$R2$R3$R4$R5$R6";
    #return $C1;

    #NEW::
    return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
}

/* Encrypt the password */
function Encrypt($Pass) {
    # Again, setting multiple variables in the name of readability over performance
    #OLD::
    #$B64 = base64_encode($Pass);
    #$MD5 = md5($B64);
    #$Hash = base64_encode($MD5);
    #return $Hash;

    #NEW::
    return base64_encode( md5( base64_encode($Pass) ) );
}


# Let's move the Mysql Connection Data to the top, so its easier to modify in the future
#OLD::
/* Mysql data */
#$MysqlUsername = "root";
#$MysqlPassword = "**********";
#$MysqlHostname = "localhost";
#$MysqlDatabase = "teamgamersnet";

#NEW::
# Moved to top

$Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
if ($Sql->connect_error){ echo $Sql->connect_error; }

/* Let's add a comment here: Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

# Let's move this to the top, right after these are set from _POST, to avoid all the 
# prior computation before running into this. Also, use empty() or isset() instead of ! checking
# See: http://www.phpbench.com/
#OLD::
#if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }

#NEW::
# Moved to top, also - syntax errors fixed: it's || not | 

/* Let's add a comment here: Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Let's add a comment here: Generate an activation code */
$xCode = GenerateCode();

/* Let's add a comment here: Write out the Insert statement  */ 
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mai
<?php

/* Mysql connection data */
$MysqlUsername = "root";
$MysqlPassword = "**********";
$MysqlHostname = "localhost";
$MysqlDatabase = "teamgamersnet";

$Username = $_POST['Username'];
$Password = $_POST['Password'];
$Email    = $_POST['Email'];

/* Check for empty variables */ 
if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }

/* Beta, Restrict users from using it until it's done */
if ($_SERVER['REMOTE_ADDR'] != "84.***.***.**") { die(); } 

/* Random Activate code generator */
function GenerateCode() {
    return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
}

/* Encrypt the password */
function Encrypt($Pass) { return base64_encode( md5( base64_encode($Pass) ) ); }

$Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
if ($Sql->connect_error){ echo $Sql->connect_error; }

/* Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

/* Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Generate an activation code */
$xCode = GenerateCode();

/* Write out the Insert statement  */   
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Insert the SQL statement */
$Create = $Sql->query($Insert);

/* Check if SQL insert went well, otherwise die */
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
<h3>Thank you for registering at Team Gamers Net</h3>
<p>Just one more step and that is to activate your account</p>
<p>You registred with username '.$Username.'<p>
<p>And your password you selected (not shown)</p>
<p>To activate and contiune, please click <a href="http://TeamGamers.net/Activate.php"><b>HERE</b></a></p>
';
$headers = "From: DoNotReply@TeamGamers.Net \r\n";
$headers .= "Reply-To: Admin@heisteknikk.com \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");    
?>

Context

StackExchange Code Review Q#16246, answer score: 5

Revisions (0)

No revisions yet.