PDO MySQL queries for web application to reserve a vehicle

Question

I normally use MySQLi and this is my first time trying out PDO. I am doing this because this is my first web application that is going to be used by the public, hence I want it to be secure. The code I wrote, following a W3 tutorial is working flawlessly, however due to a few other posts on Stack Overflow I have read, I am worried it might not be as secure as I would like.

    <?php

if (isset($_SERVER["HTTP_CF_CONNECTING_IP"])) {
  $_SERVER['REMOTE_ADDR'] = $_SERVER["HTTP_CF_CONNECTING_IP"];
}

include 'dbconfig.php';

    $pickuploc = ($_POST['pickuploc']);
    $droploc = ($_POST['droploc']);
    $pickupdate = ($_POST['pickupdate']);
    $pickuptime = ($_POST['pickuptime']);
    $dropdate = ($_POST['dropdate']);
    $droptime = ($_POST['droptime']);
    $class = ($_POST['cclass']);
    $numofdrivers = ($_POST['numdrivers']);
    $coverage = ($_POST['coverage']);
    $driversage = ($_POST['agedrivers']);
    $roadsideass = ($_POST['roadsideass']);
    $afterhoursdrop = ($_POST['afterhoursdrop']);
    $promo = ($_POST['promo']);
    $fname = ($_POST['fname']);
    $lname = ($_POST['lname']);
    $address = ($_POST['address']);
    $city = ($_POST['city']);
    $state = ($_POST['state']);
    $country = ($_POST['country']);
    $post = ($_POST['post']);
    $dlnum = ($_POST['dlnum']);
    $dlexpm = ($_POST['dlexpm']);
    $dlexpy = ($_POST['dlexpy']);
    $dlcountry = ($_POST['dlcountry']);
    $phone = ($_POST['phone']);
    $email = ($_POST['email']);
    $addcomments = ($_POST['addcomments']);

    $predropdatetime = $dropdate . " " . $droptime;
    $date = new DateTime($predropdatetime);
    $dropdatetime = $date->format('Y-m-d H:i:s');


    $prepickupdatetime = $pickupdate . " " . $pickuptime;
    $date = new DateTime($prepickupdatetime);
    $pickupdatetime = $date->format('Y-m-d H:i:s');

    $date = new DateTime('now');
    $bookingdatetime = $date->format('Y-m-d H:i:s');

    $dlexp =  "20" . $dlexpy . "-" . $dlexpm . "-01";

    if (!empty($_SERVER['HTTP_CLIENT_IP'])) {
        $remoteip = $_SERVER['HTTP_CLIENT_IP'];
    } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
        $remoteip = $_SERVER['HTTP_X_FORWARDED_FOR'];
    } else {
        $remoteip = $_SERVER['REMOTE_ADDR'];
    }

    $date = new DateTime($pickupdate);
    $sqlpickupdate = $date->format('Y-m-d');

    $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);

    $sql = "SELECT COUNT(*) AS num FROM blackout WHERE :pickupdate BETWEEN start_date AND end_date AND city=:pickuploc AND class=:class";

    $stmt = $conn->prepare($sql);

    $stmt->bindValue(':class', $class);
    $stmt->bindValue(':pickupdate', $sqlpickupdate);
    $stmt->bindValue(':pickuploc', $pickuploc);
    $stmt->execute();

    $row = $stmt->fetch(PDO::FETCH_ASSOC);

    if($row['num'] > 0){
        $blackout = true;
    } else{
        $blackout = false;
    }

    $conn = null;

    if ((($dlcountry == "US" || $dlcountry == "JP") && ($driversage == "19" || $driversage == "20") && $coverage == "inclusive") || ($class == "suv" || $class == "minivan" || $class == "convert") && ($driversage == "19" || $driversage == "20") || ($pickuploc == "oc") || ($blackout == true)) {
        echo "RESERVATION NOT BOOKED YOU NEED TO TAKE ACTION!!<br><br><br>";
        $status = "Unconfirmed";
    }else{
        $status = "Confirmed";
    }

    try {
    $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
    // set the PDO error mode to exception
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

       // prepare sql and bind parameters
    $stmt = $conn->prepare("INSERT INTO reservations (bookingdatetime, firstname, lastname, address, city, state, country, post, dlnum, dlexp, dlcountry, phone, email, addcomments, pickuploc, droploc, pickupdatetime, dropdatetime, class, numofdrivers, coverage, driversage, roadsideass, afterhoursdrop, promo, status, remoteip) 
    VALUES (:bookingdatetime, :firstname, :lastname, :address, :city, :state, :country, :post, :dlnum, :dlexp, :dlcountry, :phone, :email, :addcomments, :pickuploc, :droploc, :pickupdatetime, :dropdatetime, :class, :numofdrivers, :coverage, :driversage, :roadsideass, :afterhoursdrop, :promo, :status, :remoteip)");

    $stmt->bindParam(':bookingdatetime', $bookingdatetime);
    $stmt->bindParam(':firstname', $fname);
    $stmt->bindParam(':lastname', $lname);
    $stmt->bindParam(':address', $address);
    $stmt->bindParam(':city', $city);
    $stmt->bindParam(':state', $state);
    $stmt->bindParam(':country', $country);
    $stmt->bindParam(':post', $post);
    $stmt->bindParam(':dlnum', $dlnum);
    $stmt->bindParam(':dlexp', $dlexp);
    $stmt->bindParam(':dlcountry', $dlcountry);
    $stmt->bindParam(':phone', $phone);
    $stmt->bindParam(':email', $email);
    $stmt->bindParam(':addcomments', $addcomments);
    $stmt->bindParam(':pickuploc', $pickuploc);
    $stmt->bindParam(':droploc', $droploc);
    $stmt->bindParam(':pickupdatetime', $pickupdatetime);
    $stmt->bindParam(':dropdatetime', $dropdatetime);
    $stmt->bindParam(':class', $class);
    $stmt->bindParam(':numofdrivers', $numofdrivers);
    $stmt->bindParam(':coverage', $coverage);
    $stmt->bindParam(':driversage', $driversage);
    $stmt->bindParam(':roadsideass', $roadsideass);
    $stmt->bindParam(':afterhoursdrop', $afterhoursdrop);
    $stmt->bindParam(':promo', $promo);
    $stmt->bindParam(':status', $status);
    $stmt->bindParam(':remoteip', $remoteip);


    // insert a row

    $stmt->execute();

    echo "New records created successfully";
    }
    catch(PDOException $e)
    {
    echo "Database Error: " . $e->getMessage();
    }
    $conn = null;


echo "pick up location: " . $pickuploc . "<br> drop off locaiton: " . $droploc . "<br>pick up date: " . $pickupdate . "<br>pick up time: " . $pickuptime . "<br>drop date: " . $dropdate . "<br>drop time: " . $droptime . "<br>class: " . $class . "<br> number of drivers: " . $numofdrivers . "<br> coverage: " . $coverage . "<br>drivers age: " . $driversage . "<br>roadside assitance: " . $roadsideass . "<br>after hours drop:" . $afterhoursdrop . "<br>promo: " . $promo ."<br><br>first name: " . $fname . "<br>last name: " . $lname . "<br>address: " . $address . "<br>city: " . $city . "<br>state: " . $state . "<br>country: " . $country . "<br>post code: " . $post . "<br>dl number: " . $dlnum . "<br>dl exp month: " . $dlexpm . "<br>dl exp yesr: " . $dlexpy . "<br>dl country: " . $dlcountry . "<br>phone: " . $phone . "<br>email: " . $email . "<br>add comments: " . $addcomments;

Answer 1

You are missing charset in your DSN and you have emulated prepared statements still on. This could potentially leave your vulnerable to SQL injection. See Are PDO prepared statements sufficient to prevent SQL injection?
As I have pointed out to you in an answer on Stack Overflow you should set the proper charset and disable emulated prepared statements.

$options = [
    \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES => false,
];
$conn = new \PDO("mysql:host=$servername;dbname=$dbname;charset=utf8mb4", $username, $password, $options);

I do not know why you have 2 PDO connections opened, you need only one.

Also, you should enable error reporting for PDO and avoid using try-catch in your code, if all you do is display the error message on the screen.

Using bindParam seems completely unnecessary. It would be much more readable to pass the values in execute()

Brackets around variables are just noise and serve no purpose. e.g. ($_POST['addcomments'])

Answer 2

w3fools

following a W3 tutorial

I would say the most important suggestion you can get is never ever use any tutorial from w3shcools. They are dubbed as "w3fools" for a reason. Their tutorials are outdated, buggy and - as you can already see from other answers - far from being optimal making you a monkey writing a lot of useless code that repeats again and again.

IP address detection jiggery-pokery

Any $_SERVER array element that begins from HTTP_ is filled directly form the HTTP header. It means that to spoof it is a no-brainier at all.

You have two code blocks that effectively fill the IP address from the user input, instead of taking it from the hardware protocol. You have to get rid of the second code block completely, and make the first one unconditional. Just make your mind whether your code is behind Cloudfare or not, and either add HTTP_CF_CONNECTING_IP processing or leave REMOTE_ADDR alone accordingly.

Just learn from this educational story, How I hacked Stack Overflow and never step on this rake again.

Other issues such as wrong error reporting, double connection and overall inefficiency are already reviewed in other excellent answers.

Answer 3

1. w3schools gets its placement from shelling out big bucks for high ranking in search engine results (as I've read), not for being the best resource for web development advice. When in doubt, read Stack Exchanges sites. SE sites enjoy the benefit of constant peer review. If something is not quite right, eventually a whistleblower is going to find it and call it out.
2. There is no benefit to the parenthetical wrapping of the POST values -- this can be safely omitted. When declaring variables, use camelCase or snake_case for improved readability -- if done consistently through a project, you'll thank yourself later.

3. Your COUNT() query is looking for a true|false determination; this can be simplified using this technique: https://stackoverflow.com/a/37958929/2943403 As a personal preference, passing an array to execute() is a superior syntax because it is less verbose.

   $stmt = $conn->prepare(
       "SELECT 1
        FROM blackout
        WHERE :pickupdate BETWEEN start_date AND end_date
          AND city = :pickuploc
          AND class = :class"
   );
   $stmt->execute([
       'class' => $class,
       'pickupdate' => $sqlPickUpDate,
       'pickuploc'=> $pickUpLoc,
   ]);
   $blackout = (bool)$stmt->fetchColumn();
   

4. Definitely re-use the same connection variable, there is no benefit in grabbing another.

5. Write your simplest / less-intensive conditions first and progress to expressions that involve function calls or relatively heavier processing. Using in_array() will spare your conditional expressions from being overloaded with lots of &&s and ||s. Don't let your line width stretch on for too long, try to avoid doing a lot of horizontal scrolling when writing/reading your scripts. For my liking, your "second half" of conditions when determining if booking action is required, needs an additional set of parentheses to make the logic absolutely clear to humans. I'm actually not going to touch that ordering of the conditions because I don't want to accidentally break the logic, but if this was my script, I would go further to clear up what is happening.

   if (
       ($coverage == 'inclusive' &&
        in_array($dlCountry, ['US', 'JP']) &&
        in_array($driversAge, [19, 20])
       ) ||
       in_array($class, ['suv', 'minivan', 'convert']) &&
       in_array($driversAge, [19, 20]) ||
       $pickuploc == 'oc' ||
       $blackout
   ) {
   

6. Again with the INSERT query, I recommend declaring an associative array instead of all of those bindParam() calls.
7. You must never, ever display db error messages to the end user in production. When you are developing, it may be okay, but if you provide those precise details to someone with malicious intentions, things can go very badly for you and your project.
8. If you are going to just <br> delimit your data and print to screen, it will make things a bit simpler and more scalable to set up an array and implode the values with <br> as glue. Again, try not to write excessively wide code.

Overall, I'd say you are doing quite well -- I've seen far worse attempts. You are using prepared statements and datetime objects, so you are on the right path.

Answer 4

You got the SQL part quite well. If you only had been as careful when generating the HTML.

At the very least, you should wrap every bit of output through htmlspecialchars. I'm sure there are better ways, but that's the basic building block, and you should learn it.

function html_println($label, $value) {
    echo htmlspecialchars($label) . ": " . htmlspecialchars($value) . "<br>\n";
}

html_println("name", $name);
html_println("class", $class);
// and so on