patternphpMinor
Basic registration page and database insertion
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
register_db_check.php
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
Always use
Passwords
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
Misc
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 (egregisterGuest).
- some of your lines are too long. I would aim for 80 chars max.
!falseis justtrue:)
- Something like
$securimage->check($_POST['captcha_code']) == !falsecan be simplified to$securimage->check($_POST['captcha_code']).
Context
StackExchange Code Review Q#92732, answer score: 6
Revisions (0)
No revisions yet.