4
\$\begingroup\$

See my original post: here.

I have one html page and four php files that allow users to sign up for an email list. One of the php scripts is a cronjob that deletes unverified rows older than 24 hours, and it is not included below for the sake of post length. I use PDO for my prepared statements. Everything has been tested live and is fully functional to the best of my knowledge. Any and all feedback is welcome. I've bulleted some questions below the snippets. :)

email.html --- Users sign up here

<form action="signup.php" method="POST" autocomplete="off">
    <input type="text" autocomplete="off" placeholder="Email address" name="email" required>
    <br/>
    <input type="submit" autocomplete="off" value="Subscribe">
</form>

signup.php --- Filters and sends user input to the database

<?php 
//1---DATABASE CONNECTION---
$dbHost = "HOST";
$dbName   = "DATABASE";
$dbUser = "USER";
$dbPassword = "PASSWORD";
$port = "PORT";
$charset = 'utf8mb4';

$options = [
    \PDO::ATTR_ERRMODE            => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES   => false,
];

$dsn = "mysql:host=$dbHost;dbname=$dbName;charset=$charset;port=$port";
try {
     $pdo = new \PDO($dsn, $dbUser, $dbPassword, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}
//1---END---

//2---Add to table: IPv4 ADDRESS, EMAIL, DATETIME, and ACODE---

//prevent direct url access of .php from users, routes to starting page
if(($_SERVER['REQUEST_METHOD'] == 'POST') == NULL) {
    header("Location: email.html");
    exit (0);
}

//trim spaces on ends of user email input
$Temail = trim($_POST['email']); //(on mobile, auto-complete often leaves a space at the end)
        
//allow international characters
if(preg_match("/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$^/", $Temail)) {
    //prevents invalid email addresses
    header("Location: invalid.html");
    exit (0);
}

//Check Email Domain MX Record
$email_host = strtolower(substr(strrchr($Temail, "@"), 1));
if (!checkdnsrr($email_host, "MX")) {
    header("Location: invalid.html");
    exit (0);
}

//Prevent users from inputting a specific domain...like mine
$notallowed = [
    'mydomain.com',
];
if (!in_array($email_host, $notallowed) == NULL) {
    header("Location: notallowed.html");
    exit (0);
}

//checks database to make sure the email is not a duplicate
$stmt1 = $pdo->prepare("SELECT email FROM emailTable WHERE email = ?");
$stmt1->execute([$Temail]);
if($stmt1->fetch()) { //prevents adding a duplicate email
    header("Location: duplicate.html");
    exit (0);
}

//send verification email using seperate php file
include_once 'vEmail.php';

//check to see if email could be put together                       
if(include_once 'vEmail' == NULL) {
    header("Location: failure.html");
    exit (0);
}

//set date and time
date_default_timezone_set('America/Los_Angeles');
$dateTime = date('Ymd-His', strtotime('NOW')); // ('Ymd-His' format and LA timezone are preferred)

//variable to store ipv4 address
$euserIP4 = $_SERVER['REMOTE_ADDR'];

//add all data to the database
$stmt2 = $pdo->prepare("INSERT INTO emailTable (IP4, datetime, email, acode) VALUES (:IP4, :datetime, :email, :acode)");
$stmt2->execute(['IP4' => $euserIP4, 'datetime' => $dateTime, 'email' => $Temail, 'acode' => $Acode]);
header("Location: success.html");
exit (0);
//2---END---
?>

vEmail.php --- include_once in signup.php, sends verification email

<?php
//generate verification code
$Acode = bin2hex(random_bytes(30));
                        
//send verification email w/ code
$emailbody = 
"<html>
    <body>
        <table>
            <tr>
                <td>
                    <button><a href='https://www.MYDOMAIN.com/status/verify.php?acode=$Acode'>VERIFY</a></button>
                </td>
            </tr>
        </table>
    </body>
</html>";
                            
$headers = "Reply-To: MY NAME <[email protected]>\r\n"; 
$headers .= "Return-Path: MY NAME <[email protected]>\r\n"; 
$headers .= "From: MY NAME <[email protected]>\r\n";  
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-type: text/html; charset=UTF-8\r\n";
$headers .= "X-Priority: 3\r\n";
    
//send email
mail($Temail, 'Confirm Your Email Subscription', $emailbody, $headers, '-f ' . '[email protected]');
?>

verify.php --- Attached to link that was sent in the verification email

<?php
//1---DATABASE CONNECTION---
$vHost = "";
$vName   = "";
$vUser = "";
$vPassword = "";
$vPort = "";
$vCharset = "";

$vOptions = [
    \PDO::ATTR_ERRMODE            => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES   => false,
];

$vdsn = "mysql:host=$vHost;dbname=$vName;charset=$vCharset;port=$vPort";
try {
     $vpdo = new \PDO($vdsn, $vUser, $vPassword, $vOptions);
} catch (\PDOException $ve) {
     throw new \PDOException($ve->getMessage(), (int)$ve->getCode());
}
//1---END---

//2---VERIFICATION LINK---
//set timezone
date_default_timezone_set('America/Los_Angeles');

//prevent direct url access of .php from users, routes to starting page
if(isset($_GET['acode']) == NULL) {
    header("Location: email.html");
    exit (0);
}

//set verification code variable
$vAcode = $_GET['acode'];
    
//check if row still exists
$vStmt1 = $vpdo->prepare("SELECT verified, acode FROM emailTable WHERE acode = '$vAcode' LIMIT 1");
$vStmt1->execute();

if($vStmt1->rowCount() == NULL) {
    //EXPIRED
    header("Location: expired1.html");
    exit (0);
}

//check if row is verified ('verified' set to 0)
$vStmt2 = $vpdo->prepare("SELECT verified, acode FROM emailTable WHERE verified = 0 AND acode = '$vAcode' LIMIT 1");
$vStmt2->execute();

if($vStmt2->rowCount() == NULL) { //if 'verified' is set to 1 already
    header("Location: expired2.html");
    exit (0);
}

//since 'verified' is set to 0,update verification status to 1
$vStmt3 = $vpdo->prepare("UPDATE emailTable SET verified = 1 WHERE acode = '$vAcode' LIMIT 1");
$vStmt3->execute();

//check if the 'verified' was updated correctly
if($vStmt3->fetch()) {
    header("Location: failure.html");
    exit (0);
}

//SUCCESS
header("Location: verified.html");
exit (0);
//2---END---
?>

Questions and Comments:

  • I've included database connections for each php file, but I've found that some prefer to have a global config file for their connections. Why is that? Is it more efficient?
  • In the original post, someone mentioned that the regex found in signup.php is missing the unicode flag. Could someone explain this, because I wasn't able to find anything on it.
  • While storing IPv4 works well, I haven't been able to figure out how to correctly store IPv6 (as far as I know). I've tried this: bin2hex(inet_pton($_SERVER['REMOTE_ADDR'])); but I couldn't figure out if the output was correct, because it looked nothing like an ipv6 address. Correct me if this looks usable.
  • I'm looking into PHPMailer rather than using the native mail() function. In the case of the above scripts, would this be recommended or is it more for bulk email sending?

Edit 1:

I've just realized that the email validation found in signup.php allows an email address with spaces (ie: ohnothere are [email protected]). The previous version of the script, found at the link at the top of the post, prevented these sorts of addresses. Any ideas why that may be, or how to prevent that? I'd like to stay away from filter_var(FILTER_VALIDATE_EMAIL) since it cancels international characters. Will continue experimenting with it...

Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

Including a global configuration file is not about efficiency, it's just about centralizing the application configuration data.

Suppose, for example, that you're developing a new feature for your application, you want to use a database specifically for this purpose, so as not to interfere with the production database.

If you're including the database configuration on each script, you're going to have to make sure you check (and hopefully double-check) every script to make sure none of your development actions get propagated to the production database. (Even if you were certain none of your changes would be visible to your application if pushed to the production database, using a completely different database saves your production database and application from having to handle the load from both regular users and the development team.)

On the other hand, you could just do this. Note that defining array-valued constants is valid as of PHP 7.0.0, although you could simply use ini-style define( 'DEV_HOSTNAME', ... ) style constant definitions, it's just not as fancy.

define( 'CONFIGURATION', 'PRODUCTION' );

define( 'ENVIRONMENT', [
    'DEVELOPMENT' => [
        'hostname' => 'localhost',
        'database' => 'devdb',
        'username' => 'username',
        'password' => 'password'
    ],
    'PRODUCTION' => [
        'hostname' => 'hostname',
        'database' => 'proddb',
        'username' => 'username',
        'password' => 'password'
    ]
]);

Now all you have to do to switch environments in your application is modify the CONFIGURATION constant, since your application could access the configuration values using something like this.

$hostname = ENVIRONMENT[CONFIGURATION]['hostname'];
$database = ENVIRONMENT[CONFIGURATION]['database'];
$username = ENVIRONMENT[CONFIGURATION]['username'];
$password = ENVIRONMENT[CONFIGURATION]['password'];

What some production applications do is define an abstract Database class whose derived classes implement an interface that defines how they should be accessed, so you could switch between say MySQL and Postgres by doing something like the above. This is one of the reasons PDO is so useful; it allows us to code to an interface, not an implementation.

To answer your question about efficiency, it's actually less efficient to include a separate file that contains the configuration data.

To include a file, the interpreter has to get it first. Application configuration files are (obviously) application-specific, and thus located somewhere in or around the current directory. However, unless the include path is absolute or begins with a '.' or '..', PHP will first look for the file in the include_path.

Assuming the file is actually found eventually, reading it from wherever it is stored requires disk access, which is really slow, although the impact can be mitigated by using SSDs or even tempfs, as well as caching.

Worse yet, if you use the *_once version of either require or include, it won't just include the file (if and when the file is eventually found), the interpreter will go through the additional trouble of verifying the file hasn't already been included.

You can think of the tradeoff between inefficient configuration change propagation and execution speed as a tradeoff between development time efficiency vs. execution time efficiency, and it's definitely worth your while (pun intended).

Larger applications tend to opt for a more object-oriented approach to configuration by essentially encapsulating these global variables in a singleton class that oversees the getting and setting of configuration parameters. You'll probably see this referred to as the Registry pattern.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ With only 4 php files connecting a single database and table, I'm leaning more towards simply including the same database connection in each file (with differing variable name). Which method would you recommend is best for my situation? \$\endgroup\$
    – user231248
    Oct 2, 2020 at 19:28
2
\$\begingroup\$

Yes, use an include file for your DB connection. If you change the DB password or the server, you'll have to update several pages and make sure they all match. That is tedious, and even if you have just 4 pages today, that could be 20 or more in the future.

Avoid unnecessary repetition. Repetition is every programmer's enemy. There is also more stuff that you repeat in other pages, for example:

date_default_timezone_set('America/Los_Angeles');

What is the purpose of this ? Anyway, you should have some sort of config file to centralize settings. And then require_once should do.

DNS records

I think the merit of this is debatable:

//Check Email Domain MX Record
$email_host = strtolower(substr(strrchr($Temail, "@"), 1));
if (!checkdnsrr($email_host, "MX")) {
    header("Location: invalid.html");
    exit (0);
}

You are checking if the domain has at least one MX record. Presumably you want to thwart random submissions by spammers or whatever. But in itself that proves very little, and it does not demonstrate that the submission was made in good faith. I could use anybody's E-mail address, as long as it passes the test. So I don't see a lot of added value in this personally.

Also, transient DNS errors may occur. This function may fail from time to time, even with legitimate domain names. Normally, the mail is not delivered directly but goes to a local queue and in case of DNS lookup failures or whatever your MTA will try sending the mail at regular intervals.

Also, if I am not wrong, the RFC says that in the absence of a MX record, the MTA can use a A record as a fallback (keep in mind that many DNS zones are poorly configured).

strtolower is not needed, as domain names are not case-sensitive. Although you may want to normalize input and force the whole E-mail address to lowercase in case it contains a mix of lowercase and uppercase characters. Purely for cosmetic reasons.

Includes

Don't do this:

//send verification email using seperate php file
include_once 'vEmail.php';

That makes the code even more difficult to follow and understand. Remember, code that is hard to read/understand tends to be more buggy and less secure.

Instead, write a dedicated function that returns a boolean value, or anything you want. But you can have an include at the top of your code to import your functions of course. Could be something like this:

if (!send_verification_email($email, $verification_code)) {
    // an error occured, do something
}
// proceed normally

Misc

I think you have too many redirect pages, just consider this:

  • expired1.html
  • expired2.html
  • email.html
  • invalid.html
  • notallowed.html
  • duplicate.html
  • failure.html
  • success.html
  • verified.html

And of course they must all be very similar. I would strongly advise to evolve your coding practices - use a framework or at least a templating solution. One page would suffice, and you pass some parameters like a custom message so that the page can be rendered in many different ways. You don't need a page for every possible scenario. Just think about maintenance and bloat.

Note that you could use templates for your mails too. All you need is text files with some delimited tags eg %VERIFICATION_CODE%, then you simply replace those tags with the values from your variables.

IPv6

What is your problem with IPv6 ? Are you storing the value in a varchar field ? Then allocate at least 45 characters but there is nothing special about it. Of course IPv6 addresses tend to be longer and have a markedly different pattern. The question is, what is the purpose of storing this information and how do you intend to use it.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.