Please help with new features in my login php script - Suggestions Welcome

the_infamous

Hello. I have made a simple php script but would like to add some nice features to it such as a way of monitoring the sessions and timing out users after a specific time. Also, I would like the customer interface to be the default. Any other suggestions of improving the script is welcome. Currently, the script is not secure in terms of the admin logging in and editing stock. Of course, I would like a secure way of the admin logging in with the admin name and password. Thanks in advance.

<?php

   //Connect with current sesssion;
   session_start();
   session_register("status");
   session_register("owner_confirmed");
   session_register("totalitems");
   session_register("cart");

   //Default status: customer
   if (!isset($status)) $status="customer";

   //Extract user instructions
   extract($_POST);

   //Determine user status
   if (isset($changestatus))
   {
      if ($status=="owner") $status="customer";
      else $status="owner";
   }

   //Continue shopping after purchase
   if (isset($continue))
   {
      foreach($cart as $idx=>$num) unset($cart[$idx]);
      unset($cart);
   }

   //create stock table
   create_stock_table();

   if ($status=="owner")
   {
      if (!isset($owner_confirmed))
      {
         //Owner Functionality
         if (!isset($passwordenter))
         {
            //Check Password
            ?>
            <html>
            <head><title>Owner Verification</title></head>
            <body>
               <form action="<?=$SERVER["PHP_SELF"]?>" method="post">
                  <p>Enter Password:<input type="password" name="password"/>
                  <input type="submit" name="passwordenter" value="Enter"/</p>
               </form>
            </body>
            <?php die();
         }
         else
         {
            if ($password!="Hello")
            { ?>
               <head><title>Verification Failed</title></head>
               <body><p>Verification Failure</p></body>
               <?php die();
            }
            else $owner_confirmed="true";
         }
      }

  //Add new item to database
  if ($newitem) new_stock_item($author,$title,$price,$number);
   }
   else
   {
      //Customer Functionality

  //Obtain database entry for selected item index
  $item=get_stock_item($itemindex);

  //Edit cart contents
  if ($additem)
  {
     //Add item to cart
     if (!isset($cart[$itemindex])) $cart[$itemindex]=1;
     else $cart[$itemindex]++;
     $totalitems++;
  }
  if ($delitem)
  {
     //Remove item from cart
     if (isset($cart[$itemindex]))
     {
        if ($cart[$itemindex]>1) $cart[$itemindex]--;
        else unset($cart[$itemindex]);
        $totalitems--;
        if ($totalitems==0) unset($cart);
     }
  }
   }
?>

<html>
   <head><title>Bookshop</title></head>
   <body>
      <form action="<?=$SERVER["PHP_SELF"]?>" method="post">
         <?php if ($status=="customer")
         { ?>
            <h2>Customer Interface</h2>
            <?php if ($checkout)
            { ?>
               <h3>Checkout:</h3>
               <?php generate_order($cart); ?>
               <p><input type="submit" name="continue" value="Continue shopping"/></p>
            <?php }
            else
            { ?>
               <h3>Items Currently In Stock:</h3>
               <?php list_stock();?>
               <p>
                  Index Number:<input type="text" name="itemindex"/>
                  <input type="submit" name="additem" value="Add"/>
                  <input type="submit" name="delitem" value="Remove"/>
               </p>
               <h3>Items In Shopping Cart:</h3>
               <?php list_cart($cart); ?>
               <p>
                  <input type="submit" name="checkout" value="Proceed to checkout"/>
                  <input type="submit" name="changestatus" value="Log in as owner"/>
               </p>
            <?php }
         }
         else
         { ?>
            <h2>Owner Interface</h2>
            <h3>Add New Item:</h3>
            <p>
               Author:<input type="text" name="author"/>
               Title:<input type="text" name="title"/>
               Price:<input type="text" name="price"/>
               Number:<input type="text" name="number"/>
            </p>
            <p><input type="submit" name="newitem" value="Add item to database"/></p>
            <p><input type="submit" name="changestatus" value="Log in as customer"/></p>
         <?php } ?>
      </form>
   </body>
</html>

<?php

   function create_stock_table()
   {
      //Creates a new stock table (if one does not already exist)
      $conn=connect();
      mysql_query("CREATE TABLE Stock (indx INT(3) NOT NULL PRIMARY KEY AUTO_INCREMENT,
                                       author VARCHAR(30),
                                       title VARCHAR(30),
                                       price VARCHAR(5),
                                       number VARCHAR(5))");
      disconnect($conn);
   }

   function new_stock_item($author,$title,$price,$number)
   {
      //Add new item to stock table
      $conn=connect();
      mysql_query("INSERT INTO Stock (author,title,price,number)
                   VALUES ('$author','$title','$price','$number')");
      disconnect($conn);
   }

   function list_stock()
   {
      //Display stock table
      $conn=connect();
      $res_table=mysql_query("SELECT * FROM Stock");
      if ($res_table)
      {
         print("<table border='1'><thead><th>Index</th><th>Author</th><th>Title</th><th>Price</th><th>Number</th></thead><tbody>");
         while ($item_array=mysql_fetch_array($res_table))
         {
            extract($item_array);
            print("<tr><td>$indx</td><td>$author</td><td>$title</td><td>$price</td><td>$number</td><tr>");
         }
         print("</tbody></table>");
      }
      disconnect($conn);
   }

   function list_cart($cart)
   {
      if (isset($cart))
      {
         print("<table border='1'><thead><th>Index</th><th>Title</th><th>Price</th><th>Number</th></thead><tbody>");
         foreach ($cart as $idx=>$num)
         {
            $item=get_stock_item($idx);
            extract($item);
            print("<tr><td>$indx</td><td>$title</td><td>$price</td><td>$num</td></tr>");
         }
         print("</tbody></table>");
      }
      else print("<p>The cart is currently empty</p>");
   }

   function generate_order($cart)
   {
      if (isset($cart))
      {
         print("<table border='1'><thead><th>Index</th><th>Title</th><th>Price</th><th>Number</th></thead><tbody>");
         $total=0;
         foreach ($cart as $idx=>$num)
         {
            //Obtain details of cart item
            $item=get_stock_item($idx);
            extract($item);

        //Check that enough are in stock
        if ($number>=$num)
        {
           $newnumber=$number-$num;
           update($idx,$newnumber);
        }
        else
        {
           $num=$number;
           update($idx,0);
           $notenough="true";
        }
        $total+=$num*$price;
        print("<tr><td>$indx</td><td>$title</td><td>$price</td><td>$num</td></tr>");
     }
     print("<tr><td colspan='3'>Total Price (£):</td><td>$total</td></tr></tbody></table>");
     if (isset($notenough)) print("<p>We regret there is insufficient stock to cover all of your order</p>");
  }
  else print("<p>The cart is currently empty</p>");
   }

   function get_stock_item($index)
   {
      //Obtain the details of a specified stock item
      $conn=connect();
      $res_table=mysql_query("SELECT * FROM Stock WHERE indx=$index");
      if ($res_table) $item_array=mysql_fetch_array($res_table);
      disconnect($conn);
      return $item_array;
   }

   function update($index,$number)
   {
      //Update quantity of an item in stock
      $conn=connect();
      mysql_query("UPDATE Stock SET number='$number' WHERE indx='$index'");
      disconnect($conn);
   }

   function connect()
   {
      //Connect to database
      $connection=mysql_connect("ftemysql","ku12881","carstairs");
      mysql_select_db("ku12881",$connection);
      return $connection;
   }

   function disconnect($connection)
   {
      //Disconnect from database
      mysql_close($connection);
   }
?>

rmbarnes82

Hi,

Your script does seem a bit confused, so I will give you some ideas on how I would improve it. The following is a list of what I think the problems are, and possible solutions.

Your script does too much stuff for one script.

If I am reading it correctly, your script does the following:

Allows the users to browse products and add them to their cart
Provides the admin user with the abillity to log in
Allows the admin user the ability to edit stock

All this functionality is to much to contain within one file - it makes the file hard to read and a nightmare to maintain... If your site grows to include more functionality in the future the script could grow.... imagine trying to maintain a 2000 line script....

The soultion: put each seperate bit of functionality into a seperate script. You can make use of includes to include multiple files at runtime (see http://uk.php.net/manual/en/function.require-once.php for details on how to include files).

I would have you site work like this:

The customers use www.mysite.com/index.php - I suppose this would default to the products page of some sort.

The admin user would use www.mysite.com/admin/index.php this would default to the admin log in page unless the admin was already logged in.

The index.php file would just be a controller to run other scripts, and would include scripts as needed. For example the login page would be www.mysite.com/admin/index.php?c=login

The index.php script would recieve a get varibale: $_GET['c'] which would contain 'login', based on this you know to do a require_once on the login script to run it.

You seem to try to create the stock table each time the script runs - why?

Soloution: just create it once (say using phpmyadmin).

You use extract($POST). This is bad because say you have a post variable called quantity. After using extract you will just reference it as $quantity instead of $POST['quantity']. This means when you come to modify your script a few months after first writing it, you'll be thinking 'where the hell does $quantity come from!?!'.

Solution: take the time to type out $_POST['quantity'] each time. That way when you see it in the code, you will always know that it was posted by the user from a post form.

Using session_register

Yeah this one, when coupled with using extract($_POST) has caused a massive security flaw in your script. To demonstrate this, what would happen is I, on my pc create an html post form that submits to your sites script, and contains a form field called owner_confirmed and a value of 1.... I think I am right in saying that this would give me admin access - without a password.

Solution: use $SESSION super global array instead of session register. You still need session_start(), but now access session variables using the $SESSION array. You could then set owner_confirmed like this in your script: $SESSION['owner_confirmed'] = 1, then refer to it as $SESSION['owner_confirmed']. This means it is both more secure and more readable as you know the value you are reading has come from the session.

Let me know how you get on

Robin

bpat1434

Timing Out Users Use Javascript to set up a hidden timer to count down from say 15 minutes to 0. When it hits 0, redirect them to the logout page.

Customer interface default So set it as such. Just set a session var that they're admin. Or make them log-in double: once to get into the customer view, once for admin. Much like any forum software out there.

Secure admin log-in See previous point

Other suggestions DO NOT USE [man]extract/man!! Especially don't use it if you're not going to check what people have put in there. That's the biggest security issue. There's no sanity check on your inputs. Right now, someone could totally hack into it by way of using an SQL injection technique. Plus other ways with [man]extract/man.

I'd suggest reading Chris Shiflett's book (phpsecurity.org) on PHP Security. The first couple chapters are form inputs and such.

the_infamous

Thanks, have kept your comments in mind. Cheers Robin