Converting from MySQLi to PDO account activation

Question

I had been working on a project for some time, and then it went on the way back burner after my daughter was born. I'm back to it, and now I discover that I'm best off using PDO over MySQLi. So, I'm working on the conversion, and I'd like to get some confirmation that I'm handling things well. Essentially: Does this code look secure and sound? [It's just my account activation php.]

<?php
require_once ('common.php');

$x = $y = FALSE;
if (isset($_GET['x']) && filter_var($_GET['x'], FILTER_VALIDATE_EMAIL)) {
    $x = $_GET['x'];
}
if (isset($_GET['y']) && (strlen($_GET['y']) == 32 )) {
    $y = $_GET['y'];
}

if ($x && $y) {
    $query = "SELECT 1 FROM users WHERE (email=:email AND confirm IS NULL)";
    $query_params = array( ':email' => $x );
    try {
        $stmt = $db->prepare($query);
        $result = $stmt->execute($query_params);
    }
    catch(PDOException $ex) {
        die("Failed to run query: " . $ex->getMessage());
    }

    $count = $stmt->rowCount();
        if($count == 1) {
            echo "<h3>Your account is already activated. No need to activate again. You may now log in.</h3>";
            exit();
        } else { // Not an active user
            $query = "UPDATE users SET confirm=NULL WHERE (email=:email AND confirm=:confirm) LIMIT 1";
            $query_params = array(
                ':email' => $x,
                ':confirm' => $y
            );

            try {
                $stmt = $db->prepare($query);
                $result = $stmt->execute($query_params);
            }
            catch(PDOException $ex) {
                die("Failed to run query: " . $ex->getMessage());
            }

            $count = $stmt->rowCount();
                if($count == 1) {
                    echo "<h3>Your account is now active. You may now log in.</h3>";
                    exit();
                } else {
                    echo '<p class="error">Your account could not be activated. Please re-check the link or contact the system administrator.</p>';
                    exit();
                }
        }

} else {
    $url = BASE_URL . 'index.html';
    ob_end_clean();
    header("Location: $url");
    exit();
}

?>

I am aware that the $ex->getMessage() needs to go before production.

Answer 1

The Good

You use bound parameters. You handle errors.

The Bad

You filter the email address but then pass the unfiltered value to the select and update queries. What are you trying to achieve with the check of the filtered email?

You are not checking the validity of the 'y' param (which I assume is an activation token). Isn't the point of this kind of activation to ensure that the real person at that email is doing the activation? The way you have it, an attacker could register an account using any email address. Here is how:

1) Attacker goes to your registration page and enters someone else's email address (i.e. notme@example.com)

2) Attacker crafts and visits the url: yoursite.com/activation.php?x=notme@example.com&y=12345678901234567890123456789012 (where the y value is some random string, 32 chars in length)

Attacker now has an activated account using an email they do not own, having never received the activation email.

The Ugly

Your logic and presentation are mixed together. Perhaps for this relatively simple script, it's not that big a deal. But, over time, these things have a habit of growing and getting very messy. Look into mvc.

One try catch block should be all you need.

Example Rewrite

try {
    //initialize and filter $x and $y
    if (!$x || !$y) {
        throw new Exception("$x or $y invalid");
    }

    //run select query. It's where clause should be something like:
    $where = 'email = :email AND '
           . 'confirm = :token AND '
           . 'confirmExpiration <= :expiration AND '
           . 'activated = false';
    $params = array(
        ':email' => $x,
        ':token' => $y,
        ':expiration' => date('Ymd His'),
    );

    //check results of select query
    if (/* select was not successful */) {
        throw new Exception("Invalid request. x: $x y: $y");
    }

    //Update the account so that it is now active

    //redirect to a success page
} catch (Exception $e) {
    //if production, log it and redirect to a friendly error page
    //if development, redirect to some debug page
}