Jump to content

Can this email validation page be improved?


foxclone
Go to solution Solved by foxclone,

Recommended Posts

 

 

My contact form and this email validation works well. I'd like some input about whether this validation form can be improved.

<?php
error_reporting(E_ALL);
ini_set('display_errors', '1');

if(isset($_POST['email'])) {
 
    // EDIT THE 2 LINES BELOW AS REQUIRED
    $email_to = "help@foxclone.com";
    $email_subject = "Your email subject line";
 
    function died($error) {
        // your error code can go here
        echo "We are very sorry, but there were error(s) found with the form you submitted. ";
        echo "These errors appear below.<br /><br />";
        echo $error."<br /><br />";
        echo "Please press your back key and fix these errors.<br /><br />";
        die();
    }
 
 
    // validation expected data exists
    if(!isset($_POST['name']) ||
        !isset($_POST['email']) ||
        !isset($_POST['type']) ||
        !isset($_POST['message'])) {
        died('We are sorry, but there appears to be a problem with the form you submitted.');     
    }

   
    $name = $_POST['name']; // required
    $email = $_POST['email']; // required
    $subject = $_POST['type']; // required
    $subject2 = "Copy of email sent to foxclone.com"; //fixed
    $message = $_POST['message']; // required

 
    $error_message = "";
    $email_exp = '/^[A-Za-z0-9._%-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,4}$/';
 
  if(!preg_match($email_exp,$email)) {
    $error_message .= 'The Email Address you entered does not appear to be valid.<br />';
  }
 
    $string_exp = "/^[A-Za-z .'-]+$/";
 
  if(!preg_match($string_exp,$name)) {
    $error_message .= 'The Name you entered does not appear to be valid.<br />';
  }
 
  if($subject='') {
    $error_message .= 'You must select a Category.<br />';
  } 

  if(($subject)!=('Questions'||'Report Problem'||'Suggestion'||'Other'||'Website Problem')) {
    $error_message .= 'The Category you entered is invalid.<br />';
    }
 
  if(strlen($message) < 10) {
    $error_message .= 'The Comments you entered do not appear to be valid.<br />';
  }
 
  if(strlen($error_message) > 0) {
    died($error_message);
  }
 
    $email_message = "Form details below.\n\n";
 
     
    function clean_string($string) {
      $bad = array("content-type","bcc:","to:","cc:","href");
      return str_replace($bad,"",$string);
    }
 
     
 
    $email_message .= "Name: ".clean_string($name)."\n";
    $email_message .= "Email: ".clean_string($email)."\n";
    $email_message .= "Subject: ".clean_string($subject)."\n";
    $email_message .= "Message: ".clean_string($message)."\n";

    $formcontent="From: $name\r\nEmail: $email\r\nSubject: $subject\r\nMessage: $message";
    if ($subject =="Website Problem") {
            $recipient="webmaster@foxclone.com" ;
          }
    else {
            $recipient="help@foxclone.com";
          }
    $mailheader = "From: $email\r\n";

    
    mail($recipient, $subject, $formcontent, $mailheader) or die("Error1!");
    mail($email, $subject2, $formcontent, $mailheader) or die("Error2!");
    echo "Mail Sent. Thank you " . $name . ", we will contact you shortly.. " . " -" . "<a href='index.php#home' style='color:#ff0099;'> Return Home</a>";
  
 
}
?>

Thanks in advance.

Edited by foxclone
Link to comment
Share on other sites

  • foxclone changed the title to Can this email validation page be improved?

Didn't look through the whole set of code but a quick partial answer is, you can use the php filter_var function for your email validation.  That will be better than your regex.

if (false === filter_var($email, FILTER_VALIDATE_EMAIL)) {
    $error_message .= 'The Email Address you entered does not appear to be valid.<br />';
|

 

Another better way to check for input:

if (empty(trim($_POST['name'])) || empty(trim($_POST['email'])) || etc ) {
    died('We are sorry, but there appears to be a problem with the form you submitted.');  
}

// Code continues

This is a more common "positive" guard clause, where if something should cause an exit from processing, check for that and return/exit.  The rest of the code is considered "happy path".  You are doing a similar thing with your short circuit evaluation, but it requires all the nots, and isn't clear or easily maintainable code.  I wouldn't recommend it when you can do something simpler.

The obvious improvement is that your code will pass if someone types a simple space into your form.

Link to comment
Share on other sites

@gizmola - Thanks for giving my code a look. I'm having a problem with your second solution. Visual Studio is giving me errors starting with wanting me to put a curly brace in front of the if. Should I delete all the code before that?

 

NOTE: got it fixed. Deleted all code before it. Working beautifully now.

Edited by foxclone
additional info
Link to comment
Share on other sites

if($subject='') {
    $error_message .= 'You must select a Category.<br />';
}

This is incorrect code, as you're assigning an empty string to $subject rather than comparing to one.  The if won't trigger because an empty string is a false-like value, but your subject will be blank in the email.  You need to use double-equals (==) for comparison.

 

if(($subject)!=('Questions'||'Report Problem'||'Suggestion'||'Other'||'Website Problem')) {
    $error_message .= 'The Category you entered is invalid.<br />';
}


 
This is incorrect code, as you cannot compare against multiple values in that way.  If you wanted to test against multiple values, you need to repeat the variable name and condition for each one, as in:

if ($subject != 'Questions' || $subject != 'Report Problem' || ...){

That would be correct code, but incorrect logic as it would end up being always true.  If $subject == 'Questions', then it's != 'Report problem' and the condition is true.  Likewise, if $subject == 'Report Problem', it is != 'Questions' and the condition is true.

The correct logic would be to use and (&&) as in

if ($subject != 'Questions' && $subject != 'Report Problem' && ...){

Alternatively, in this type of situation, you can use the in_array function and check against an array of values.  This is shorter and more similar to your original code.

if (!in_array($subject, ['Questions', 'Report Problem', ...])){


Finally,

$string_exp = "/^[A-Za-z .'-]+$/";
if(!preg_match($string_exp,$name)) {
    $error_message .= 'The Name you entered does not appear to be valid.<br />';
}


While this is not incorrect like the others, it's generally recommended to not try and validate a persons name (beyond something like maximum length / not blank).  There's really no specific format for a persons name to validate against, and doing so means you'll always end up eventually telling someone that their name is not valid.

Link to comment
Share on other sites

there's something that can be fixed, simplified, and even removed in just about every line of code. the code is insecure (several places), provides a bad User eXperience (vague, cryptic user messages and it appears that the form and from processing code are on different pages), contains unnecessary and unused variables and code, has no error handling that would tell you (the developer) when and why it doesn't work, and contains an operational problem that will probably make it not send emails from actual visitors (which may have actually worked for the testing you did.)

a list just going from top to bottom -

  1. the php error related settings should be in the php.ini on your system, so that they can be changed in a single place. when you put your application onto a live/public server, you want to log all php errors (you don't want to display them as this gives hackers useful information when they intentionally do things that cause errors.) if the php.ini on your live server is already setup to do this, you don't have to touch your code when moving it. with the settings in your code, you have to find and edit/remove these lines of code every place they exist. Keep It Simple (KISS.)
  2. you should detect if a post method form has been submitted, rather than to detect if a specific field is set (and don't try to detect if the submit button is set because there are cases where it won't be.) by hard-coding a specific field in the test, you are creating more work every time you write new code.
  3. the two lines after the main comment in the code are configuration settings. any value that is application specific in the  body of the code should be set in this section, so that you don't need to go through the code to find all of them or to remove them when posting code in a help forum (do you really want your email addresses to get indexed by search engines on phpfreaks?) despite you editing the first of these two variables, neither are being used anywhere in the code.
  4. the died() function is ridiculous and has been floating around on the internet for years. you should display any user/validation errors when you re-display the form and repopulate the form fields with any existing entered/selected/checked values, so that the visitor doesn't need to keep reentering things over and over. the form processing code and the form should be on the same page. the user should not need to be pressing any keys to get to the point of correcting any errors. this function does not support doing any of this.
  5. the long list of isset(), which are now empty(trim()) calls, is pointless. firstly, if you had a form with 30 fields, would writing out a statement like this make sense? (your answer should be no.) next, except for unchecked checkbox/radio fields, all other fields will be set once the form has been subtitled. this line does nothing useful.
  6. don't copy variables to other variables for nothing. this is just a waste of time typing and making typo mistakes. something that would be useful, would be to trim all the data values, before validating them. this can be done with one single line of code, by keeping the input data as a set, in an array variable, then operating on elements in this array variable throughout the rest of the code.
  7. use the same name for any particular item throughout the code. you are referring to one item as 'type', 'subject', and 'category'.
  8. you should store user/validation errors in an array, using the field name as the array index, and don't store html markup as part of the message. this will let you preform dependent validation tests only when appropriate, output the error next to the corresponding field if you want, and keep any markup/styling in the html document where it belongs. to test if there are no errors, the array will be empty. to display the errors at the appropriate location(s) in the html document, you would test if the array is not empty.
  9. as @gizmola already pointed out, use php's filter_var with the FILTER_VALIDATE_EMAIL flag.
  10. the if($subject='') { line only uses one =, so it is an assignment operator, not a comparison operator.
  11. the next test, if(($subject)!=('Questions'||...)) { doesn't work at all. you would need an array of the permitted choices (which you should have anyway, so that you can dynamically build the select/option menu), then use in_array() to test if the submitted value is not in the array of permitted choices.
  12. the clean_string function is another piece of junk found on the internet. delete it and forget you ever saw it. it doesn't even do an case-insensitive replacement, so just including a capital letter in any of the values will bypass it. you are also not using the $email_message that's being built with calls to it in it.
  13. any external, unknown, dynamic value that you output in a html context, such as an email body or on a web page, should have htmlentities() applied to it to help prevent cross site scripting.
  14. these emails are NOT being sent from the email address that someone has entered in the form (except perhaps during your testing.) they are being sent from the mail server at your web hosting. the From: mail header must contain an email address that corresponds to your web hosting. you can include the submitted email address as a Reply-to: mail header, after validating that it is exactly and only one validly formatted email address.
  15. the error handling for the mail() call should setup and display a helpful message for the visitor, e.g. - The email could not be sent, the site owner has been notified, and you would LOG all the information you have abut the problem such as the  datetime, the actual error returned by the mail() call, and the values that were used in the mail() call.
  16. by sending the response email to the arbitrarily submitted email address, you are allowing spammers/bots send email through your mail server. forget about doing this. just setup and display the success message.
  17. you should actually switch to use either the phpmailer or swiftmailer class. they take care of formatting message bodies and mail headers, so you will have more success at getting your emails to work.
  18. after the successful completion of the form processing code (no errors), you should perform a header() redirect to the exact same url of the current page to cause a get request for that page. this will prevent the browser from re-submitting the form data if the user reloads the page or browses away from and back to that page.
  19. if you want to display a one-time success message, store it in a session variable, then test, display, and clear that variable at the appropriate location in the html document, not forgetting to apply .htmlentities() to any external, unknown, dynamic value in the message.

 

 

Link to comment
Share on other sites

@mac_gyver - Thanks for your detailed response. The code has been in use for over a year without complaint from our users although I'm open to making the email system more secure. The email page was initially developed when I had little knowledge and was using various internet pages to develop it. I can see that I have a lot of studying to do before I start re-writing it. Thanks again.

@kicken - Thanks for pointing out those errors.

Link to comment
Share on other sites

if you apply all the corrections and suggestions, you should end up with something that looks like this (tested except for actually sending the email) -

<?php

// initialization

session_start();

$email_contact = "help@foxclone.com";
$email_website = "webmaster@foxclone.com";

// definition of permitted types/subject/category. use to dynamically build the option list,
// pre-selecting any existing choice, and used in the validation logic
$permitted_types = ['Questions', 'Report Problem', 'Suggestion', 'Other', 'Website Problem'];

$post = []; // array to hold a trimmed working copy of the form data
$errors = []; // array to hold user/validation errors


// post method form processing
if($_SERVER['REQUEST_METHOD'] == 'POST')
{
	// trim all the data at once
	$post = array_map('trim',$_POST); // if any of the fields are arrays, use a recursive call-back function here instead of php's trim function

	// inputs: name, email, type/subject/category, message - all required
	
	// validate the inputs
	if($post['name'] === '')
	{
		$errors['name'] = 'Name is required.';
	}
	if($post['email'] === '')
	{
		$errors['email'] = 'Email is required.';
	}
	else
	{
		// while it is true that the following email format validation will produce an error
		// for an empty value, you should specifically tell the visitor what is wrong with what
		// they submitted
		if (false === filter_var($post['email'], FILTER_VALIDATE_EMAIL))
		{
			$errors['email'] = 'The Email Address you entered does not appear to be valid.';
		}
	}
	if($post['type'] === '')
	{
		$errors['type'] = 'You must select a Type/Subject/Category.';
	}
	else
	{
		// you will only see the following error due to a programming mistake or someone/something submitting their own values
		if(!in_array($post['type'],$permitted_types))
		{
			$errors['type'] = 'The selected Type is invalid.';
			// you would want to log the occurrence of this here...
		}
	}
	if($post['message'] === '')
	{
		$errors['message'] = 'Message is required.';
	}
	else
	{
		if(strlen($post['message']) < 10)
		{
			$errors['message'] = 'The Message must be at least 10 characters.';
		}
	}

	// if no errors, use the submitted data
	if(empty($errors))
	{

		// apply htmlentities() to help prevent cross site scripting when viewing the received email in a browser
		$formcontent = htmlentities("From: {$post['name']}\r\nEmail: {$post['email']}\r\nSubject: {$post['type']}\r\nMessage: {$post['message']}", ENT_QUOTES);

		if ($post['type'] === "Website Problem")
		{
			$recipient=$email_website;
		}
		else
		{
			$recipient=$email_contact;
		}

		// add $post['email'] as a Reply-to: header if desired, it is one, valid email address at this point
		$mailheader = "From: $recipient\r\n";

		if(!mail($recipient, $post['type'], $formcontent, $mailheader))
		{
			// an error
			// setup the user error message
			$errors['mail'] = 'The email could not be sent, the site owner has been notified.';
		
			// system error handling goes here... - datatime, get the last error message, include the mail parameter values...
			// at this point, all parameters are either an internal value, have been validated they they are just an expected value/format, or have had htmlentities() applied.
			
		}
	
		// if no errors at this point, success
		if(empty($errors))
		{
			$_SESSION['success_message'] = "Mail Sent. Thank you {$post['name']}, we will contact you shortly..";
			die(header("Refresh:0"));
		}
	}
}

// html document starts here...
?>


<?php
// display any success message
if(!empty($_SESSION['success_message']))
{
	// for demo purposes, just output it as a paragraph. add any markup/styling you want
	echo '<p>';
	echo htmlentities($_SESSION['success_message'], ENT_QUOTES);
	echo " - <a href='index.php#home' style='color:#ff0099;'> Return Home</a>";
	echo '</p>';
	unset($_SESSION['success_message']);
}
?>

<?php
// display any errors
if(!empty($errors))
{
	// for demo purposes, just output them as a paragraph. add any markup/styling you want
	echo '<p>'; echo implode('<br>',$errors); echo '</p>';
}
?>

<?php
// (re)display the form here..., re-populating the fields with any existing values


?>

 

Link to comment
Share on other sites

at the success point in the form processing code, you can log or insert the data in a database table so that you have a record of what should have been emailed. some mail servers are configured to silently ignore some invalid attempts and the code could appear like it's sending, when it actually isn't.

Link to comment
Share on other sites

@mac_gyverThanks again. I spent the morning trying to configure phpmailer to work without success. I have a two column layout for the contact form and phpmailer totally ignored all css settings. It also failed to fill the dropdown for the type of message. I'll give your suggestion after I eat lunch.

Link to comment
Share on other sites

@mac_gyver Thanks once more, the script works perfectly. Is there a way to include a CC instead of a Reply-to in the mailheader so the person sending the message gets a copy of what they sent? Replacing the Reply-to with CC doesn't work.  I guess I could put both addresses in an array and process it into the mailheader.

Edited by foxclone
Link to comment
Share on other sites

19 hours ago, foxclone said:

@gizmola - Thanks for giving my code a look. I'm having a problem with your second solution. Visual Studio is giving me errors starting with wanting me to put a curly brace in front of the if. Should I delete all the code before that?

 

NOTE: got it fixed. Deleted all code before it. Working beautifully now.

It was missing a closing param in the snippet I provided.

Link to comment
Share on other sites

most likely the session_start is failing. do you have php's error related settings set as suggested so that php will help you by either displaying or logging all the errors it detects? when i tested and bypassed the fact that i wasn't actually sending the email, the success message worked as expected and i got the red navigation link back to the home page.

Link to comment
Share on other sites

@mac_gyver  I just noticed another problem. After sending an email and getting the success message, if I go to any other page, then return to the contact page, the success message is still there. I tried putting a session_destroy() just before going into the contact page's html, but it didn't help.

Do you have any ideas?

 

Link to comment
Share on other sites

Now I'm not getting the success message at all. I'm getting the following errors on my web host:

Warning: session_start(): Session cannot be started after headers have already been sent in /home/foxclo98/test.foxclone.com/contact.php on line 5

Warning: Cannot modify header information - headers already sent by (output started at /home/foxclo98/test.foxclone.com/contact.php:1) in /home/foxclo98/test.foxclone.com/contact.php on line 104

These warnings showed after adding "error_reporting = E_ALL" (without quotes) to the top of the file.

Link to comment
Share on other sites

@gizmola Thanks for chiming in. Here's the code as it exists at the moment:

error_reporting = E_ALL
<?php

// initialization

session_start();

$email_contact = "help@foxclone.com";
$email_website = "webmaster@foxclone.com";

// definition of permitted types/subject/category. use to dynamically build the option list,
// pre-selecting any existing choice, and used in the validation logic
$permitted_types = ['Questions', 'Report Problem', 'Suggestion', 'Other', 'Website Problem'];

$post = []; // array to hold a trimmed working copy of the form data
$errors = []; // array to hold user/validation errors


// post method form processing
if($_SERVER['REQUEST_METHOD'] == 'POST')
{
	// trim all the data at once
	$post = array_map('trim',$_POST); // if any of the fields are arrays, use a recursive call-back function here instead of php's trim function

	// inputs: name, email, type/subject/category, message - all required
	
	// validate the inputs
	if($post['name'] === '')
	{
		$errors['name'] = 'Name is required.';
	}
	if($post['email'] === '')
	{
		$errors['email'] = 'Email is required.';
	}
	else
	{
		// while it is true that the following email format validation will produce an error
		// for an empty value, you should specifically tell the visitor what is wrong with what
		// they submitted
		if (false === filter_var($post['email'], FILTER_VALIDATE_EMAIL))
		{
			$errors['email'] = 'The Email Address you entered does not appear to be valid.';
		}
	}
	if($post['type'] === '')
	{
		$errors['type'] = 'You must select a Type/Subject/Category.';
	}
	else
	{
		// you will only see the following error due to a programming mistake or someone/something submitting their own values
		if(!in_array($post['type'],$permitted_types))
		{
			$errors['type'] = 'The selected Type is invalid.';
			// you would want to log the occurrence of this here...
		}
	}
	if($post['message'] === '')
	{
		$errors['message'] = 'Message is required.';
	}
	else
	{
		if(strlen($post['message']) < 10)
		{
			$errors['message'] = 'The Message must be at least 10 characters.';
		}
	}

	// if no errors, use the submitted data
	if(empty($errors))
	{

		// apply htmlentities() to help prevent cross site scripting when viewing the received email in a browser
		$formcontent = htmlentities("From: {$post['name']}\r\nEmail: {$post['email']}\r\nSubject: {$post['type']}\r\nMessage: {$post['message']}", ENT_QUOTES);

		if ($post['type'] === "Website Problem")
		{
			$recipient=$email_website;
		}
		else
		{
			$recipient=$email_contact;
		}

		// add $post['email'] as a Reply-to: header if desired, it is one, valid email address at this point
		$mailheader = "From: $recipient\r\n";

		if(!mail($recipient, $post['type'], $formcontent, $mailheader))
		{
			// an error
			// setup the user error message
			$errors['mail'] = 'The email could not be sent, the site owner has been notified.';
		
			// system error handling goes here... - datatime, get the last error message, include the mail parameter values...
			// at this point, all parameters are either an internal value, have been validated they they are just an expected value/format, or have had htmlentities() applied.
			
		}
	
		// if no errors at this point, success
		if(empty($errors))
		{
			$_SESSION['success_message'] = "Mail Sent. Thank you {$post['name']}, we will contact you shortly..";
			die(header("Refresh:0"));
		}
	}
}

// html document starts here...
?>


<?php
// display any success message
if(!empty($_SESSION['success_message']))
{
	// for demo purposes, just output it as a paragraph. add any markup/styling you want
	echo '<p>';
	echo htmlentities($_SESSION['success_message'], ENT_QUOTES);
	echo " - <a href='index.php#home' style='color:#ff0099;'> Return Home</a>";
	echo '</p>';
	unset($_SESSION['success_message']);
}
?>

<?php
// display any errors
if(!empty($errors))
{
	// for demo purposes, just output them as a paragraph. add any markup/styling you want
	echo '<p>'; echo implode('<br>',$errors); echo '</p>';
}
?>

<?php
// (re)display the form here..., re-populating the fields with any existing values


?>

<?php require_once("header.php");?>

<style>
  input, select {
  width: 20rem;
  line-height:30px;
  border:2px solid #2f496e;
  padding: 0;
  margin: 0;
  height: 30px;
  -moz-box-sizing: border-box;
  -webkit-box-sizing: border-box;
  box-sizing: border-box;
  font: 500 1rem sans-serif;
  background: #fff;
}

.input {
  text-indent: 3px;
}
</style>
</head>
<body>
  <?PHP require_once("navbar.php"); ?>


<div class="head__h1"> Need help? Have a suggestion? Why not send us an email?</div>     
  <div class="subtext"> We'll get back to you soon </div>
    <div class ="download">

	  <div class="cont__row" style="background-color: #d9b44a;">
         <div class="cont__column">
              
		       <form method="POST">
                
            <label>Name</label><br> 
            <input type="text" name="name"><br> <br> 
                
            <label>Email</label><br> 
            <input type="email" name="email"><br> <br> 
          
        <label>Select a Category</label> <br> 
            <select name="type" id="category" size="1">
                <option value=''>                 </option>
                <option value='Questions'>Questions</option>
                <option value="Report Problem">Report Problem</option>
                <option value='Suggestion'>Suggestion</option>
                <option value='Other'>Other</option>
                <option value="Website Problem"> Website Problem</option>
            </select>
         
            </div>
        
            <div class="cont__column">
            <label>Message</label><br> 
            <textarea name="message" rows="10" cols="50" style="font: 500 1rem sans-serif"></textarea><br> <br> 
          
          
            <div class="button">
            <input type="image" id="myimage" src="images/email1.jpg" style="height:40px; width:160px;"/>
                
        </form>
      </div>
    </div>
  </div>
  </div> 
  <?PHP require_once("footer.php"); ?>

 

Link to comment
Share on other sites

@gizmola - Here are the warnings. It's happening as soon as I open that page, even though the page displays and the email sends when I click the send button.

Warning: session_start(): Session cannot be started after headers have already been sent in /home/foxclo98/test.foxclone.com/contact.php on line 7

Warning: Cannot modify header information - headers already sent by (output started at /home/foxclo98/test.foxclone.com/contact.php:1) in /home/foxclo98/test.foxclone.com/contact.php on line 106

 

Edited by foxclone
additional info
Link to comment
Share on other sites

11 hours ago, foxclone said:
error_reporting = E_ALL
<?php

this is random nonsense. that line is the syntax you would use if putting the setting into a php.ini file. it's not the syntax for putting the setting into a php file and if it was the correct syntax (which your first post in this thread contains), it would be php code and would go after the opening php tag.

the current error you are getting, which you apparently didn't even read to determine that the output that's preventing the header() from working, is occurring on line 1 of your file. that's the random line you added before the opening php tag.

if your dynamic pages are being cached in the browser, you need to setup your web server's configuration, so that it tells the browser to never cache .php files.

 

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.