Email subscription queries for a PHP website

Question

Is this code resistant against injection? I already used 'real-escape-string' but I don't know if this good enough. As you see I replaced my personal info. And the code does work. I'm building this site for a family member so it needs to be very protected against 'hackers'. Thank you in advance.

HTML:

<input type="text" name="email" id="email">
<div id="sub" class="button"> Subscribe </div>
<div id="unsub" class="button"> Unsubscribe </div>
<div id="answer"></div>

PHP:

$conn = new mysqli('#', '#', '#', '#');

if ($conn->connect_error) {
       die("Connection failed: " . $conn->connect_error);
} 

$email = $_POST['email'];

$sql = sprintf("SELECT * FROM ... WHERE email = '%s'", mysql_real_escape_string($email)); 

$result = $conn->query($sql);

    if ($result->num_rows > 0) {

        die ("Already subscribed");

    } else {

        $sql2 = sprintf("INSERT INTO ... (email) VALUES ('%s')", mysql_real_escape_string($email));
        $result2 = $conn->query($sql2);
        echo "You subscribed!";

    }

Answer 1

If you are using the mysqli functions, then do not call mysql_real_escape_string(), which is part of the deprecated mysql suite of functions. At the least, you should change it to call mysqli_real_escape_string() so that you are using the same suite of functions.

That said, using sprintf() to compose the query and escaping the string parameter yourself is not the best practice. A more foolproof way would be to use prepared statements, such that the library takes care of any necessary escaping for you.

$select_query = $conn->prepare("SELECT * FROM … WHERE email = ?");
$select_query->bind_param('s', $email);
if (! $select_query->execute()) {
    die("Database error");
}
if ($select_query->num_rows > 0) {
    die("Already subscribed");
}

---

I wouldn't use a SELECT query at all to detect whether an e-mail address is already subscribed. Rather, I would place a UNIQUE constraint on the email column, and catch any attempt to insert a duplicate.

Reasons for doing it that way are:

• It's generally good practice to enforce such uniqueness constraints in the database schema. It makes the schema more foolproof and self-documenting. It also automatically creates an index on the email column, which could improve search performance.
• If you enforce uniqueness in the PHP code rather than in the database, then you are vulnerable to a race condition. If two requests come in at the same time, it's possible that one of them could manage to sneakily perform an insert in between the other request's select and insert.
• Eliminating a SELECT query would make it more efficient. You should be checking the result of the INSERT anyway — which you don't. (Your code ignores $result2.)

Answer 2

Really good thoughts from @200_success so I won't rehash the prepared statement and eliminating SELECT thoughts, which I agree with fully.

You do also seem to have a cross-site request forgery (CSRF) vulnerability which you should be aware of. Right now any attacker could simply make arbitrary posts against your endpoint, causing email address to be added to your database. Typically, this is mitigated by using a session token placed in a hidden form field and validated against when POST is made.

---

It is unclear to me how you code differentiates a subscribe action from an unsubscribe action. It would seem like regardless of which button was pushed, a subscribe would attempt to be made.

---

if ($result->num_rows > 0) {

    die ("Already subscribed");

} else {

    $sql2 = sprintf("INSERT INTO ... (email) VALUES ('%s')", mysql_real_escape_string($email));
    $result2 = $conn->query($sql2);
    echo "You subscribed!";
}

It is good practice to design away unnecessary else conditions like you have here. There is no reason whatsoever for your else condition here. You already do a good job of bailing out of execution when you have the insert conflict. This could just be:

if ($result->num_rows > 0) {
   die(...);
}
// rest of code

---

I would consider applying email format validation before you go through the trouble of making the (relatively) expensive database call. This can also help clean up your list. Consider something like:

$email = filter_input(INPUT_POST, 'email', FILTER_VALIDATE_EMAIL);    
if(is_null($email)) {
    // value was missing from input
    // perhaps message user and bail
}
if($email === false) {
    // email format validation error
    // perhaps message user and bail
}
// continue with code