Bug Report: Error In Sales Model Causes Deletion Of Full Table

Error In Sales Model Causes Deletion Of Full Table

First, I haven't tested this live on your demo because it could be damaging. However I did downloaded your latest code (today) and I can see that the sales model still has the bug. Reproduc

H

Hugo

Reported
First, I haven't tested this live on your demo because it could be damaging. However I did downloaded your latest code (today) and I can see that the sales model still has the bug.

Reproducing the error:

1. Make sure you are logged, in this case as owner. Not sure if the other privileges/roles are affected.
2. Call the action by going to route /admin/sales/delete on the browser. That's right don't append any ID.
3. That's it all your invoices are now gone. This has happened to me twice. Which is why I went to check the code for a possible vector /vulnerability. It ain't fun to have this occurred on a production live site. Thank God for backups.

The code:

Part of the problem is that the sale_id column is always NULL in sma_sales. Each time the user create an invoice this column is always left with the default NULL value.

The real problem is that the deleteSales() function in the Sales_model.php has two lines of code triggering the delete of the sale one by id and another by sale_id. Here is the code:

```php
public function deleteSale($id)
{
$this->db->trans_start();
$sale_items = $this->resetSaleActions($id);
if ($this->db->delete('sale_items', ['sale_id' => $id]) && $this->db->delete('sales', ['id' => $id]) && $this->db->delete('costing', ['sale_id' => $id])) {
$this->db->delete('sales', ['sale_id' => $id]);
$this->db->delete('payments', ['sale_id' => $id]);
$this->site->syncQuantity(null, null, $sale_items);
}
```

Notice this line: `&& $this->db->delete('sales', ['id' => $id]) &&`
Is an IF with all two AND conditions, if true, it goes again and try to delete the sale here but this time by sale_id: `$this->db->delete('sales', ['sale_id' => $id]);`

Now first this is an IF condition with all conditions using AND so if the delete of sales fails it won't enter so this line: `$this->db->delete('sales', ['sale_id' => $id]);` can be removed. But this specific line is the culprit. Since there is no validation for `$id` been NULL, it goes right through the query builder and basically forms this SQL statement: `DELETE FROM sma_sales WHERE sale_id IS NULL`. Now remember that sale_id is always NULL, so that's basically a NUKE and there goes your data.

A basic fix is to just remove that extra line `$this->db->delete('sales', ['sale_id' => $id]);` from the code. Or to put some validation for $id parameter.
  • MS

    Mian Saleem

    Answered
    Thank you very much for reporting this. I am check and shall add the validation soon.
  • Login to Reply