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:

 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