patternphpMinor
User register script PHP
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.
``
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
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
Here's the same code, with my comments removed:
# 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.