2
\$\begingroup\$

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>
\$\endgroup\$
2
\$\begingroup\$

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)).

\$\endgroup\$
4
  • \$\begingroup\$ Hi Tim - many thanks for your thorough response here - it's greatly appreciated. That's a really valid point about the test_input function. I will look into alternatives specific to each case point. I was just looking for one to target them all initially before drilling down with validation. For the name/surname filters, what would you recommend I use? Duplication - great shout, normally I hate data redundancy. Usability - I had indeed considered stripping symbols and spaces from the telephone and then validating it against a numbers format. Then if it failed, I would explain :) \$\endgroup\$ – michaelmcgurk Apr 16 '17 at 7:44
  • \$\begingroup\$ ^ apologies for the seemingly messy reply - I did format it but it seems it didn't carry this through. \$\endgroup\$ – michaelmcgurk Apr 16 '17 at 7:44
  • 1
    \$\begingroup\$ @michaelmcgurk Catch-all functions are always very difficult, as some data can be filtered very strictly, while other data needs to be (nearly) completely intact (passwords for example). Normally, type-dependend validation + proper defenses (encoding, prepared statements, etc) is enough. If you want more protection, I would use an actual WAF. Name filters depend on the application. If it is important that you get the actual, correct name, no filter would be the only way to go. If it doesn't really matter, just allow the various diacritics and maybe ., ', -, and it's fine. \$\endgroup\$ – tim Apr 16 '17 at 8:03
  • \$\begingroup\$ All fantastic advice, Tim - you've been a huge help :-) I'll maybe not be so strict on validation as I am right now for first name, last name and just look for those characters and letters. I may be back later today with more questions :-D \$\endgroup\$ – michaelmcgurk Apr 16 '17 at 8:19
2
\$\begingroup\$
// 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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.