Page 1 of 1
Basic Form Validation
Posted: Sun Feb 18, 2007 1:44 pm
by thiscatis
Hello phpdn,
I`m using this code at the moment to validate my forms
Code: Select all
function validateFormInput() {
foreach ($_POST as $key => $input) {
strip_tags($input, '<a><b><i><u><li><div><style><script><br>');
if (strlen($input) < 2) {
$error = true;
echo "<div style=\"form_error\">Please correct the <b>$key</b> field.</div>";
}
}
}
Would you say this is enough for a basic validation or are there essential stuff that needs to be checked?
Posted: Sun Feb 18, 2007 1:46 pm
by feyd
The function doesn't actually do anything. $_POST is never altered.
Posted: Sun Feb 18, 2007 1:48 pm
by thiscatis
What do you mean?
Can't I use the error boolean in the php page the stop the form from being processed?
Posted: Sun Feb 18, 2007 2:09 pm
by thiscatis
err I just tested again, does exactly what I wants it to do, strip the html tags and checks the length...
Posted: Sun Feb 18, 2007 2:34 pm
by onion2k
Whether your validation is enough depends on what you want. If you want to check a field is longer than 1 character after stripping a few bits of HTML then what you have there is enough. I believe it's best to validate every form field based on an expected value. If the user is supposed to enter an email address then I validate the field against an email regular expression, if they're supposed to enter a URL I check the field contains a URL, if it's supposed to be a date then I check it's a valid date, and so on. I find it's best to make sure everything in the form is what it's meant to be simply because it stops clients phoning me up asking why their site looks wrong when they put a telephone number in the email section.
Posted: Sun Feb 18, 2007 3:07 pm
by Mordred
Explain this:
Code: Select all
strip_tags($input, '<a><b><i><u><li><div><style><script><br>');
It is dangerous, RTFM on strip_tags.
Generic validation, just like generic escaping (ala magic_quotes) is never a good idea, there is always a case when it doesn't work, so avoid doing it. Heed
onion2k.
Posted: Sun Feb 18, 2007 4:34 pm
by feyd
thiscatis wrote:err I just tested again, does exactly what I wants it to do, strip the html tags and checks the length...
Are you sure that it does it to $_POST?
Posted: Mon Feb 19, 2007 4:30 am
by Mordred
Ha, on second glance, it if isn't $something = strip_tags(...) it just does nothing. Even if it did, dangerous tags and attributes will still pass, leaving vectors for XSS attacks.
Posted: Mon Feb 19, 2007 4:47 am
by jito
If i'm not wrong, the point here is that thiscatis's function working on the temporary variable $input, not on the original POST variables. So it will not effect $_POST.
Posted: Mon Feb 19, 2007 5:15 am
by onion2k
It doesn't need to affect $_POST. The function is only validating the data, it's not doing anything with it. Quite frankly I think it's correct: validation functions shouldn't change any data. Leave the data cleansing to another function.
Posted: Mon Feb 19, 2007 7:50 am
by feyd
If the point of validation is simply checking, then
strip_tags() is not apart of it (not that it affects anything.) The only thing doing anything would then be the
strlen() check.. but where does $error go? If this is the entire function, which as far as has been posted it is, then it goes no where and does nothing as well. The only thing that would therefore happen as result is the function echoing some bits about fields needing corrections because they're less than two characters in length.
Posted: Mon Feb 19, 2007 9:46 am
by RobertGonzalez
I agree with feyd. The original function:
Code: Select all
<?php
function validateFormInput() {
foreach ($_POST as $key => $input) {
strip_tags($input, '<a><b><i><u><li><div><style><script><br>');
if (strlen($input) < 2) {
$error = true;
echo "<div style=\"form_error\">Please correct the <b>$key</b> field.</div>";
}
}
}
?>
Take the POST array and loops it's keys and values. It looks at a value, and after removing a handful of HTML tags, checks the length of the remaining string. If that is less than 2 chars long, it sets a var that it literally locked in the scope of the function (so it is doing nothing) then echo's out a statement to the browser.
It is just my opinion, but functions should return a value. In this case, you could make this function an is_ function and check boolean against it in your code.
Code: Select all
<?php
function is_post_data_valid() {
foreach ($_POST as $key => $input) {
strip_tags($input, '<a><b><i><u><li><div><style><script><br>');
if (strlen($input) < 2) {
return false;
}
}
return true;
}
// Usage
if (!is_post_data_valid()) {
// Handle error displays
}
// carry on
?>