2
\$\begingroup\$

Below is a PHP script which dynamically finds and process all of the form elements such as inputs, textareas, dropdowns etc... then organize them into a table as shown in the image below when sending the recipient.

enter image description here

I am new to PHP and what I have put together below is made of many snippets from across the web with some little help from another developer. I need advice and guidance from real experts on:

  1. If the script is secure enough?

  2. What are your thoughts, is there a way to better achieve the same results? is there a way to optimize it further?

Here is the required HTML:

<form class="placeholder" id="contactForm" action="assets/form-scripts/form-script.php" method="post" autocomplete="off" novalidate="novalidate">
<input type="hidden" name="email_send_to" value="request@loaidesign.co.uk">
<input type="hidden" name="email_subject" value="Call Back Request">

   <fieldset>
    <p>Input</p>
      <div>
       <input name="Input_Name">
      </div>
   </fieldset>

<button class="submit button" data-form-error="Please check your entries &amp; try again" data-form-pass="Sent, thank You!" type="submit">Submit</button></form>

Here is the PHP

<?php include ("../../configuration.php");
    // Only start sessions if they haven't been already to prevent errors
    if (empty($_SESSION)) {session_start();
}

// If 'data' var was received via POST from form-validation.js
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    // There's not really a need for this line with modern browsers
    ob_start();

    // Open the div around the message
    $message = "<html><body><head><meta http-equiv='Content-Type' content='text/html; charset=utf-8' /></head><div style=\"color:#485363\">";
    $message .= "<img style='width:180px; margin-bottom:20px;' src='" . $domainName . "assets/elements/logo.png' alt='" . $websiteName . " Logo' />";
    $message .= "<table style='border:1px solid #96A1B2; color:#485363; border-collapse:collapse; text-align: left;' cellpadding='10'>";

    // Loop through every single post value
    foreach ($_POST as $key => $value) {
        // If it's not empty
        if (!empty($value)) {
            if ($key == "email" || $key == "Email") {
                $replyto = $value;
            }
            if ($key == "email_send_to") {
                if (strpos($value, ",") !== false) {
                    $multipleEmails = true;
                    $to = explode(",", $value);
                } else {
                    $to = $value;
                }
            }
            if ($key == "email_subject") {
                $subject = $value;
                $message .= "<tr><th colspan='2'>" . $subject . "</th></tr>";
            }

            // See if it's a GA key, if it is, do nothing
            if ($key != "|ga" && $key != "PHPSESSID" && $key != "email_send_to" && $key != "email_subject") {
                // Change the name attributes to look a bit more human-readable
                $thisKey = str_replace("-", " ", str_replace("_", "|", $key));

                if (is_array($value)) {
                    $message .= "<strong>" . $thisKey . ":</strong> ";

                    // Setup a counter to determine the last iteration of the foreach loop
                    $i = 0;

                    // Find out how big the array that we're iterating through is...
                    $len = count($value);

                    // Iterate through the array
                    foreach ($value as $key2 => $option) {
                        if ($i < $len - 1) {
                            // This is not the last value so add a comma.
                            $message .= $option . ", ";
                        } else {
                            // This IS  the last value so add a full stop.
                            $message .= $option . ".";
                        }
                    }
                    $message = substr($message, 0, -1);
                    $message .= "<br />";
                    continue;
                }
                // Populate the message var
                $message .= "<tr><td style='color:#485363; border:1px solid #96A1B2;'><strong>" . $thisKey . "</strong></td><td style='color:#485363; border:1px solid #96A1B2;'>" . $value . "</td></tr>";
            }
        }
    }

    // TRIM THE URL:
    $input = trim($domainName, '/');

    // If scheme not included, prepend it
    if (!preg_match('#^http(s)?://#', $input)) {
        $input = 'http://' . $input;
    }

    $urlParts = parse_url($input);

    // remove www and update $domainName
    $domainName = preg_replace('/^www\./', '', $urlParts['host']);

    if ($to == "") {
        $to = "error@" . $domainName;
    }
    if ($subject == "") {
        $subject = "No Subject";
    }
    if ($replyto == "") {
        $replyto = "noreply@" . $domainName;
    }
    // Close the div around the message
    $message .= '</table></div></body></html>';

    // Mail variables
    $headers = "From: info@" . $domainName . "\r\n";
    $headers .= "Reply-To: " . $replyto . "\r\n";
    $headers .= "MIME-Version: 1.0\r\n";
    $headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";

    // Attempt to send
    if ($multipleEmails == true) {
        foreach ($to as $recipient) {
            $sendMail = @mail($recipient, $subject, $message, $headers);
        }
    } else {
        $sendMail = @mail($to, $subject, $message, $headers);
    }

    // If it fails...
    if (!$sendMail) {
        // Terminate processing with error
        die("There was a problem sending the email");
    } else {
        // Terminate processing with success msg
        header("Location: http://loaidesign.co.uk/assets/includes/thank-you.php");
    }

    // As above, no real need for this line with modern browsers
    ob_flush();

    // Terminate
    die();
}
?>
|improve this question
\$\endgroup\$
1
\$\begingroup\$

I'm no security expert, so I have nothing to add to the obvious points @tim provided. I am an experienced PHP developer though, so I would like to offer you my insights on your code.

It has been a while since I used PHP in the procedural style, but I gave it a shot and refactored your code a bit. It has not been tested, but I believe it should behave the same as yours. Let's first have a look at the code:

<?php

include("../../configuration.php");

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    die('ERROR: Expecting POST request');
}

$host = preg_replace('/^www\./', '', parse_url($domainName))['host'];

$replyTo = "noreply@$host";
$to = "error@$$host";
$subject = 'No Subject';
$vars = array();

foreach ($_POST as $key => $value) {

    if (empty($value)) {
        continue;
    }

    switch ($key) {
        case 'email':
            $replyTo = $value;
            break;

        case 'email_send_to':
            $to = explode(',', $value);
            break;

        case 'email_subject':
            $subject = $value;
            break;

        case '|ga':
        case 'PHPSESSID':
            break;

        default:
            $cleanKey = strtr($key, array(
                '-' => ' ',
                '_' => '|'
            ));
            $vars[$cleanKey] = is_array($value)
                ? implode(', ', $value) . '.'
                : $value;
    }
}

$varsAsHtml = '';
foreach ($vars as $name => $content) {
    $varsAsHtml .=
        '<tr>'
        .   "<td  style='color:#485363; border:1px solid #96A1B2;'><strong>$name</strong></td>"
        .   "<td style='color:#485363; border:1px solid #96A1B2;'>$content</td>"
        .'</tr>'
    ;
}

$message = <<<MESSAGE
<html>
    <head>
        <meta http-equiv='Content-Type' content='text/html; charset=utf-8' />
    </head>
    <body>
        <div style="color:#485363">
            <img style='width:180px; margin-bottom:20px;' src='{$domainName}assets/elements/logo.png' alt='$websiteName Logo' />
            <table style='border:1px solid #96A1B2; color:#485363; border-collapse:collapse; text-align: left;' cellpadding='10'>
                <tr><th colspan='2'>$subject</th></tr>
                $varsAsHtml
            </table>
        </div>
    </body>
</html>
MESSAGE;

$headers = <<<HEADERS
From: info@$host
Reply-To: $replyTo
MIME-Version: 1.0
Content-Type: text/html; charset=ISO-8859-1
HEADERS;

foreach ($to as $recipient) {
    $mailSent = mail($recipient, $subject, $message, $headers);
}

if (!$mailSent) {
    die('ERROR: There was a problem sending the email');
}

header("Location: http://loaidesign.co.uk/assets/includes/thank-you.php");

What did I do and why:

  • No need to start the session, since you are not using it
  • As soon as you notice it is not a POST request, you are done, no need to nest here
  • Build the message in as little steps as possible, and most of all keep that HTML readable. You'll have to admit that my message reads a lot clearer and it would be a lot easier to change something or spot errors (there where a few that I fixed, like opening the body before the head and outputting your $key outside a td)
  • I changed your chain of if statements to a single switch, because it makes a lot more sense here and is a lot more readable
  • I started by defaulting your variables before the loop, so you do not need to check their existence and validity afterwards before using them. If they are found inside the loop, they will just be overwritten.
  • I removed the intermediate variables you used to parse your host out of the $domainName. You don't reuse them, and they don't really at to the readability of your code. I also gave that variable a different name of $host. Reusing global variables can lead to horrible bugs.
  • I always make an array out of those $to recipients, so the foreach will be enough to send them. If there is only one recipient, it will only iterate once.
  • I reduced the parsing of array $value to a single implode. It should have the same effect, is a lot cleaner and probably faster as well.
  • I replaced your str_replace(str_replace) with the strtr function. It should be faster, is more readable, and a lot easier to add extra replacements later on.
  • else statements can be useful, but you can often do without if you think a bit further, and it leads to cleaner code. There is ie. no need for an else if you already die inside your if

I'm probably forgetting a few things here, but I think things should be clear from the code. Do feel free to ask if anything isn't clear or if you want me to explain further.

I hope this helps, and happy coding!

Update
As for your question about security, your script is indeed not very secure. The obvious solution would be not to have the subject and address in your html, but keep them on the server side. If you want to be able to use that script for multiple forms, you could work with some sort of form id, and in your script determine, based on the id, which subject and e-mail to use. Something like this:

$forms = array(
    'callback' => array(
        'subject' => 'Call back request',
        'to' => 'email@example.com'
    ),
    // possible other forms
);

$formId = isset($POST['form_id']) ? $POST['form_id'] : null;

if (! array_key_exists($formId, $forms) {
    die('ERROR: unknown form id');
}

$to = $forms[$formId]['to'];
$subject = $forms[$formId]['subject'];
|improve this answer
\$\endgroup\$
  • \$\begingroup\$ Thank you very much @pevera - your very well documented and detailed answer is really helpful. I am still going through it - but I have a quick question please. As you can see, currently I am using <input type="hidden" name="email_send_to" value="request@loaidesign.co.uk"> <input type="hidden" name="email_subject" value="Call Back Request"> - which is not secure is it? anyone can just inject things into it, if so, is there another way of doing this? thank you \$\endgroup\$ – Leo May 9 '15 at 11:08
  • \$\begingroup\$ @Leo have a look at the update... \$\endgroup\$ – Pevara May 10 '15 at 15:36
  • \$\begingroup\$ You rock! Thank you, will post the updated version soon for a final review :) \$\endgroup\$ – Leo May 10 '15 at 15:37
2
\$\begingroup\$

If the script is secure enough?

If you are not echoing or interacting with a database, and you don't have any authentication (the script is open for everyone) there isn't all that much that can go wrong.

Your script is open to mail injection, but that doesn't really matter, as the script itself allows the sending of multiple emails to user controlled recipients.

I'm not sure if this is intended though, because the example form doesn't allow this, and it doesn't seem like a good idea, because it allows anyone to mass-distribute spam via your server. Also, as you are calling mail for each recipient, your script could help an attacker in a DOS attack.

If this is not intended: Do not trust user input, ever. Attackers don't have to comply with what a HTML form says, they can just send whatever they want, for example POSTDATA=email_send_to=spam@example.com,spam@example2.com,spam@example3.com&email_subject=Buy stuff&Input_Name=test

Misc

  • Your code is pretty deeply nested. You should return early to reduce the level of nesting, eg if ($_SERVER['REQUEST_METHOD'] != 'POST') { return; }, and you should extract code into functions (this not only reduces the level of nesting, but also increases readability and reusability).
  • Don't swallow errors with @.
  • Many of your comments don't add all that much information. It's not good to have comments like that, because it leads to a reader not reading any comments - not even important ones - after a while. You can just remove many of them, eg // Terminate, // If it fails..., // Terminate processing with error, etc. Many other comments would become useless (or could be converted to PHPDoc comments) if you extract code to functions.
|improve this answer
\$\endgroup\$

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.