2

I am writing a PHP script and it appears that I'm having issues with out of order execution. I have a for loop that checks the existence of records in a table with an insert following it. The idea was to keep the insert from happening if the for loop finds an existing record. However, when I run the following script, it would appear that my insert statement is being executed either during or before the for loop since I'm still getting record insertion into reservations even when a matching record exists in reservedTickets that should halt execution. What is the best way to handle this?

PHP:

for ($i=1; $i<=$_POST['numTickets']; $i++)
{
    $checkTID = $_POST["tid".$i];
    if (mysql_fetch_assoc(mysql_query("SELECT EXISTS(SELECT 1 FROM reservedTickets WHERE tid=".$checkTID.") AS 'exists';"))['exists'] == 1)
    {
        header("Location: http://10.12.76.201/reservations/");
    }
}

mysql_query("INSERT INTO reservations (aid, approval) VALUES (".$_POST['aid'].", 0);");
$reservationID = mysql_insert_id(); 
  • not possible. php scripts do not execute lines of code "out of order". you are, however, vulnerable to [sql injection attacks](http://bobby-tables.com), so enjoy having your server pwn3d. – Marc B Aug 10 '15 at 20:16
  • 1
    Header should be set before any output is produced by the script. Put something verbal like echo instead of header to see if your script hits the condition – paulus Aug 10 '15 at 20:17
  • 1
    and note that `header()` doesn't terminate your script, it just tells PHP to output (eventually) a header. Your loop is going to keep running through ALL of the for() loop, then will hit your insert query **REGARDLESS** of the redirect you may/may not have asked PHP to do. – Marc B Aug 10 '15 at 20:17
  • If you can, you should [stop using `mysql_*` functions](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php). They are no longer maintained and are [officially deprecated](https://wiki.php.net/rfc/mysql_deprecation). Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) [statements](http://php.net/manual/en/pdo.prepared-statements.php) instead, and consider using PDO, [it's really not hard](http://jayblanchard.net/demystifying_php_pdo.html). – Jay Blanchard Aug 10 '15 at 20:18
  • @MarcB It's an internal tool for a team of ~10 sales people with a requested dev time of 2 weeks. Figured I could skip some of the required sanitation due to the tight time frame and add it later if necessary. – 99ProblemsAndTheyreAllCode Aug 10 '15 at 20:19
  • 3
    bad attitude. security starts at the beginning. not "meh, we'll deal with it after our system gets totally destroyed". and don't you read dilbert? sales people are the most dangerous things in the universe... and you're giving them the opportunity to stuff whatever they want into your DB queries? – Marc B Aug 10 '15 at 20:20
  • @MarcB, assertions are happening on the form prior to this page executing the POST with restrictions using the `Session` variable to ensure they are only accessing this page from the form that posts to it. You're right though, in my haste I skipped some necessary security steps and I should sanitize my inserts. – 99ProblemsAndTheyreAllCode Aug 10 '15 at 20:24
  • 1
    Rather than sanitize your inserts, I suggest you use parametrized queries. This answer has lots of information on how to do it: https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – Andy Lester Aug 10 '15 at 20:47

1 Answers1

3

The execution is happening after the loop because calling the header() function alone will not halt execution. Rather, it tells PHP to append that response header when the request is complete but doesn't stop any code below it from running.

You should use one of the following constructions:

for ($i=1; $i<=$_POST['numTickets']; $i++)
{
    $checkTID = $_POST["tid".$i];
    if (mysql_fetch_assoc(mysql_query("SELECT EXISTS(SELECT 1 FROM reservedTickets WHERE tid=".$checkTID.") AS 'exists';"))['exists'] == 1)
    {
        header("Location: http://10.12.76.201/reservations/");
        exit;  // STOP script execution, and send the header
    }
}

or

$found = false;
for ($i=1; $i<=$_POST['numTickets']; $i++)
{
    $checkTID = $_POST["tid".$i];
    if (mysql_fetch_assoc(mysql_query("SELECT EXISTS(SELECT 1 FROM reservedTickets WHERE tid=".$checkTID.") AS 'exists';"))['exists'] == 1)
    {
        $found = true;
        break; // exit the for loop
    }
}

if (!$found) {
    mysql_query("INSERT INTO reservations (aid, approval) VALUES (".$_POST['aid'].", 0);");
    $reservationID = mysql_insert_id();
}
drew010
  • 68,777
  • 11
  • 134
  • 162