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

Basic registration page and database insertion

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

Problem

Please review my registration code and suggest what changes I should make to improve security. I'm new to PHP and this is my first project.
Also I have no Idea how to make a "forget password page" as I've used password_hash to store passwords in database.


VALIDATION FORM - REGISTER

include 'register_db_check.php';
?>

DomainName.com - REGISTER

 Login  |  Register   |   Contact Us 

">

User Name : 

">
* 

Name : 
">
* 

Father Name : 
">
* 

E-mail : 
">
* 

Password : 
">
* 

Mobile Number : 
">
* 

Interests :
* 
  

Gender: 

 >Male

 >Female 
*  

Enter Captcha Here: 

OR

Click Below To Sign In From Your ID

    var frmvalidator = new Validator("myForm");
    frmvalidator.addValidation("uname","req","Please enter a Valid User Name");
    frmvalidator.addValidation("name","req","Please enter your First Name");
    frmvalidator.addValidation("fname","req","Please enter your Father Name");

    frmvalidator.addValidation("email","req","Please enter your Email");
    frmvalidator.addValidation("email","email","Please enter Valid Email");

    frmvalidator.addValidation("mnum","numeric","Only digits are allowed");

    chktestValidator.addValidation("sex","selone_radio","Select atleast one");


register_db_check.php


0) {
header ("Location:register.php?error=Email already exist.");
exit();
}
$sql = mysqli_query($conn, "SELECT * FROM
guests WHERE uname = '".$uname."'");
if(mysqli_num_rows($sql) > 0) {
/*print 'alert("Username already In Use!");'; //Prompts the user
print 'window.location.assign("register.php");';*/ //redirects to login.php
header ("Location:register.php?error=Username already exist.");
exit();
}

$result = mysqli_query($conn, "INSERT INTO
guests`(uname, name, fname, email, password, mnum, interest) VALUES ('$uname','$name','$fname','$email','$pass','$

Solution

Security

SQL Injection

Never put any variable data directly into SQL queries (the type of query doesn't matter, all can be vulnerable to SQL injection). This allows attackers to execute SQL queries themselves, with which they can see confidential database data. Depending on the db software and settings, they might also be able to write to files (which might gain them code execution), read sensitive files, execute system commands, etc.

Always use prepared statements.

XSS

Never echo variable data without sanitizing it. You do a good job here with htmlspecialchars($_SERVER["PHP_SELF"]), but not with the rest of the variables (eg $uname, $name, etc, all of which are also user supplied). This allows an attacker to execute arbitrary JavaScript in the context of the victims browser, which allows the stealing of cookies, key logging, phishing, and bypassing of CSRF (which means the attacker can do anything the victim can do).

Always use htmlspecialchars($string, ENT_QUOTES, 'UTF-8') whenever echoing a variable. Also note this list of places where this in not enough to prevent XSS.

Passwords

password_hash is the correct way of handling this. Regarding your question: A forgot password feature should NOT be sending the plaintext password to the user. Instead, a password reset link with a token which can be validated should be send to the users email address.

Captcha

You seem to validate the captcha after inserting the user into the database. This doesn't make any sense. The only thing the captcha protects is the redirection to /FvWithCaptcha/thankyou.php. But a user could just enter that path on their own.

Misc

  • your coding style makes it hard to read your code. Use proper indentation and be consistent with the location of your curly brackets. Any IDE will do this for you.



  • comments that just repeat what the code says are not helpful (eg If the values are posted, insert them into the database.). If you think that it's not obvious that this block of code does that, extract it to a well-named function (eg registerGuest).



  • some of your lines are too long. I would aim for 80 chars max.



  • !false is just true :)



  • Something like $securimage->check($_POST['captcha_code']) == !false can be simplified to $securimage->check($_POST['captcha_code']).

Context

StackExchange Code Review Q#92732, answer score: 6

Revisions (0)

No revisions yet.