Validation code tips

Question

if (isset($_POST['submit'])) {
 if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
 $errors = array();
 $error = 0;
 if (!preg_match('/^[A-Za-z](?=[A-Za-z0-9_.]{3,20}$)[a-zA-Z0-9_]*\.?[a-zA-Z0-9_]*$/i', $_POST['username'])) {
 $errors[] = 'Your username must start with a letter (A-Z) and be between 3-20 characters and may only contain alphanumeric characters (A-Z, 0-9 _ ) or a period (1) (".")';
 $error = 1;
 }
 $STH = $DBH->prepare("SELECT username FROM users WHERE username = ?");
 $STH->execute(array($_POST['username']));
 if ($STH->rowCount() > 0) {
 $errors[] = 'The username is already taken!';
 $error = 1;
 }
 if (strlen($_POST['password']) < 3) {
 $errors[] = 'Your password must be longer than 3 characters!';
 $error = 1;
 } else if ($_POST['password'] != $_POST['passconf']) {
 $errors[] = 'You didn\'t verify your password correctly!';
 $error = 1;
 }
 if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
 $errors[] = 'The e-mail is not valid!';
 $error = 1;
 } else {
 $STH = $DBH->prepare("SELECT email FROM users WHERE email = ?");
 $STH->execute(array($_POST['email']));
 if ($STH->rowCount() > 0) {
 $errors[] = 'The email is already taken!';
 $error = 1;
 }
 }
 }
}

This is my current validation for a registration page it works, but I'd like to improve it, give me some tips

• Xavier Combelle: I have some doubt on the username regex
• John: How could I improve it/change it? Why?
• Xavier Combelle: given some explaination in my answer

Answer 1

You have two long if blocks:

if (isset($_POST['submit'])) {
 if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
 ...
 }
}

Why not combine them?

if (isset($_POST['submit']) && (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token'])) {
 ...
}

also the way you've structured this, you don't need $error, to see if there are errors you can simply check empty($errors) (true => ok, false => not ok).

Other than that, your code looks fine.

• John: Thanks, but is really 2 queries needed for my username check and e-mail check? That's what I mainly had thoughts about
• John: The reason I didn't combine the If statements is because if someone mess with the token / or tries to submit a form from another place, I want to log that someone tried a CSRF attack. If I combine them, I would log everytime I don't submit the form and load the page :P
• yannis: @John Well you can get rid of username altogether, and use the email as username. But if you want them both, and they are supposed to be unique then yes you do have to have the two queries...
• John: Shouldn't I be able to query with a OR in the SQL statement, then try fetch the object or something?
• yannis: @John There are ways to combine the two queries. Easy ways where you won't know which of the two clauses failed (just that one of them did as a simple OR query) and more creative ways where it'd be possible to get all the info you want. But the two queries are extremely low cost, combining in any way won't have any distinguishable performance benefit and there are chances that you actually stress the db a little more. Anyways, if it's the sql you wan't reviewed, I'd suggest you add detailed table structures and their expected size on the question, too little info for a good answer as it is.

Answer 2

I think the correct regex for the login should be

if (!preg_match('/^[A-Za-z][A-Za-z0-9_.]{2,19}$/i', $_POST['username'])) {
 $errors[] = 'Your username must start with a letter (A-Z) and be between 3-20 characters and may only contain alphanumeric characters (A-Z, 0-9 _ ) or a period (1) (".")';
 $error = 1;
 }

EDIT I think that the current regexp allows more thant 30 letters