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:
- Make sure you are logged, in this case as owner. Not sure if the other privileges/roles are affected.
- Call the action by going to route /admin/sales/delete on the browser. That’s right don’t append any ID.
- 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.
- MSAnswered
Thank you very much for reporting this. I am check and shall add the validation soon.
- Login to Reply