Two implementations of a PHP sign-up page

Question

I want the most secure and well-written version for my sign-up page. I'm adding AJAX to it for validation, too. I'm still writing it.

If you have tips on AJAX or on how to improve this PHP code for AJAX, feel free to point out any mistake and make any modification you want, or if there are other security concerns I should worry about. I'm fairly new this is the most I know about how to fight SQL injection.

I wrote this version first after I learned about SQL injection:

if (isset($_POST['submit'])){
include_once 'dbh.inc.php';
 $username = mysqli_real_escape_string($conn, $_POST['username']);
 $pwd = mysqli_real_escape_string($conn, $_POST['pwd']);
 $email = mysqli_real_escape_string($conn, $_POST['email']);
if(empty($username) || empty($pwd) || empty($email)){
 echo "Fill in all Fields!";
}else {
 if(!filter_var($email, FILTER_VALIDATE_EMAIL)){
 echo "Email is invalid!";
 }else{
 $stmt = $conn->prepare("SELECT * FROM users WHERE username=?");
 $stmt->bind_param("s", $uid);
 $uid = $username;
 $stmt->execute();
 $result = $stmt->get_result();
 $usernamecheck = mysqli_num_rows($result);
 $rowNum = $result->num_rows;
 if($rowNum > 0){
 echo "Username is taken!";
 }else{
 $stmt = $conn->prepare("SELECT * FROM users WHERE email=?");
 $stmt->bind_param("s", $uemail);
 $uemail = $email;
 $stmt->execute();
 $result = $stmt->get_result();
 $usernamecheck = mysqli_num_rows($result); 
 $rowNum = $result->num_rows;
 if($rowNum > 0){
 echo "Email is taken";
 }else{
 $hashedPwd = password_hash($pwd, PASSWORD_DEFAULT);
 $stmt = $conn->prepare("INSERT INTO users (username, pwd, email) VALUES (?, ?, ?)");
 $stmt->bind_param("sss",$uid, $password, $theemail);
 $uid = $username;
 $password = $hashedPwd;
 $theemail= $email;
 $stmt->execute();
 $result = $stmt->get_result();
 header("location: ../user-login.php");
 }
 }
 }
 }
 }else{
header("location: ../user-signup.php");
exit();
}

and this version, which I was told is a better version, but has no prepared statements, which i learned is the main thing to fight SQL injection:

include_once '../dbh.inc.php';
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
$username = $_POST['username'];
$email = $_POST['email'];
$pwd = $_POST['pwd'];
if(empty($username) || empty($pwd) || empty($email)){
 echo "Fill in all Fields!";
 exit();
}elseif(!filter_var($email, FILTER_VALIDATE_EMAIL)){
 echo "Email is invalid!";
 exit();
 }else{
$chk = "SELECT * FROM user WHERE email='$email'";
$result = mysqli_query($conn, $chk);
while ($row = mysqli_fetch_array($result)) {
 if ($row['email'] == $email) {
 echo "Email Match";
 exit();
 } else {
 $chk = "SELECT * FROM user WHERE username='$username'";
 $result = mysqli_query($conn, $chk);
 while ($row = mysqli_fetch_array($result)) {
 if ($row['username'] == $username) {
 echo "Username Match";
 exit();
 } else {
$hashedPwd = password_hash($pwd, PASSWORD_DEFAULT);
$join = sprintf("INSERT INTO user(username, email, password) " .
 "VALUES ('%s', '%s', '%s', '%s'); ",
 mysqli_real_escape_string($conn, $username),
 mysqli_real_escape_string($conn, $email),
 mysqli_real_escape_string($conn, $hashedPwd),
 mysqli_insert_id($conn));
 if (mysqli_query($conn, $join)) {
 $i = "joined";
 } else {
 $i = "failed to join";
 }
 echo $i;
 }

• Mast: Did you write the second version as well?
• Mast: What does this have to do with jQuery?

Answer 1

Both examples show really poor coding style which honestly makes it hard to give a good evaluation or to come to some A vs. B decision on which code is better, as both pieces of code have significant problems.

• You have a lot of unnecessary code nesting. You should think about inverting conditions, designing away ELSE conditionals wherever possible, and providing early exits. This minimizes number of code paths/complexity and makes your code easier to read, maintain, and debug.

For example:

if (!isset($_POST['submit'])) {
 header("location: ../user-signup.php");
 exit();
}
// rest of code, now without nesting

and

if(!filter_var($email, FILTER_VALIDATE_EMAIL)){
 echo "Email is invalid!";
 exit();
}
// rest of code, now without nesting

• Will your code fail if the DB file is not able to be included? If so, this should be require_once instead of include_once. You should never continue code execution if a key dependency is unavailable. Let the fatal error happen. Then the fix is more readily apparent.
• It makes no sense to start escaping strings for insert before you have even validated the input (though I would agree that prepared statements is generally the better way to go anyway).
• Why bother to perform SELECT to see if username/email already exists? Why not have unique indexes on username and email in your DB and go straight to INSERT step, checking the query result to see if insert was successful or whether unique constraint was violated. This will make it such that you only do a single query on happy path use case vs. doing three queries on happy path use case the way you are currently do it.
• Should username and email really both be unique constraints? It seems very odd to use both fields as unique identifiers for a user record. I would suggest one of them should be authoritative.
• I generally think that your pattern of directly echoing out error conditions is going to be a coding pattern that, as you develop your expertise, will be one you want to move away from.
• Your database handling code really only considers the happy path. What if statement preparation fails? What if the insert fails?
• If $conn contains a mysqli object, and you are working in OO context (which is good), why are you also using procedural mysqli like mysqli_real_escape_string()? You should be using $conn->escape_string() (though again, I would suggest prepared statements and eliminating this altogether).
• On insert, there is no reason to call get_result() on statement object after execution. The return of $stmt->execute() is a boolean indicating success/failure.
• Is the string being empty or not really the only validation you need to perform on username and password? At a very minimum, I would guess that your DB field configuration would prescribe a maximum length. Do you really want to query the database when a user passes you a 5000 character long username (potentially silently truncating the username in the process)? Do you really want to allow usernames or passwords that are a single character?
• In general, I would suggest that you continue to look at more elegant ways to validate your input without having to get into a bunch of nested conditionals. filter_input_array() is a pretty useful tool, but there are other common PHP validation libraries (especially in popular PHP frameworks) that can allow you to better manage validation configuration and execution.
• Why add unnecessary variables after you perform statement binding?
• Stylistically there are lots of concerns:
  • indentation is really off
  • you have no or inconsistent spacing around flow control characters
  • you have inconsistent spacing around assignment operators
  • you have an odd mix of camelCase (which is OK) and nocase symbols. Shouldn't $usernamecheck be $usernameCheck?
  • I would highly recommend that you adopt a coding style (preferably PSR-1/PSR-2 compliant) and stick with it. Use stylechecking in your IDE to help you enforce it.
• The second piece of code is actually broken in that you insert will never work since the query have more values being listed than there are names.

Putting it all together:

if(!isset($_POST['submit'])) {
 header("location: ../user-signup.php");
 exit();
}
// input validation
$validations = [
 'username' => [
 'filter' => FILTER_VALIDATE_REGEXP,
 'options' => ['regexp' => '/yourregex/']
 ],
 'pwd' => [
 'filter' => FILTER_VALIDATE_REGEXP,
 'options' => ['regexp' => '/yourregex/']
 ],
 'email' => FILTER_VALIDATE_EMAIL
];
$inputs = filter_input_array(INPUT_POST, $validations);
$errors = [];
foreach($inputs as $key => $value) {
 if(!$value)) {
 $errors[] = $key;
 }
 $$key = $value;
} 
if(count($errors) > 0) {
 // perhaps include form again
 // and/or give error messages based on field name values in $errors
 exit();
}
// insert in DB
require_once 'dbh.inc.php';
$query = 'INSERT INTO users (username, pwd, email) VALUES (?, ?, ?)';
$stmt = $conn->prepare($query);
if($stmt instanceof mysqli_stmt === false) {
 // perhaps add 500 series header
 // some error message or redirect
 exit();
}
$pwdHash = password_hash($pwd);
$result = $stmt->bind_param('sss', $username, $pwdHash, $email);
if ($result === false) {
 // perhaps add 500 series header
 // some error message or redirect
 exit();
}
$result = $stmt->execute();
if ($result === false) {
 if($stmt->errno === 1169) {
 // we have unique constraint violation
 // you may or may not want to give user message about this
 // i..e to change username
 } else {
 // some other error
 }
 $stmt->close();
 exit();
}
// success
$stmt->close();
header("location: ../user-login.php");

Answer 2

• (削除) Escape the hashed password, don't hash the escaped password. (削除ここまで)... just hash the pw.
• Prefer prepared statements over escaping.
• Break up the logic into functions.
• Please indent better - your formatting is a bit of a mess.
• You do not need to use both escaping and prepared statements. Pick one. (Pick prepared statements)

---

Edit:

In this case, if your IDs are integers, you don't need to escape them or use prepared statements. Just check if they are integers first with is_numeric(). If you know they're integers then you know they're safe.

Generally though, prepared statements are the way to go.

• Qirel: A password should only be hashed, never escaped, as that would change the hashed result.
• I wrestled a bear once.: If its hashed there should be nothing left to escape. Escaping a hashed pw should have no effect
• I wrestled a bear once.: Hashing an escaped password o the other hand will change the escaped password.. hence the first bullet
• I wrestled a bear once.: Although.. If u were to hash the escaped pw every time the software would never know the differencw
• Qirel: Your first comment, "Escape the hashed password, don't hash the escaped password.", suggests that you should in fact escape it (after hashing) - which isn't needed at all ;-) it should be hashed and left alone until insertion.