-2

I am trying to delete a product listing from my shirt_types table (which is tee-shirt products). I have an administrator page that list all the items in the table along with there information. I have added a delete link at the end of the columns for each item. When I click the delete button it redirects me to the shirt_delete page like wanted, but then nothing. It includes the header, then the rest of the page is blank. I think at the very least, the header and the footer should be displayed but this is not the case. Below is the code I used is list_shirts:

$select_shirts = "SELECT shirt_type, shirt_quantity, shirt_color, price, shirt_description, photo, shirt_types_id from shirt_types order by $sort";
$exec_select_shirts = @mysqli_query($link, $select_shirts);
if(!$exec_select_shirts){
    echo "The shirt types information could not be retrieved from the shirt_types table because of: ".mysqli_error($link);
    mysqli_close($link);
    include('footer_admin.php');
    die();
} else {
echo "<div id='list_users'><table id='list_user' border='0'>";
    echo "<tr>";
        echo "<th><a href='".$_SERVER['PHP_SELF']."?sort=size&bool=".!$bool."'>Size</a></th>";
        echo "<th><a href='".$_SERVER['PHP_SELF']."?sort=qnty&bool=".!$bool."'>Quantity</a></th>";
        echo "<th><a href='".$_SERVER['PHP_SELF']."?sort=color&bool=".!$bool."'>Color</a></th>";
        echo "<th><a href='".$_SERVER['PHP_SELF']."?sort=price&bool=".!$bool."'>Price</a></th>";
        echo "<th><a href='".$_SERVER['PHP_SELF']."?sort=desc&bool=".!$bool."'>Description</a></th>";
        echo "<th><a href='".$_SERVER['PHP_SELF']."?sort=photo&bool=".!$bool."'>Photo</a></th>";
        echo "<th>Delete</th>";
    echo "</tr>";
while ($one_row = mysqli_fetch_assoc($exec_select_shirts)) {
    echo "<tr>";
        echo "<td class='first'>".$one_row['shirt_type']."</td>";
        echo "<td class='second'>".$one_row['shirt_quantity']."</td>";
        echo "<td class='first'>".$one_row['shirt_color']."</td>";
        echo "<td class='second'>".$one_row['price']."</td>";
        echo "<td class='first'>".$one_row['shirt_description']."</td>";
        echo "<td class='second'><img src='./images/".$one_row['photo']."' /></td>";
        echo "<td class='first'><a href='shirt_delete.php?shirt_types_id=".$one_row['shirt_types_id']."'>Delete</a></td>";
    echo "</tr>";
}

and here is the shirt_delete.php file that I am attempting to use to delete the shirts and their information from the database.

<?php
require('mysql_connect.php');
session_start();

if (isset($_SESSION['shirt_users_id']) && isset($_SESSION['full_name'])) {
$title="Delete Shirts Page";
include_once("header_admin.php");

if(!empty($_GET['shirt_types_id'])){
$shirt_types_id = $_GET['shirt_types_id'];
mysqli_query($link, "SET AUTOCOMMIT = 0");
$del_shirt_users_id = "DELETE shirt_types.*
            FROM shirt_types
            WHERE shirt_types_id = $shirt_types_id";
$$del_shirt_types_id = @mysqli_query($link, $del_shirt_types_id);
if(!$$del_shirt_types_id){
    rollback(mysqli_error($link));
}else{
    mysqli_query($link, "COMMIT");
    header('refresh: 0; url=list_shirts.php');
}
}else{
echo "Problem occurred";
header('refresh: 3; url=list_shirts.php');
}

} else {
echo "You are not an authentic administrator. Being directed to the login page...";
header("Refresh: 2; url='login.php'");  
}

mysqli_close($link);
require("footer.php");
die();
?>

NOTE: I understand that SQL injection is a real thing and in a real world application that this code would not suffice. But this is a part one course of a three part series. We are not to worry about sql injection at the present moment in time. Thank you everyone for your suggestions and worries about this though!

NerdsRaisedHand
  • 311
  • 2
  • 3
  • 9
  • 1
    $del_shirt_users_id = "DELETE FROM shirt_types WHERE shirt_types_id = $shirt_types_id"; – Krish R Dec 05 '13 at 17:23
  • You should start with adding error handling and removing the `@` error supressing operator. And fixing the sql injection problem. – jeroen Dec 05 '13 at 17:24
  • your code is wide open to SQL injection specially since it's now public. Just picture what happens if someone manufactures the following URL: `shirt_delete.php?shirt_types_id=1+OR+1=1`. Your query becomes `DELETE shirt_types.* FROM shirt_types WHERE shirt_types_id = 1 OR 1=1` which will delete your entire shirt_types table! Do yourself a favour and learn how to prevent SQL injection before you release any such code to the public: http://php.net/manual/en/security.database.sql-injection.php – svoop Dec 05 '13 at 17:27
  • @svoop I've removed the 'duh' from your comment. As Andy noted above, it's a useful comment otherwise. – Andrew Barber Dec 05 '13 at 20:38
  • @andy You're right, just looked it up in the urban dict and I must admit that I though it had a different meaning. (I'm not a native English speaker.) Thanks for removing it. – svoop Dec 06 '13 at 09:42

2 Answers2

2

Try using, Remove shirt_types.*

$del_shirt_users_id = "DELETE  FROM shirt_types
        WHERE shirt_types_id = $shirt_types_id";

instead of

$del_shirt_users_id = "DELETE shirt_types.*
        FROM shirt_types
        WHERE shirt_types_id = $shirt_types_id";

and also, change :

 $del_shirt_types_id = @mysqli_query($link, $del_shirt_types_id);
 if(!$del_shirt_types_id){

instead of

$$del_shirt_types_id = @mysqli_query($link, $del_shirt_types_id);
if(!$$del_shirt_types_id){
Krish R
  • 22,583
  • 7
  • 50
  • 59
  • Ive tried your suggestion, but I still cannot tell if its working because when delete_shirt.php is loaded, just the header appears, and I am never redirected to list_shirts.php and the footer still does not even appear? – NerdsRaisedHand Dec 05 '13 at 17:27
  • Thank you User876345 The stupid double $$ was the mistake that I was looking for! everything works now! – NerdsRaisedHand Dec 05 '13 at 17:36
2

By building SQL statements with outside variables, you are leaving yourself wide open to SQL injection attacks.

In your specific case, if someone shirt_types_id with a value of "0 or (1=1)", then the SQL that you will create will look roughly like this:

DELETE FROM shirt_types WHERE shirt_types_id = 0 or (1=1)

and since 1=1 is always true, then you will delete every shirt_types record.

Please learn about using parametrized queries, preferably with the PDO module, to protect your web app. http://bobby-tables.com/php has examples to get you started, and this question has many examples in detail.

Community
  • 1
  • 1
Andy Lester
  • 91,102
  • 13
  • 100
  • 152
  • Very true, Andy, couldn't agree more! And I'm shocked other answers to this question don't mention this at all. – svoop Dec 05 '13 at 17:31
  • Thank you for this, I have edited my question to include a note that sql injection is not something were required to worrie about for this specific project – NerdsRaisedHand Dec 05 '13 at 17:32
  • Even without worrying about SQL injection, there are many good reasons to use parametrized queries. They are there to help you reduce errors as well as be more secure. It's frustrating that you're taking a class at some learning institution and they're teaching you bad habits from the start. – Andy Lester Dec 05 '13 at 17:34
  • If you only knew how bad of professors we have... They still teach all depreciated things in every language. As well as do no error handling... honestly they shouldnt even be allowed to teach. – NerdsRaisedHand Dec 05 '13 at 17:39