1

I have email contact in PHP and I wanted to add part where it should check if there is actual order ID written in <input> in my table, otherwise, it sends email.

EDIT: added prepared statement $stmt->execute([ ':order' => $order ]);

<?php
if (isset($_POST['submit'])) {
$subject = $_POST['subject'];
$message = $_POST['message'];
$order = $_POST['orderId'];
$mailTo = "mail@mail.com";
        if ($order != "") {
          $db = new PDO('mysql:host=localhost;dbname=dbname;charset=utf8', 'username', 'password');
          $order = $_POST['orderId'];

          $stmt = $db->query("SELECT * FROM Orders WHERE OrderID= :order ");
          $stmt->execute([ ':order' => $order ]);

          if (!$row = $stmt->fetch(PDO::FETCH_ASSOC)) {
              echo 'No such ID';
          }
          else {
              $txt .= "Query Received!\n\nOrder ID: ".$order."\n\nMessage context: \n\n".$message;
              mail($mailTo, $subject, $txt);
          }
        }
        else {
                $txt .= "Bug report received!\n\n"."Message context: \n\n".$message;
                mail($mailTo, $subject, $txt);
        }
}
?>

And my HTML:

        <center><form class="query-form" method="post">
 <input style="width: 300px;" class="orderId" type="text" name="orderId" placeholder="Order ID.     Leave blank if reporting a bug">
 <br>
 <input required style="width: 300px;" type="text" name="subject" placeholder="Subject">
 <br>
 <textarea required name="message" placeholder="Query text" style="width: 300px;" maxlength = "700"></textarea>
 <br>
 <input type="submit" name="submit" placeholder="Send Query">
</form></center>

When I fill up orderId input and on purpose type characters that aren't in my table ("test"), it still sends an email ( while it should echo that there is no such order ID provided in input):

Query Received!

Order ID:

Message context:

Test

But when I leave orderId empty, PHP works just fine and gives me second message, as wanted.

Can you please tell me why it's just going through that code?

Community
  • 1
  • 1
Quazey
  • 81
  • 9
  • 2
    According to the documentation for `PDOStatement::fetch()`, "FALSE is returned on failure." Is an empty recordset a failure? – miken32 Mar 04 '20 at 18:16
  • @miken32 can you please explain this. I don't get it – Quazey Mar 04 '20 at 19:14
  • 1
    **WARNING**: When using PDO you should be using [prepared statements](http://php.net/manual/en/pdo.prepared-statements.php) with placeholder values and supply any user data as separate arguments. In this code you have potentially severe [SQL injection bugs](http://bobby-tables.com/). Never use string interpolation or concatenation and instead use [prepared statements](http://php.net/manual/en/pdo.prepared-statements.php) and never put `$_POST`, `$_GET` or any user data directly in your query. Refer to [PHP The Right Way](http://www.phptherightway.com/) for general guidance and advice. – tadman Mar 04 '20 at 19:33
  • Your indentation here is inconsistent to the point it looks like that inner `if` has two `else` clauses. – tadman Mar 04 '20 at 19:34
  • @tadman it looks like, but there is closing } for ````if```` after first ````else````. I would try to do this code with prepared statements. For now, I don't get them at all. – Quazey Mar 04 '20 at 19:57
  • 1
    There's nothing much to "get", you just put things like `:order` in the query in place of `'$order'` and then `execute([ 'order' => $order ])` which 100% takes care of escaping and injection issues for you. – tadman Mar 04 '20 at 20:27
  • @tadman so does ````if (isset($_POST['submit']))```` take care of all $_POST then? If it does, I still need $order in ````if ($order != "")```` to be get so it could do all the job. – Quazey Mar 04 '20 at 21:37
  • 1
    The `ifset()` function only tells you if that value was supplied. It does nothing to prevent SQL injection, for that you *need* placeholder values. Try looking at your PDO result set and see if there's any rows returned before unpacking the data. – tadman Mar 04 '20 at 21:38
  • Have you ever tried var-dumping `$row` to see what you get? – El_Vanja Mar 04 '20 at 22:37
  • @tadman I managed to fix it! Please check out my answer on this question. I don't know if code when sending mail is still vulnerable. Is it? – Quazey Mar 06 '20 at 15:44
  • 1
    It's not the email part, it's the injection into SQL part that's the huge issue. Within the context of *HTML email* you'll want to properly HTML-escape all values so people can't inject arbitrary HTML into your template, like a "name" of `
    LOL
    ` would make your email entirely red. For plain-text you can do whatever, but keep in mind that if people can send arbitrary emails to arbitrary people you *will* inevitably end up a spam vector.
    – tadman Mar 06 '20 at 18:21

1 Answers1

2

Code that fixed my problem was this one

<?php
if (isset($_POST['submit'])) {
    $order = $_POST['orderId'];

    if ($order != "") {
        try {

        $db = new PDO('mysql:host=localhost;dbname=dbname;charset=utf8', 'username', 'password');

        $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
        $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $order = $_POST['orderId'];

        $stmt = $db->prepare("SELECT * FROM Orders where OrderId = :orderid ");
        $stmt->execute([ ':orderid' => $order ]);
        if ($stmt->fetch(PDO::FETCH_ASSOC)) {
            $subject = $_POST['subject'];
            $message = $_POST['message'];
            $order = $_POST['orderId'];
            $mailTo = "mail@mail.com";
            $txt .= "Query Received!\n\nOrder ID: ".$order."\n\nMessage context: \n\n".$message;
            mail($mailTo, $subject, $txt);
        }
        else {
            echo "No such ID.";
        }

        }
        catch (PDOException $e) {
            print "Error!: " . $e->getMessage() . "<br/>";
            die();
        }
    }
    else {
        $subject = $_POST['subject'];
        $message = $_POST['message'];
        $order = $_POST['orderId'];
        $mailTo = "mail@mail.com";
        $txt .= "Report received!\n\n"."Message context: \n\n".$message;
        mail($mailTo, $subject, $txt);
    }
}
?>

Making code to work


Problem with original code was part if (!$row = $stmt->fetch(PDO::FETCH_ASSOC)). It didn't do the job.

That's why, after executing $stmt->execute([ ':orderid' => $order ]);, needed to fetch data searched in table and then if there is such row fetched, send an email. If there is no such row, give an error "No such ID."

if ($stmt->fetch(PDO::FETCH_ASSOC)) {
  $subject = $_POST['subject'];
  $message = $_POST['message'];
  $order = $_POST['orderId'];
  $mailTo = "mail@mail.com";
  $txt .= "Query Received!\n\nOrder ID: ".$order."\n\nMessage context: \n\n".$message;
  mail($mailTo, $subject, $txt);
}
else {
  echo "No such ID.";
}

Also, I have moved part of code that sends email to run separately with all it's variables after doing all the job with searching:

  1. If orderId input is empty or not: if ($order != "")
  2. If orderId input is empty, check if there is actual row in table specified in OrderId input

At the end, used catch, which in coding progress itself helps you to check if code in try works

Read more about prepared statements: https://www.php.net/manual/en/pdo.prepared-statements.php

Read more about PDO connections and connection managing: https://www.php.net/manual/en/pdo.connections.php

Preventing SQL injection


Article How can I prevent SQL injection in PHP?

Using setAttribute() when connecting to database:

$db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

What is mandatory, however, is the first setAttribute() line, which tells PDO to disable emulated prepared statements and use real prepared statements. This makes sure the statement and the values aren't parsed by PHP before sending it to the MySQL server (giving a possible attacker no chance to inject malicious SQL).

Second, using prepared statements when searching for specified row:

$order = $_POST['orderId'];

$stmt = $db->prepare("SELECT * FROM Orders where OrderId = :orderid ");
$stmt->execute([ ':orderid' => $order ]);
Quazey
  • 81
  • 9