4
\$\begingroup\$

This is for commercial purposes. It's not going to a database, but will just be emailed to a person via phpMailer.

I am not good at PHP (truly awful). I'd like to know whether this is good enough for going production or not.

<?php
session_start(); //allows use of session variables

if ($_SERVER["REQUEST_METHOD"] == "POST" && !empty($_SESSION['token']) && $_SESSION['token'] == $_POST['token']) {

      $token = md5(uniqid(mt_rand(), true));
      $_SESSION['token'] = $token;
      $_SESSION['token_time'] = time();


   if (empty($_POST["first-name"])) {
     $firstNameErr = "First name is required";
   } else {
     $first_name = test_input($_POST["first-name"]);
   }

    if (empty($_POST["last-name"])) {
     $lastNameErr = "Last name is required";
   } else {
     $last_name = test_input($_POST["last-name"]);
   }

   if (empty($_POST["email"])) {
     $emailErr = "Email is required";
   } else {
     $email = test_input($_POST["email"]);
   }

   if (empty($_POST["message"])) {
     $messageErr = "Message is required";
   } else {
     $message = test_input($_POST["message"]);
   }


   if(isset($first_name) && isset($last_name) && isset($email) && isset($message))
   {
     $_SESSION['first_name'] = $first_name;
     $_SESSION['last_name'] = $last_name;
     $_SESSION['email'] = $email;
     $_SESSION['message'] = $message;
     header("Location: contact9SessionsCRSF2.php");
     exit; 
   }
}
else {
  $token = md5(uniqid(mt_rand(), true));
  $_SESSION['token'] = $token;
  $_SESSION['token_time'] = time();
}

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

  <nav class="navbar navbar-default navbar-fixed-top">
  <div class="container-fluid">
    <!-- Brand and toggle get grouped for better mobile display -->
    <div class="navbar-header">
      <button type="button" class="navbar-toggle collapsed" data-toggle="collapse" data-target="#bs-example-navbar-collapse-1" aria-expanded="false">
        <span class="sr-only">Toggle navigation</span>
        <span class="icon-bar"></span>
        <span class="icon-bar"></span>
        <span class="icon-bar"></span>
      </button>
      <a class="navbar-brand" href="index.html">New Daimaru Hotel<small id="address_of_hotel">345 E 1st Street Los Angeles, CA 90012</small></a>
    </div>

    <!-- Collect the nav links, forms, and other content for toggling -->
    <div class="collapse navbar-collapse" id="bs-example-navbar-collapse-1">
      <ul class="nav navbar-nav navbar-right">
        <li><a href="about.html">About</a></li>
        <li><a href="rooms.html">Rooms & information</a></li>
        <li><a href="reserve.php">Book to Reserve</a></li>
        <li><a href="location.html">Location Info</a></li>
        <li><a href="additional.html">Additional Info</a></li>
        <li><a href="contact.php">Contact</a></li>
      </ul>
    </div><!-- /.navbar-collapse -->
    <ul class="nav navbar-nav navbar-right">
        <p class="pull-right language_choice_english"><a href="contact.php">English</a></p>
        <p class="pull-right language_choice_japanese"><a href="contact-ja.php">Japanese</a></p>
    </ul>
  </div><!-- /.container-fluid -->
</nav>
<div id="main-container" class="col-xs-12">

  <form class="form-horizontal"  method="post" action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">

  <input type="hidden" name="token" value="<?php echo $token; ?>" />
    <div class="form-group">
      <label class="col-sm-2 control-label">First Name</label>
      <div class="col-sm-10">
      <input class="form-control" name="first-name" id="first-name" placeholder="First Name" type="text" value="<?php if(isset($first_name)) { echo $first_name; }?>">
       <?php if(isset($firstNameErr)) print ('<span class="error">* ' . $firstNameErr . '</span>'); ?>
      </div>
    </div>
    <div class="form-group">
      <label class="col-sm-2 control-label">Last Name</label>
      <div class="col-sm-10">
      <input class="form-control" name="last-name" id="last-name" placeholder="Last Name" type="text" value="<?php if(isset($last_name)) { echo $last_name; }?>">
       <?php if(isset($lastNameErr)) print ('<span class="error">* ' . $lastNameErr . '</span>'); ?>
     </div>
    </div>
      <div class="form-group">
      <label class="col-sm-2 control-label">Email</label>
      <div class="col-sm-10">
      <input class="form-control" name="email" id="email" placeholder="Email" type="email" value="<?php if(isset($email)) { echo $email; }?>">
       <?php if(isset($emailErr)) print ('<span class="error">* ' . $emailErr . '</span>'); ?>
      </div>
    </div>
    <div class="form-group">
      <label class="col-sm-2 control-label">Message</label>
      <div class="col-sm-10">
      <textarea class="form-control" rows="2" placeholder="Please type in your message" name="message" id="message" value="<?php if(isset($message)) { echo $message; }?>"></textarea>
       <?php if(isset($messageErr)) print ('<span class="error">* ' . $messageErr . '</span>'); ?>
      </div>
    </div>

    <div class="form-group">
    <div class="col-sm-offset-2 col-sm-10">
      <button type="submit" class="btn btn-default">Submit</button>
    </div>
  </div>
</form>

Here's the page that does the actual mailing:

<?php

session_start();

$first_name = $_SESSION['first_name'];
$last_name = $_SESSION['last_name'];
$email = $_SESSION['email'];
$message = nl2br($_SESSION['message']);

require 'PHPMailerAutoload.php';

$mail = new PHPMailer;

//$mail->SMTPDebug = 3;                               // Enable verbose debug output

$mail->isSMTP();                                      // Set mailer to use SMTP
$mail->Host = 'hosting_info';  // Specify main and backup SMTP servers
$mail->SMTPAuth = true;                               // Enable SMTP authentication
$mail->Username = 'testy@URL.com';                 // SMTP username
$mail->Password = 'ugh';                           // SMTP password
$mail->SMTPSecure = 'tls';                            // Enable TLS encryption, `ssl` also accepted
$mail->Port = 587; 

$mail->addReplyTo( $email, $first_name );
$mail->addAddress( $email, $first_name );
$mail->addAddress( 'my_email_address', 'Staff' );
$mail->From = 'my_email_address';
$mail->FromName = 'Staff';


$mail->isHTML(true);                                  // Set email format to HTML

$mail->Subject = 'Contact';
$mail->Body    = "<h1>First Name: $first_name</h1>"
                                    ."<h1>Last Name: $last_name</h1>"
                                    ."<h1>Email: $email</h1>"
                                    ."<h1>Message $message</h1>"; 

$mail->AltBody = 'To view the message, please use an HTML compatible email viewer!';

if(!$mail->send()) {
    header('location: URL/contactError.html');

} else {
    header('location: URL/contactResult2.html');

}

Contents of contactError.html are:

<h1>Apologies, it seems that the email didn't get sent.</h1>
<h3>Please refill the form, and try again.</h3>
<h3>Thank you!</h3>

Contents of contactResult2.html are:

  <h4 class="typl8-gamma centered">Contact Form Emailed</h4>
  <h1 class="typl8-delta centered">Thank you for contacting us.</h1>
  <h3 class="typl8-delta centered">You will hear from us shortly</h3>
| improve this question | | | | |
\$\endgroup\$
  • \$\begingroup\$ We could give you a better review if you included the page that actually does the mailing :-) \$\endgroup\$ – Quill Dec 29 '15 at 1:24
  • \$\begingroup\$ @Quill-HATMANIAC, hey, have included the page that does the mailing: =) \$\endgroup\$ – user273072545345 Dec 29 '15 at 6:24
2
\$\begingroup\$

Input Validation

The test_input function seems to be from here, and it's really not a good function to use (see here and here).

In short: it isn't recommended defense against anything (the recommendation for XSS are to HTML encode when outputting data, not when receiving it, and the function doesn't provide defense against anything else), and it makes your data dirty, leading to usability problems.

Misc

  • Your indentation is off, and your bracket positioning is inconsistent, making your code harder to read.
  • Using a new CSRF token for each request is good for security, but not that good for usability (breaks back button, etc). Evaluate if this is really necessary, and if not, use a token per session.
  • Having file names such as contact9SessionsCRSF2 or contactResult2 is a sign that you have files in your code base which are not actually used, but kept "just in case". This can easily lead to confusion, think about deleting them and removing the 2 from the file names, and/or use version control.
  • The token generation and setting is duplicated code, which is not a good idea as it makes it hard to change and clutters up your code. You could extract it into a generateAndSetToken function.
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ 1) point made on indentation 2) this CSRF token doesn't seem to break the back button ... is that a bad sign? 3) contact9SessionsCRSF2 and contactResult2 are not "just in case files" but useful files that come in place due to headers. \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:27
  • \$\begingroup\$ 4) regarding the fact the token generation is duplicated, I'm going to be removing the second one from bottom else ... \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:28
  • \$\begingroup\$ @user27307254534534534543675765 2) my mistake, in this case it really doesn't break the back button (resubmitting doesn't work though, but that's probably a good thing). 3) In that case, there are probably better file names (contactResultHeader and contactResultBody come to mind, but I don't know the exact contents). 4) But then it wouldn't work anymore. You could remove the generation in the if case, which actually doesn't seem to be needed. \$\endgroup\$ – tim Dec 29 '15 at 19:47
  • \$\begingroup\$ actually, the first token generated is needed moreso than than the one at the else at bottom because what happened when I had only the one at the else, the token caused grief when I deliberately messed up the required fields, and then would mess it up again before submitting it ... but it would blank out each time that happen despite the echo thing ... so when I had it on the top, and messed up the required fields several times, the variables stayed in place ... \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:53
  • \$\begingroup\$ also point made on the filenames ... will fix that later today ... \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:53
1
\$\begingroup\$

First your code looks rather clean, and easy to read: that's already an important point.

Then a possible improvement I immediately notice is that, because processing of the query parameters is quite similar for all of them, you can dramatically reduce your code, both in control part and in HTML part.

To do so, first simply register your query parameters:

  $params = [
    'first-name' => NULL,
    'last-name' => NULL,
    'email' => NULL,
    'message' => NULL,
  ];

Then the four successive occurrences of primary control part becomes:

  foreach ($params as $key => $value) {
    if (!empty(trim($_POST[$key]))) {
      $params[$key] = htmlspecialchars(stripslashes(trim($_POST[$key])));
    }
  }

Above you can see that:

  • here we don't any more generate an error message: seems not useful to do it now, will be done in HTML part so we don't use supplemental variables for this.
  • since this is now a function it can include what you're previously doing in test_input().
  • but we added the trim() early to the !empty($_POST[$key]), because if the initial content was one or more space(s) it wouldn't be seen as empty!

Once each parameter is controlled, now we can also reduce the way we accept or not to use the whole input, like this:

  if (array_filter($params) == $params) {
    foreach ($params as $key => $value) {
      $_SESSION[$key] = $value;
    }
    header("Location: contact9SessionsCRSF2.php");
    exit; 
  }

Here we take advantage of how array_filter() works when no callback function is provided, simply dropping "not-true" items.

Last point regarding this control part, I don't understand why you generate token and token-time twice: once (always) just before controlling, and another time if control failed.
So I'm giving it up in this complete version of the control part (you can re-establish it if there is a good reason):

session_start(); //allows use of session variables

if ($_SERVER["REQUEST_METHOD"] == "POST"
&& !empty($_SESSION['token'])
&& $_SESSION['token'] == $_POST['token']) {

  $token = md5(uniqid(mt_rand(), true));
  $_SESSION['token'] = $token;
  $_SESSION['token_time'] = time();

  $params = [
    'first-name' => NULL,
    'last-name' => NULL,
    'email' => NULL,
    'message' => NULL,
  ];
  foreach ($params as $key => $value) {
    if (!empty(trim($_POST[$key]))) {
      $params[$key] = htmlspecialchars(stripslashes($_POST[$key]));
    }
  }
  if (array_filter($params) == $params) {
    foreach ($params as $key => $value) {
      $_SESSION[$key] = $value;
    }
    header("Location: contact9SessionsCRSF2.php");
    exit; 
  }
}

Then we may do something pretty equivalent for the HTML part (only the main-container content changes):

<div id="main-container" class="col-xs-12">
  <form class="form-horizontal"  method="post"
    action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>">

    <input type="hidden" name="token" value="<?php echo $token; ?>" />
<?php
$param_names = [
    'first-name' => 'First Name',
    'last-name' => 'Last name',
    'email' => 'email',
    'message' => 'message',
];
foreach ($param_names as $key => $value) {
?>
    <div class="form-group">
      <label class="col-sm-2 control-label"><?php echo $value; ?></label>
        <div class="col-sm-10">
            <input class="form-control" type="text"
              name="<?php echo $key; ?>" id="<?php echo $key; ?>"
              placeholder="<?php echo $value; ?>"
              value="<?php echo $params[$key]; ?>">
<?php
if(!$params[$key])
?>
        <span class="error">* <?php echo $value; ?> is required</span>
      </div>
    </div>

    <div class="form-group">
      <div class="col-sm-offset-2 col-sm-10">
        <button type="submit" class="btn btn-default">Submit</button>
      </div>
    </div>
  </form>
</div>
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ um .... thank you for doing this! The thing is ... like I said ... super bad at PHP ... so ... um, and I'm the person who's expected to maintain the site for now ... so ... I'd like it to be something that I can "understand" ... ie, this is a bit over my head ... but I'll try it out later ... thank you again! \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:25
  • \$\begingroup\$ Glad it could help you. If so, might you consider accepting my answer? Apart from this note that I almost only targetted the strategy of coding, but you may benefit from the other answers, which give you some good advices about best practices. \$\endgroup\$ – cFreed Dec 29 '15 at 19:36
1
\$\begingroup\$

$mail->AltBody = 'To view the message, please use an HTML compatible em...

For basic information, why is an html message REQUIRED? Output the data the user needs in plain text and email.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ good point ... I might insert some graphics though to prettify up the email, so ... maybe the line is valid for that purpose? \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:14
  • \$\begingroup\$ I'd still argue that point... Include the data in the email, even as just plain text. It's very bad for the user if they are using a non-html view (outlook setting, for example). \$\endgroup\$ – Canadian Luke Dec 29 '15 at 19:18
  • \$\begingroup\$ ok ... point made ... I'm trying to work my way though everyone's comments, and I'm honestly at loss at how to handle the feedback because honestly I'm terrible at PHP, and this form is for commercial purposes, and I would like to have it commercial grade as much as possible before it goes live. \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:21
  • \$\begingroup\$ I'm going through some extra PHP courses, trying to get used to OOP and using Composer items. The best advice I can give is practice as much as you can, and deal with things in small steps until you have mastered the particular domain you're working in. Good luck, and remember, we're here to help, not to criticize or otherwise. \$\endgroup\$ – Canadian Luke Dec 29 '15 at 19:57
  • \$\begingroup\$ thank you. =) This is, I'm hoping, first AND last time, I'm dealing with PHP. Hence my anxiety that this has to be ready for production purposes. And thank you for reminding me not to take things personally here ... not that I have but it hasn't helped that I'm like, um .... huh? ... =) \$\endgroup\$ – user273072545345 Dec 29 '15 at 20:00
1
\$\begingroup\$

Styling and readability

Your indentation is inconsistent, both for blocks on the same level (3 and 6 spaces) as blocks on different levels (2 or 3 spaces). Inconsistent whitespace can cause a developer to wrongly believe code belongs to a different block than it actually belongs. I recommend fixing all indentation.

Your code-style is inconsistent in some places. Between if and the curly brace is sometimes whitespace, sometimes not. You sometimes place else on a new line, you sometimes do not.

You sometimes use print and you sometimes use echo. While they do not work exactly the same, you use them in the same context. I recommend replacing one of the two with the other for clarity.

Logic errors

You test several parameters with a construction like if (empty($_POST["email"])) {, but use isset($email) in an if-statement that redirects the user. empty(...) is a shorthand for !isset($var) || $var == FALSE. You are not testing for the condition when $var == FALSE. This is a problem with, for example, the empty string. Since you do use trim(..) later to sanitize the data, you probably should use that here too.

Code duplication

The following block is duplicated in both the if and the else-clause. You can move this code above the if-statement, and omit the else statement completely.

$token = md5(uniqid(mt_rand(), true));
$_SESSION['token'] = $token;
$_SESSION['token_time'] = time();

You have several similar blocks to validate the input, like the one below. To avoid code duplication you can consider putting this code in a function.

if (empty($_POST["message"])) {
  $messageErr = "Message is required";
} else {
  $message = test_input($_POST["message"]);
}

Sanitizing

Your function named test_input does not do what it's name suggest. Instead, it does seem to make an attempt at sanitizing the input. I am unsure what stripslashes(..) is doing in there. Furtermore, your call to htmlspecialchars does not name an encoding.

You are not checking if the email-address actually is in the form of an email adddress. You might want to at least consider if it matches .+@.+\..+.

Html semantics

contactError.html and contactResult2.html use headers to output a message. I am assuming that you have the proper html-boilerplate around them. You are misusing headers to display text of various sizes. The text displayed is not actually a header, and should be displayed in a span-tag with custom class instead.

No guards in mail

Your mail script does not check if the content of the session variables is there. It does not even guard against reloading the page. If I extract the name of the script that does the mailing, I can go there directly and a bogus email is sent.

Edit: I was wrong about it not being guarded against reloading the page, because the page itself will do another redirect. A possible exploit with the current setup could be to have an url that looks like an image on a forum, which will actually redirect to yoursite.com/contact9SessionsCRSF2.php. What this will do is creating an army of innocent people sending you empty emails. A DDOS... without needing to actually hack any computers. You should check the validity of the used parameters in the script itself, not assume that a different script in a different request did that for you.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ ok, please forgive me in advance if I misunderstood anything you said here ... 1) both the forms contactError.html and contactResult2.html do have proper html-boilerplate around them ... 2) I thought that my first script checked that the session variables are there before it was directed to the contactResult1.html ... not sure how to guard against reloading of the page though ... \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:23
  • \$\begingroup\$ 3) didn't realize that the test_input function doesn't do what it's supposed to do ... oh ... unless you're looking at the name of the function, and the fact that it's a misnomer of its function? 4) will remove the second token generated in the else at the bottom \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:23
  • \$\begingroup\$ just realized that I do guard against the reloading of the page sort of ... when the page is submitted, the user is redirected to either contactError.html or contactResult2.html ... so ... it does guard against the sessions being reloaded? \$\endgroup\$ – user273072545345 Dec 29 '15 at 19:31
  • \$\begingroup\$ I was partly wrong about the guards. When you inspect the requests in your browser, it will show the mailing page as a seperate request. Refreshing should indeed just reload the error or the result rather than resending the email. Going back in the browser will I guess go back to the last non-redirect. It still partly stands, because the script itself should verify that it is using the correct content, not some arbitrary other script. This setup is still exploitable, because I can, for example, put an "image" on a forum, which will actually redirect to yoursite.com/contact9SessionsCRSF2.php. \$\endgroup\$ – Sumurai8 Dec 29 '15 at 20:02
  • \$\begingroup\$ not sure about what you meant by the comment of "image" on a forum ... but you do have a point that anyone who knows the name of contact9SessionsCRSF.php which is the phpMailer file as one can see from my post above ... but since I'm not using 'POST OR GET, but rather header this is the only way I knew how to get the sessions variables over to the phpMailer form. Any ideas how to make it more secure?1 \$\endgroup\$ – user273072545345 Dec 29 '15 at 20:10
0
\$\begingroup\$

Limit length of incoming, outgoing data. Restrict incoming POST or other to only the character ranges needed. UTF8 has characters which you likely don't need to support; some are used for abusive purposes. Use regular expressions to check incoming POST or other data. Consider captcha's for limiting abuse. If possible do not support emails containing markup. Unset, clear or overwrite each variable when finished with it.

Place lines of code containing credentials for SMTP user account into a separate file without a PHP file extension. Use Apache or other server side restrictions to prevent any direct access. Use file system permissions to restrict access also. Then use require_once within the PHP script to then include this file.

Related but not PHP code effort: have email admin limit number of outgoing messages, IP address sources that can connect with this isolated SMTP account.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ regarding utf8, I think that is needed, as the form will be both in English and in Japanese (I do not know Japanese), but thank you for bringing up that point! \$\endgroup\$ – user273072545345 Dec 29 '15 at 21:32
  • \$\begingroup\$ regarding placing lines of code containing credentials for SMTP account in separate file without PHP file extension ... and then using Apache to prevent direct access ... how do I do that? I know that the system administrator here at work won't know how to do that, and it'll be up to me to tell him ... \$\endgroup\$ – user273072545345 Dec 29 '15 at 21:33
  • \$\begingroup\$ please excuse my naivety, but ... what do you mean by limiting the number of outgoing messages, IP address sources that can connect wit this isolated SMTP account? \$\endgroup\$ – user273072545345 Dec 29 '15 at 21:36
  • \$\begingroup\$ Remove sensitive code and place into another file(s). These files can have different extension than .php for further restriction. Use require_once PHP directive to include the file. Use Apache server configuration, preferred, or .htaccess files to prevent access to files with those extensions. PHP can still access the code and interpret but no one can view the source. Use file system permissions to further restrict these sensitive files, placing them as owned by a separate account, read only file, etc. \$\endgroup\$ – kph0x1 Dec 29 '15 at 22:50
  • \$\begingroup\$ Hopefully the smtp account is setup with a reasonable outgoing limit for emails sent. It should be limited in PHP code- keep track of how many messages sent from client session, ip address -but also limited at smtp host server. Once a script is compromised, you may not discover until you find your email reputation has been ruined or you receive cease & desist letter. Log each message sent. \$\endgroup\$ – kph0x1 Dec 29 '15 at 22:59

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.