0

I thought you guys would know the best way to do this:

When I delete an order ($prodId) from the ORDERS table, this script then goes and deletes all the items-ordered lines from the ORDERED_ITEMS table, which houses all the items ordered from every order in the system.

Is there a best practice to ensure that what I want deleted is deleted and only that? I'm worried about something going wrong/injected/mistyped with/into the script and accidentally deleting all the ordered item lines for all orders by mistake.

This is how far I got.

$delete_prod_items = mysqli_real_escape_string($con,$_REQUEST['prodId']);
if (is_numeric($delete_prod_items)){
    $sql3 = "DELETE from proteus.ordered_items where order_id = $orderId";
    mysqli_query($con,$sql3) or die('DELETE Order $orderId from the Ordered Items table failed: ' . mysqli_error($con).'<br>');     
}
  • This script is POSTed into by my form.
  • $orderID is the order number that the script uses to identify which ITEM rows should be deleted
  • $delete_prod_item is the escaped $prodID value. I was trying to be super cautious. perhaps I don't need this.

Am I missing anything?

PillBoxCharger
  • 193
  • 3
  • 12
  • Yes, where is `$delete_prod_items` used? And what is `$orderId`? – John Conde Apr 08 '14 at 00:43
  • 1
    Another best practice is to perform any modifications using `POST`, not `GET`. So `REQUEST` here is a bad practice. – zerkms Apr 08 '14 at 00:44
  • $prodID is the order number. I am deleting all rows in the ORDERED_ITEMS table that has an ORDER_ID = $prodID. $delete_prod_items is my escaped $prodID number, trying to clean it out...in case. – PillBoxCharger Apr 08 '14 at 00:45
  • Consider using UPDATE instead of DELETE, and just set a flag to 'deleted' – Strawberry Apr 08 '14 at 00:59

4 Answers4

2

Don't know why anyone is mentioning it, but the best way to really protect any statement is using PreparedStatements:

$delete_prod_items = mysqli_real_escape_string($con,$_REQUEST['prodId']);
$mysqli = new mysqli("localhost", "localuser", "password", "database");
if ($mysqli->connect_errno) {
    echo "Failed to connect to MySQL: " . $mysqli->connect_error;
}
if (!($stmt = $mysqli->prepare("DELETE FROM `proteus`.`ordered_items` WHERE `order_id` = ?"))) { //whatever query you want
    echo "Prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
}
$stmt->bind_param("s", $delete_prod_items);
$stmt->execute();
$stmt->close();

Also take after what @zerkms mentioned and use POST requests for your information.

Rogue
  • 11,105
  • 5
  • 45
  • 71
1

First things first you need to take care of the sql injections. The link will give you an idea.

Secondly you could use javascript to get a pop-up which asks the user a confirmation before deletion.

Next, to avoid the unintentional deletion of more than one row is to include LIMIT 1 to your query.

N.B. You could also limit priveleges by creating a different user (username) to access the mysql database use it in your mysql_connect('host', 'username', 'password', 'database') function . If you are displaying something really important you may consider not giving deletion rights.

Community
  • 1
  • 1
Vagabond
  • 877
  • 1
  • 10
  • 23
0
  • Escaping data is good for preventing SQL Injection
  • It's better to limit the request with $_POST instead of $_REQUEST
  • If this page is for admin only, then it's fine since only you have access, however, if users have access to it, therefore it's better to apply some condition to the request before proceeding to the deletion (example, verify that they are deleting something related to their accounts only)
CMPS
  • 7,733
  • 4
  • 28
  • 53
0

As a first order of business, do the following...

  • Limit the request method to POST or even better, DELETE if you can get PHP to handle it.
  • Use prepared statements with bound parameters to avoid SQL injection vulnerabilities.

The main issue you will face is unauthorised requests. The best solution to this is to use a CSRF token.

Phil
  • 157,677
  • 23
  • 242
  • 245