Working on Email Form for 2 Days - Simple Error Checking

Any help is appreciated - Error messages won't appear

[timothius]

Hey all. I've been working on this form for my brother's website for days now and I'm a bit out of sorts. I'm just a little better than a PHP newbie, and I need to get this working as soon as I can. Any help is GREATLY appreciated!

Ok, so the form itself is simple:

<form name="phpformmailer" method="post" action="<?php $_SERVER['PHP_SELF']?>">
<label>Your Name: (required)</label>
<br/>
<input id="textName" type="text" name="name"><br/>
<label>Your Email: (required)</label>
<br />
<input id="textEmail" type="text" name="email"><br/> 
<label>Your Message:</label>
<br/> 
<textarea name="themessage" cols="30" rows="5"></textarea><br/>
<input id="submitButton" type="image" name="B1" src="/images/submit.jpg" width="430" height="28" value="Submit" >

<input type="hidden" name="stage" value="process">
</form>

Further up the page I have all the PHP code. This appears before anything else on the page:

<?php
ini_set('session.use_only_cookies',1);
session_start();
?>

Then I have HTML stuff, and then:

<?php
global $error_message;
$error_message = '';

session_register("SESSION");

$showBox = FALSE;

if (isset($_POST['stage']) && ($_POST['stage'] == 'process')) {

$showBox = TRUE;

//check user input for possible header injection attempts!
function is_forbidden($str,$check_all_patterns = true)
{
 $patterns[0] = '/content-type:/';
 $patterns[1] = '/mime-version/';
 $patterns[2] = '/multipart/';
 $patterns[3] = '/Content-Transfer-Encoding/';
 $patterns[4] = '/to:/';
 $patterns[5] = '/cc:/';
 $patterns[6] = '/bcc:/';
 $forbidden = 0;
 for ($i=0; $i<count($patterns); $i++)
 {
 $forbidden = preg_match($patterns[$i], strtolower($str));
 if ($forbidden) break;
 }
//check for line breaks if checking all patterns
 if ($check_all_patterns AND !$forbidden) $forbidden = preg_match("/(%0a|%0d|\\n+|\\r+)/i", $str);
 if ($forbidden)
 {
 $error_message = 'Forbidden Text';
 }
}

// ------------------------- EXECUTION OF FUNCTION ABOVE -------------------------

foreach ($_REQUEST as $key => $value) //check all input
{
 if ($key == "themessage") is_forbidden($value, false); //check input except for line breaks
 else is_forbidden($value);//check all
}

// ------------------------- CREATE EMAILS -------------------------

$replyemail="[e-mail]";

$name = $_POST["name"];
$email = $_POST["email"];
$thesubject = "Email from Example.com";
$receipt_subject = "message to Example.com";
$themessage = $_POST["themessage"];

$success_sent_msg='Your email has been successfully sent! You will receive a reply as soon as possible. For your convenience, a copy of your message has been sent to you. Thank-you for contacting example.com!';

$replymessage = "Thank you for your email!

Your message has been recevied and will be replied to shortly.

Please DO NOT reply to this email.


Below is a copy of the message you submitted:
--------------------------------------------------
Message: $themessage
--------------------------------------------------

Thank you,
#*$!#*$!X #*$!XX
www.example.com";

$themessage = "From: $name \nMessage: $themessage";



// ------------------------- PROCESSING -------------------------

if (!session_is_registered("SESSION")){
 $errors_message = "Invalid form submission";
} elseif (!$error_message) {
mail("$replyemail",
 "$thesubject",
 "$themessage",
 "From: $email\nReply-To: $email");
mail("$email",
 "Receipt of $receipt_subject",
 "$replymessage",
 "From: $replyemail\nReply-To: $replyemail");
} else $error_message = "Failed to send message.";
}
?>


<?php
// ------------------------- FEEDBACK BOX -------------------------
?>

<?php if ($showBox == TRUE) { ?>

<?php if ($error_message) {?>
<div id="errorBox"><p><?php echo $error_message; ?></p></div>;
<?php } else ?>
<div id="successBox"><p><?php echo $success_sent_msg; ?></p></div>;
<?php echo $error_message;?>

<?php } ?>

Explanations:

So, the code might be ugly (the ugly parts would be me, the clever parts are from the script that thedemosite.co.uk offers.

Basically, it creates a function that checks the form input against an array of forbidden strings, and then just the text inputs for line breaks.

I'm trying to get the $error_message variable as an indicator of whether the form will mail off or not, and whether it will show a error dialogue box that I setup further down the page. So far I'm not getting the $error_message variable to fill up. I don't know why.

ANY SUGGESTIONS on security and on how to make this form better is GREATLY appreciated. Note that I will do data validation later. I'm just working on one thing at a time.

[rocknbil]

The short story (pretty sure) is you've declared $error_message as a global outside the function, but it needs to be declared inside the function as well.

I suggest you can eliminate global usage of $error_string by returning the value from your function. Look at this simplified example to see how.

if ($key == "themessage") { $error_message = is_forbidden($value, false); } //check input except for line breaks
else { $error_message = is_forbidden($value); }//check all

Then in your function,

function is_forbidden($str,$check_all_patterns = true)
{
$error_message=null;
// the rest of your code as you have it, then
return $error_message;
}

Since "$error_message" is localized to this function, it won't stomp over any other instances of $error_message outside the function - **if** you remove

global $error_message;

[timothius]

Thank-you for your reply rocknbill, however I made the changes you suggested and still $error_message refuses to become TRUE. I tried with the $error_message variable being both global and not.

I thought I was doing the same thing as you were suggesting when I made $error_message a GLOBAL variable. Still a little confused on that front, but regardless, it still isn't functioning correctly, so there must be something else wrong?

Oh, and may I ask why $error_message was set to null inside the function?

And please, any other suggestions to make this work better is appreciated!

[Matthew1980]

Hi there timothius,

I'm not going to tread over rocknbils toes here, but I will point out something that I always find from new programmers:-

<form name="phpformmailer" method="post" action="<?php $_SERVER['PHP_SELF']?>">

As it stands this line wouldn't do anything as your missing the semi colon from the end of 'PHP_SELF']; ?> I'm not sure that this would generate an error, but it's certainly not good practice to do this.

$_SERVER['PHP_SELF']; in it's own right should be avoided if at all possible, I assume from reading this that you're posting/submitting this form to itself? That's good, a form's default action is to post/submit to itself, so in theory you can leave the action="" blank, or put the name of the file the form is contained in there instead.

Three things to know about form's:-

1) If not defined in the action="" attribute, a form defaults to submitting to itself.
2) If not explicitly set in method="POST" a form defaults to GET
3) When submitting data either through $_POST or $_GET, data type is always a string - something to remember if your doing pagination

Lastly, setting that variable to NULL inside the functions parameter list means that your declaring the variable, but it will have no state, therefore wouldn't generate undefined index error's when the script is run - it's kind of like stating it's an optional variable, use it if it's there, ignore it if its not...

Hope that helps..

Cheers,
MRb

[timothius]

Thank-you for your reply and information Matthew1980. I didn't know some of those things.

I still have to say that I am really struggling with this error-checking part of the code. It is totally not behaving like I expect it to.

The goal of the code below is to have $error_message set if there is anything in the form's input that is forbidden.

Please help. I am so clueless as to why the email always sends, even if I enter illegal text. Can someone please point out my logic mistake(s) here?

THANKS!

//check user input for possible header injection attempts!
function is_forbidden($str,$check_all_patterns = true)
{
 $patterns[0] = '/content-type:/';
 $patterns[1] = '/mime-version/';
 $patterns[2] = '/multipart/';
 $patterns[3] = '/Content-Transfer-Encoding/';
 $patterns[4] = '/to:/';
 $patterns[5] = '/cc:/';
 $patterns[6] = '/bcc:/';
 $forbidden = 0;
 for ($i=0; $i<count($patterns); $i++)
 {
  $forbidden = preg_match($patterns[$i], strtolower($str));
  if ($forbidden) break;
 }
//check for line breaks if checking all patterns
 if ($check_all_patterns AND !$forbidden) $forbidden = preg_match("/(%0a|%0d|\\n+|\\r+)/i", $str);
 if ($forbidden)
 {
 $error_message = null;
 $error_message = 'Forbidden Text';
// echo $error_message;
 return $error_message;

 }
}

// ---------------- EXECUTION OF FUNCTION ABOVE ------------------

foreach ($_REQUEST as $key => $value) //check all input
{
 if ($key == "themessage") {
 $error_message = is_forbidden($value, false); //check input except for line breaks
echo $error_message; }
 else $error_message = is_forbidden($value); echo $error_message;//check all
}

// SKIP CREATE EMAIL PART

// ------------------------- PROCESSING -------------------------

if (!session_is_registered("SESSION")){
  $error_message = "Invalid form submission";
} elseif (!$error_message) {
mail("$replyemail",
   "$thesubject",
   "$themessage",
   "From: $email\nReply-To: $email");
mail("$email",
   "Receipt of $receipt_subject",
   "$replymessage",
   "From: $replyemail\nReply-To: $replyemail");
} else $error_message = "Failed to send message.";
}
?>


<?php
// ------------------------- FEEDBACK BOX -------------------------
?>

<?php if ($showBox == TRUE) { ?>

<?php if ($error_message) {?>
<div id="errorBox"><p><?php echo $error_message; ?></p></div>;
<?php } else ?>
<div id="successBox"><p><?php echo $success_sent_msg; ?></p></div>;
<?php echo $error_message;?>

<?php } ?>

[rocknbil]

I'm not going to tread over rocknbils toes here

You walk on the tops, I'll walk on the bottoms :-P. Absolutely, SELF has vulnerabilities, but "one problem at a time" is why I didn't mention it.

I am so clueless as to why the email always sends, even if I enter illegal text.

This is what's called (well, what I call it) linear programming: your script executes top to bottom. After your error checking, there's nothing to stop the script and exit if errors are found. This is why it sends it anyway.

still $error_message refuses to become TRUE. ... Still a little confused on that front, but regardless, it still isn't functioning correctly ... may I ask why $error_message was set to null inside the function?

In my simplified example, the error message won't return "true", it will return either null or a string value (which probably answers the second question . . . )

You might want to get your head around scope and the way functions work. A function is like a closed unit. It's only aware of the conditions set inside it **except** input parameters you send to it and the values returned from it - and this includes any variables you set. $error_message outside a function is not "aware" of $error_message inside a function, and vice versa. This in itself is a powerful concept: I have this black box, I send it something, it gives a result. Nothing I do anywhere else in my program is affected by what goes on in that function. This is (one reason) why globals are discouraged, they change the rules.

So given that concept, my function will either return null or a **helpful** error string, I can now send it data and if I get nothing back, the data passes.

$errs = check_my_data($some_data);
if ($errs) {
output_the_form('Error Title',$errs);
}
else {
on_our_merry_way();
}

If $errs is null, the if construct will return false when we ask "if ($errs)"; If there **are** errors, it passes our "helpful error string" on to the output_the_form function with the error string as a second optional parameter.

function output_the_form($formtitle,$errors=null) {

What that says is the $formtitle is required for this function to work, but the $errors parameter is optional - allowing you to use output_the_form for both the initial load of the form **and** in the event of an error.

This kind of thinking builds better user interfaces too - the above scenario is far better than a silly "errors, go back" or a redirect requiring usage of sessions to maintain form values (which often fails if cookies are disabled.)

A simplified revised version of my previous code would be **something like** this.

if ($key == "themessage") { $error_message = is_forbidden($value, false); } //check input except for line breaks
else { $error_message = is_forbidden($value); }//check if ($error_message) { echo $error_message; exit; }

See the exit? Stop the program there. That's a bit cheesy, you get a white page with an error on it, but it may help you understand what's up.

[timothius]

Thanks rocknbil, the original program actually had an exit(), but changed it to set the value of the $error_message instead.

After a lot of staring at the screen over the weekend, I figured out that the problem I had mainly lay in the last part that you had mentioned, and I changed it to:

 if ($forbidden)
 {
 $error_flag = null;
 $error_flag = 'You have entered text that is not permitted in the form.';
 return $error_flag;
 }
}

// ------------------------- EXECUTION OF FUNCTION ABOVE 

foreach ($_REQUEST as $key => $value) //check all input
{
 if ($key == "themessage") {
 $error_message = is_forbidden($value, false); //check input except for line breaks
if ($error_message) break;
}
 else $error_message = is_forbidden($value); { //check all
 if ($error_message) break;} 
}

Thank-you both very much for your help!