Sanity check on form validation code

[David Mehler]

Hello,

I've got a form with various fields. One is a text input field called
name with a size and a maxlength of 30. I've got the following
validation code for this field. I'd appreciate feedback on it before I
do the others.

Thanks.
Dave.

$contact_page_errors = array();
$errorCount = "";
$name = "";

function test_input($data)
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);
return $data;
}

// Validate the name field
if (empty($_POST["name"]))
{
$contact_page_errors['name'] = "Name is Required";
$errorCount++;
}
else
{ // trims, strips slashes, and runs through htmlspecialchars
$name = test_input($_POST["name"]);
// Field should be at least two characters maximum of 30 and non-numeric
if (!strlen($name <= 2)) {
$contact_page_errors['name'] = "Name must have at least two characters\n";
$errorCount++;
}
if (strlen($name > 30)) {
$contact_page_errors['name'] = "Name can not have more than 30
characters\n";
$errorCount++;
}
if (is_numeric($name)) {
$contact_page_errors['name'] = "Name can not be numeric\n";
$errorCount++;
}
}
// check if name only contains letters and whitespace
if (!preg_match("/^[A-Z][a-zA-Z -]+$/",$name))
{
$contact_page_errors['name'] = "Name must be from letters,
dashes, spaces, first letter uppercase, and must not start

with dash.\n";
$errorCount++;
}
// Use php's filter_var to sanitize what's left
$name = filter_var($name, FILTER_SANITIZE_STRING);
} // end of name checks

[cipher]

Hi,

This looks good for validation, except for something that i want to mention!

The first thing is not a 'problem' : but

$errorCount = 0;

would have been great! (Instead of initializing it as if it was string)

On your `test_input` function:

The combination of `stripslashes` and `htmlspecialchars` (called in this
way) may increase your vulnerability on SQL injection (if you are using
this form to do tasks on the database).

Although the `filter_var` does some work, it is advisable to use
parameterized queries if you are planning to do some works on the database!

--
cipher
fb/twitter/github: nootanghimire

Post by David Mehler
Hello,
I've got a form with various fields. One is a text input field called
name with a size and a maxlength of 30. I've got the following
validation code for this field. I'd appreciate feedback on it before I
do the others.
Thanks.
Dave.
$contact_page_errors = array();
$errorCount = "";
$name = "";
function test_input($data)
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);
return $data;
}
// Validate the name field
if (empty($_POST["name"]))
{
$contact_page_errors['name'] = "Name is Required";
$errorCount++;
}
else
{ // trims, strips slashes, and runs through htmlspecialchars
$name = test_input($_POST["name"]);
// Field should be at least two characters maximum of 30 and non-numeric
if (!strlen($name <= 2)) {
$contact_page_errors['name'] = "Name must have at least two characters\n";
$errorCount++;
}
if (strlen($name > 30)) {
$contact_page_errors['name'] = "Name can not have more than 30
characters\n";
$errorCount++;
}
if (is_numeric($name)) {
$contact_page_errors['name'] = "Name can not be numeric\n";
$errorCount++;
}
}
// check if name only contains letters and whitespace
if (!preg_match("/^[A-Z][a-zA-Z -]+$/",$name))
{
$contact_page_errors['name'] = "Name must be from letters,
dashes, spaces, first letter uppercase, and must not start
with dash.\n";
$errorCount++;
}
// Use php's filter_var to sanitize what's left
$name = filter_var($name, FILTER_SANITIZE_STRING);
} // end of name checks

[Ashley Sheridan]

Just one thing really:

Your regex is only matching for ansi letters, so any letter with an
accent on won't be matched (poor Amélie!)

A better regex might be something like this:

if (!preg_match("/^[\p{L}][\p{L} '-]+$/u",$name))

There is more information on what the \p does at
http://php.net/manual/en/regexp.reference.unicode.php

I also added the apostrophe there too, as names like O'Leary are quite
common, and I assumed this was validation being used for persons names?


Thanks,
Ash
http://www.ashleysheridan.co.uk

Post by cipher
Hi,
This looks good for validation, except for something that i want to mention!
The first thing is not a 'problem' : but
$errorCount = 0;
would have been great! (Instead of initializing it as if it was string)
The combination of `stripslashes` and `htmlspecialchars` (called in this
way) may increase your vulnerability on SQL injection (if you are using
this form to do tasks on the database).
Although the `filter_var` does some work, it is advisable to use
parameterized queries if you are planning to do some works on the database!
--
cipher
fb/twitter/github: nootanghimire

Post by David Mehler
Hello,
I've got a form with various fields. One is a text input field called
name with a size and a maxlength of 30. I've got the following
validation code for this field. I'd appreciate feedback on it before I
do the others.
Thanks.
Dave.
$contact_page_errors = array();
$errorCount = "";
$name = "";
function test_input($data)
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);
return $data;
}
// Validate the name field
if (empty($_POST["name"]))
{
$contact_page_errors['name'] = "Name is Required";
$errorCount++;
}
else
{ // trims, strips slashes, and runs through htmlspecialchars
$name = test_input($_POST["name"]);
// Field should be at least two characters maximum of 30 and non-numeric
if (!strlen($name <= 2)) {
$contact_page_errors['name'] = "Name must have at least two characters\n";
$errorCount++;
}
if (strlen($name > 30)) {
$contact_page_errors['name'] = "Name can not have more than 30
characters\n";
$errorCount++;
}
if (is_numeric($name)) {
$contact_page_errors['name'] = "Name can not be numeric\n";
$errorCount++;
}
}
// check if name only contains letters and whitespace
if (!preg_match("/^[A-Z][a-zA-Z -]+$/",$name))
{
$contact_page_errors['name'] = "Name must be from letters,
dashes, spaces, first letter uppercase, and must not start
with dash.\n";
$errorCount++;
}
// Use php's filter_var to sanitize what's left
$name = filter_var($name, FILTER_SANITIZE_STRING);
} // end of name checks

[Jim Lucas]

Post by David Mehler
Hello,
I've got a form with various fields. One is a text input field called
name with a size and a maxlength of 30. I've got the following
validation code for this field. I'd appreciate feedback on it before I
do the others.
Thanks.
Dave.
$contact_page_errors = array();
$errorCount = "";
$name = "";
function test_input($data)
{
$data = trim($data);
$data = stripslashes($data);
$data = htmlspecialchars($data);
return $data;
}
// Validate the name field
if (empty($_POST["name"]))
{
$contact_page_errors['name'] = "Name is Required";
$errorCount++;
}
else
{ // trims, strips slashes, and runs through htmlspecialchars
$name = test_input($_POST["name"]);
// Field should be at least two characters maximum of 30 and non-numeric
if (!strlen($name <= 2)) {
$contact_page_errors['name'] = "Name must have at least two characters\n";
$errorCount++;
}
if (strlen($name > 30)) {
$contact_page_errors['name'] = "Name can not have more than 30
characters\n";
$errorCount++;
}
if (is_numeric($name)) {
$contact_page_errors['name'] = "Name can not be numeric\n";
$errorCount++;
}
}
// check if name only contains letters and whitespace
if (!preg_match("/^[A-Z][a-zA-Z -]+$/",$name))
{
$contact_page_errors['name'] = "Name must be from letters,
dashes, spaces, first letter uppercase, and must not start
with dash.\n";
$errorCount++;
}
// Use php's filter_var to sanitize what's left
$name = filter_var($name, FILTER_SANITIZE_STRING);
} // end of name checks

After cleaning up the code a little for readability, found a few errors.

See comments starting with ###

$contact_page_errors = array();
### if you plan to use it as a number, define is as one!
$errorCount = 0;
$name = "";

function test_input($data) {
$data = trim($data);

### Doing this without knowing if get_magic_quotes_gpc() was
### enabled, could do unexpected things to your data.
### So, do something like this instead...
if ( get_magic_quotes_gpc() )
$data = stripslashes($data);

$data = htmlspecialchars($data);
return $data;
}

// Validate the name field
if (empty($_POST["name"])) {
$contact_page_errors['name'] = "Name is Required";
$errorCount++;
} else {
// trims, strips slashes, and runs through htmlspecialchars
$name = test_input($_POST["name"]);

// Value: between 2 and 30 in length and non-numeric

### here you had (strlen($name <= 2))
if (strlen($name) <= 2) {
$contact_page_errors['name'] = "Name must have at least 2 characters";
$errorCount++;
}

### here you had (strlen($name > 30))
if (strlen($name) > 30) {
$contact_page_errors['name'] = "Name can not have more than 30 characters";
$errorCount++;
}
if (is_numeric($name)) {
$contact_page_errors['name'] = "Name can not be numeric";
$errorCount++;
}
}
// check if name only contains letters and whitespace
if ( !preg_match("/^[A-Z][a-zA-Z -]+$/", $name ) ) {
$contact_page_errors['name'] = "Name must be from letters, dashes, spaces,
first letter uppercase, and must not start with dash.";
$errorCount++;
}
// Use php's filter_var to sanitize what's left
$name = filter_var($name, FILTER_SANITIZE_STRING);
} // end of name checks

### Since you are trying to build an array of error messages,
### you should create them as a sub-array like the following
### example.

$contact_page_errors['name'][] = 'My error message';

### Then when you want to spit them out to the screen, do this

foreach ( $contact_page_errors AS $fname=>$fvalue )
echo "<div><h3>{$fname}</h3>\n<p>",
join("</p>\n<p>", $fvalue),
"</p></div>";