2
\$\begingroup\$

The following code books one appointment for one or more slots. I have never used database before, but have seen so many warnings regarding SQL injections. Since it has never affected me I have not paid attention.

<?php

    include('php/connect.php'); 

    if(isset($_POST['slots_booked'])) $slots_booked = mysqli_real_escape_string($link, $_POST['slots_booked']);
    if(isset($_POST['name'])) $name = mysqli_real_escape_string($link, $_POST['name']);
    if(isset($_POST['akeri'])) $akeri = mysqli_real_escape_string($link, $_POST['akeri']);
    if(isset($_POST['email'])) $email = mysqli_real_escape_string($link, $_POST['email']);
    if(isset($_POST['phone'])) $phone = mysqli_real_escape_string($link, $_POST['phone']);
    if(isset($_POST['typ'])) $typ = mysqli_real_escape_string($link, $_POST['typ']);
    if(isset($_POST['fri'])) $fri = mysqli_real_escape_string($link, $_POST['fri']);
    if(isset($_POST['booking_date'])) $booking_date = mysqli_real_escape_string($link, $_POST['booking_date']);



    $booking_array = array(
            "slots_booked" => $slots_booked,    
            "booking_date" => $booking_date,
            "name" => $name,
            "email" => $email,
            "Åkeri" => $akeri,
            "phone" => $phone,
            "typ" => $typ,
            "fri" => $fri
    );

    $explode = explode('|', $slots_booked);

    foreach($explode as $slot) {

        if(strlen($slot) > 0) {

            $stmt = $link->prepare("INSERT INTO bookings (date, start, name, email, phone, typ, fri) VALUES (?, ?, ?, ?, ?, ?, ?)"); 
            $stmt->bind_param('sssssss', $booking_date, $slot, $name, $email, $phone, $typ, $fri);
            $stmt->execute();


        } // Close if

     } // Close foreach

    header("location: Done.html");
    exit;
?>
\$\endgroup\$
2
\$\begingroup\$

mysqli::real_escape_string(..)

Don't use mysqli::real_escape_string(..) or its procedural variant (because you should not use mysqli::query(..)). You only need to use this if you are writing raw queries. With real prepared queries you don't have to worry about escaping data, because the data is sent separately.

If used, mysqli::real_escape_string(..) is best used on variables that are used as a string in the query, just before they are put into the query. You are doing additional processing on $slots_booked, which could potentially lead to problems if an attacker can force it to split in the middle of an escape sequence.

$booking_array

Why does this variable exist? It is not being used. Remove it.

The happy case

You are only coding for the happy case. That is: If everything went as planned. You should do the following:

  • Validate the input. Is it in a format you expect it to be? Besides that you could get bogus data in your database, the rest of the code can't assume the variables to be correct.
  • Handle all outputs of all methods you call

mysqli::prepare(..)

mysqli::prepare(..) returns either a mysqli_stmt or the boolean false if an error occurs. Guess what happens when you call bind_param on a boolean - it gives an error.

mysqli_stmt::bind_param(..)

This method returns either true or false based on if binding the parameters worked. Since you don't check what's in the variables, or even require variables to be set, this could very well return false.

mysqli_stmt::execute()

This method returns either true or false based on if the query executed correctly. If this returns false, it means your data was not saved. You probably want to at least log that, and probably tell that to the user.

Procedural or OO

Choose either the procedural style or the OO style for consistency in your code. You are currently using both.

Potential output before header

Since you are not preventing obvious errors, some of the methods can write errors to the page before the header(..) call. This causes an additional error, since you can't write headers after the output has started. Additionally, this means that the user is not redirected as expected.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your comments. This code was found on the internet as a booking system, I do not fully understand it. Thus this thread. Booking array. Correct, it's useless, will remove it. slots booked is a string from checkboxes so one checkbox means one value, two means two with pipe in between. So capture all the error bool returns from for each, Ok will do that. How should I validate the inputs? Most of them are strings with name, email and phone. I understand that I can look for phone and email with regex, but are there any better options? What do I need to remove from the strings to make \$\endgroup\$ – Andreas Sep 12 '16 at 10:59
  • \$\begingroup\$ them safe? ( )I guess? $ \$\endgroup\$ – Andreas Sep 12 '16 at 11:01
  • \$\begingroup\$ Use filter_var and appropriate filters. I would in this case not try to sanitize variables, but instead return an error on non-conforming input. \$\endgroup\$ – Sumurai8 Sep 12 '16 at 11:23
4
\$\begingroup\$

Code has to be readable

  if(isset($_POST['slots_booked'])) $slots_booked = mysqli_real_escape_string($link, $_POST['slots_booked']);

Going through this code I really have to focus to try to figure out what is happening, there is no 'flow'. Now add 7 of those and you got some serious reading ahead of you. You are using prepared statements so don't escape your values 2 times, prepared statements protect from sql injection.

Use meaningful english names

Database columns, variable names etc have to be in english and have to tell what they are responsible for.

  $booking_array = array(
  "slots_booked" => $slots_booked, // not entirely clear what this column stores
  "booking_date" => $booking_date,
  "name" => $name,
  "email" => $email,
  "Åkeri" => $akeri, // lets open up google translate
  "phone" => $phone,
  "typ" => $typ, // no idea what this column stores
  "fri" => $fri // no idea what this column stores
  );

As @Sumurai8 noted: $booking_array is not even used yet it takes up 30% of the code.

This one is just left there hanging. I suppose $slots_booked are an array of some slots? No idea what those slots represent. Variable name $explode will not help with understanding code it is used in.

 $explode = explode('|', $slots_booked);

Foreach does not start that bad. $explode is array of $slot's, it asks for renaming $explode to $slots.

What kind values do you expect $slot's to have? Checking empty($slot) does not cut it?

How short preparation of query is! Just 3 lines of code. It's great isn't it? Not necessarily. You sacrificed code's readability by making it so short. Each parameter should have proper placeholder name => :name, start => :start. I would recommend providing bindParam() with type you expect given param to have.

Here we are given another clue as to what $slot might be. It is start of bookings. Maybe instead of $slot something like $booking_start would be more understandable?

foreach($explode as $slot) {

    if(strlen($slot) > 0) {

        $stmt = $link->prepare("INSERT INTO bookings (date, start, name, email, phone, typ, fri) VALUES (?, ?, ?, ?, ?, ?, ?)");
        $stmt->bind_param('sssssss', $booking_date, $slot, $name, $email, $phone, $typ, $fri);
        $stmt->execute();


    } // Close if

} // Close foreach

Below code is example in which way your code can go and be more readable. DB object was taken from here.

class Booking {
    public $start;
    public $name;
    public $akeri;
    public $email;
    public $phone;
    public $typ;
    public $fri;
    public $date;

    public function save() {
        $stmt = DB::prepare("INSERT INTO bookings (date, name) VALUES (:date, :name)");
        $stmt->bind_param(':date', $this->date);
        $stmt->bind_param(':name', $this->name);
        // rest of attributes
        $stmt->execute();
    }
}

$bookingDates = explode('|', $_POST['slots_booked']);
foreach($bookingDates as $bookingDate) {
    $booking = new Booking();
    $booking->name = $_POST['name'];
    // set rest of attributes

    $booking->save();
}

EDIT

Changed $bookingDetails to $bookingDates

\$\endgroup\$
0
\$\begingroup\$

I have included some thoughts with code context below:

include('php/connect.php');

Consider using require() here as my guess is that this script will not execute without this dependency. You should fail fast if dependencies are not met.


if(isset($_POST['slots_booked'])) $slots_booked = mysqli_real_escape_string($link, $_POST['slots_booked']);
if(isset($_POST['name'])) $name = mysqli_real_escape_string($link, $_POST['name']);
if(isset($_POST['akeri'])) $akeri = mysqli_real_escape_string($link, $_POST['akeri']);
if(isset($_POST['email'])) $email = mysqli_real_escape_string($link, $_POST['email']);
if(isset($_POST['phone'])) $phone = mysqli_real_escape_string($link, $_POST['phone']);
if(isset($_POST['typ'])) $typ = mysqli_real_escape_string($link, $_POST['typ']);
if(isset($_POST['fri'])) $fri = mysqli_real_escape_string($link, $_POST['fri']);
if(isset($_POST['booking_date'])) $booking_date = mysqli_real_escape_string($link, $_POST['booking_date']);

Validate this data before working with it. Validate and cast the string POST input to appropriate values for use within the script. I.e. validate email format, validate/cast numeric data types, validate non-zero lengths strings, etc.

Take a look at PHP's filter_input_array for good example of how you can better approach this:

http://php.net/manual/en/function.filter-input-array.php

Also in this section, lines of code are too long. You should try to keep lines less than 80 characters in length. Use curly braces around code inside conditional for all but most trivial bits of logic to improve readability of code.

Example:

if(isset($_POST['slots_booked'])) {
    $slots_booked = mysqli_real_escape_string($link, $_POST['slots_booked']);
}
if(isset($_POST['name'])) {
    $name = mysqli_real_escape_string($link, $_POST['name']);
}
// etc.

If you properly validate the data up front and use in combination with prepared statements, you don't need to perform mysqli_real_escape_string() on the data.


$booking_array = array(
        "slots_booked" => $slots_booked,    
        "booking_date" => $booking_date,
        "name" => $name,
        "email" => $email,
        "Åkeri" => $akeri,
        "phone" => $phone,
        "typ" => $typ,
        "fri" => $fri
);

Where is this used?


$explode = explode('|', $slots_booked);

Reconsider how you are POSTing data for this field. If you post it as an array using array notation like:

<input name="slots_booked[]" ...>

then you can run your validation filter against each individual element of the array up in the validation steps and already have an array to work with, eliminating the need for this explode() call.


foreach($explode as $slot) {

Your prepared statement initialization should happen before this foreach loop. Prepared statements were meant to leverage this use case, allowing a single prepare, then executing it multiple times. This skips some overhead on the MySQL end with not having to make a query plan for each execution. You can also perform your parameter binding here as well, as there is no need to perform this operation multiple times either.


    if(strlen($slot) > 0) {

You should have already validated this input before ever getting to the point of trying to make inserts in the database. Reference earlier notes on data validation.


         $stmt = $link->prepare("INSERT INTO bookings (date, start, name, email, phone, typ, fri) VALUES (?, ?, ?, ?, ?, ?, ?)");

What do typ and fri mean?

Use meaningful field names in your table and variable names in your code. You gain nothing by cutting a field name short other than possible confusion down the line.

Your current prepared statement logic assumes happy path execution. What happens if prepare() fails?


        $stmt->bind_param('sssssss', $booking_date, $slot, $name, $email, $phone, $typ, $fri);

Move this outside loop. Consider parameter typing here. If you truly want to store integers, or doubles in your DB table, then you should enforce that here as well so you are not accidentally invoking potentially unexpected type conversion behavior in the database.

Your current binding logic assumes happy path. What if binding fails?


        $stmt->execute();

The code around query execution only considers happy path. What happens if insert fails? You do nothing to handle this. What happens with other rows that might have already been or will be inserted as you iterate in this loop? Does this booking need to happen transactionally (i.e. all inserts must succeed before committing change)?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.