Filtering/validating user input

Question

I have a simple PHP form I'm developing to let users submit their name, telephone and email.

At this stage, I basically want to prevent as little abuse and nasty code getting into my system, so I've tried filtering and validating the input.

I was hoping I could get a quick scan to see if I'm doing things well or if it needs improvement.

<?php
session_start();

// prevent form resubmission on hitting back button
header("Cache-Control: no cache");
session_cache_limiter("private_no_expire");

// dev
define('IS_ENV_PRODUCTION', false);

// error reporting
ini_set("error_reporting", E_ALL);
ini_set("error_log", "log/phperror.txt");
error_reporting(E_ALL & ~E_NOTICE);

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


// If Form submitted
if ($_SERVER['REQUEST_METHOD'] == 'POST') {

  // FIRST NAME
  if (empty($_POST["firstname"])){
    $fnameErr = "First name is required";
  } else {
    $firstname = test_input($_POST["firstname"]);
    if (!preg_match("/^[a-zA-Z ]*$/",$firstname)) {
      $fnameErr = "Only letters and white space allowed"; 
    }
  }

  // SURNAME
  if (empty($_POST["surname"])){
    $snameErr = "Surname is required";
  } else {
    $surname = test_input($_POST["surname"]);
    if (!preg_match("/^[a-zA-Z ]*$/",$surname)) {
      $snameErr = "Only letters and white space allowed"; 
    }
  }

  // TELEPHONE
  if (empty($_POST["telephone"])){
    $telErr = "Telephone is required";
  } else {
    $telephone = test_input($_POST["telephone"]);
    if (!filter_var($telephone, FILTER_VALIDATE_INT)) {
      $telErr = "Invalid Telephone format"; 
    }
  }

  // EMAIL
  if (empty($_POST["email"])){
    $emailErr = "Email is required";
  } else {
    $email = test_input($_POST["email"]);
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
      $emailErr = "Invalid email format"; 
    }
  }

  // if no errors found
  if ($fnameErr == '' || $snameErr == '' || $telErr == '' || $emailErr == '') {

    echo 'Form completed successfully!';
    // mail 
    exit();

  }

}
?>

    <form method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>" data-toggle="validator" role="form" id="dseForm" class="form">

      <fieldset class="form-group">

          <legend>Contact Details</legend>

          <div class="form-group">
              <label for="firstname">First Name</label>
              <input type="text" class="form-control" id="firstname" name="firstname" aria-describedby="firstname" placeholder="First Name" value="<?php if (isset($_POST['firstname'])) { echo htmlspecialchars($_POST['firstname']); } ?>" required aria-required="true" />
              <span class="error"><?php echo $fnameErr;?></span>
          </div>

          <div class="form-group">
              <label for="surname">Surname</label>
              <input type="text" class="form-control" name="surname" id="surname" aria-describedby="surname" placeholder="Surname" value="<?php echo htmlspecialchars($_POST['surname']); ?>" required aria-required="true" />
              <span class="error"><?php echo $snameErr;?></span>
          </div>

          <div class="form-group">
              <label class="control-label" for="telephone">Telephone:</label>
              <div class="input-group">
                  <input type="tel" class="form-control" name="telephone" id="telephone" placeholder="Telephone Number" value="<?php echo htmlspecialchars($_POST['telephone']); ?>" required aria-required="true" />
              </div>
              <span class="error"><?php echo $telErr;?></span>
          </div>

          <div class="form-group">
              <label class="control-label" for="email">Email:</label>
              <div class="input-group">
                  <input type="email" name="email" id="email" class="form-control" placeholder="Email Address" value="<?php echo htmlspecialchars($_POST['email']); ?>" required aria-required="true" />
              </div>
              <span class="error"><?php echo $emailErr;?></span>
          </div>

      </fieldset>

      <?php
      if ($failed_login_attempt >= 3) {
      ?>
      <h2>ReCaptcha</h2>
      <div class="g-recaptcha" data-sitekey=""></div>
      <?php 
      }
      ?>

      <button type="submit" id="submitForm" value="Submit" class="submit btn">Submit</button>
      <input type="hidden" name="submitted" id="submitted" value="TRUE" />
      <input type="hidden" name="submit_id" value="<?php //echo mt_rand(); ?>">
      <input type='hidden' name='post_id' value="<?php echo createPassword(64); ?>">

  </form>

Answer 1

test_input

I see this function quite often, and I don't think it's a good idea to use it (see here). It is much better to have specific input filters which match what the input should be. Additionally, applying encoding functions at the wrong time - on input - can lead to double encoding and thus invalid data. The same is true for stripslashes, which may severely change your data.

Filters

Your name filters are quite strict, and many people couldn't enter their real name. If that is acceptable for your application, it can stay as-is, otherwise you might want to look at Falsehoods Programmers Believe About Names.

Encoding

You correctly encode the input when printing it to defend against XSS. That is great, as input filters should only be used as defense in depth, but never as your only defense.

Personally, I would also encode other variables, eg the error variables. Code changes, and if the error variables contain user input in the future, you would be vulnerable.

Ideally, you could use a template engine, which automatically HTML encodes all variables.

Duplication and dedicated Input class

There is duplication in your input filters (eg first and last name).

I would create a dedicated Input class with methods like Input::getAlpha($name), Input::getBool($name), Input::getInt($name), etc, which will return the filtered GET variable (you would then have ...post() methods which return POST). You can also have a generic Input::getByRegex($name, $regex) which filters the input by the given regex.

Usability

You are only allowing numbers in your telephone number. That seems acceptable, but users will enter numbers in all sorts of formats. Instead of just telling them that they did it wrong, you need to tell them how to do it correctly (eg Invalid Telephone format: Please only use digits (eg 123555678)).

Answer 2

// if no errors found
if ($fnameErr == '' || $snameErr == '' || $telErr == '' || $emailErr == '') {

This evaluates true everytime $fnameErr is empty. Use && instead of ||, so all errors have to be empty.