From 418580a52da2346366f1a385a5f3100fb5a2214b Mon Sep 17 00:00:00 2001
From: jekkos <jekkos@users.noreply.github.com>
Date: Fri, 6 Mar 2026 17:01:38 +0100
Subject: [PATCH] Fix second-order SQL injection in currency_symbol config
 (#4390)

* Fix second-order SQL injection in currency_symbol config

The currency_symbol value was concatenated directly into SQL queries
without proper escaping, allowing SQL injection attacks via the
Summary Discounts report.

Changes:
- Use $this->db->escape() in Summary_discounts::getData() to properly
  escape the currency symbol value before concatenation
- Add htmlspecialchars() validation in Config::postSaveLocale() to
  sanitize the input at storage time
- Add unit tests to verify escaping of malicious inputs

Fixes SQL injection vulnerability described in bug report where
attackers with config permissions could inject arbitrary SQL through
the currency_symbol field.

* Update test to use CIUnitTestCase for consistency

Per code review feedback, updated test to extend CIUnitTestCase
instead of PHPUnit TestCase to maintain consistency with other
tests in the codebase.

---------

Co-authored-by: Ollama <ollama@steganos.dev>
---
 app/Controllers/Config.php                    |  3 +-
 app/Models/Reports/Summary_discounts.php      |  5 +-
 .../Models/Reports/Summary_discounts_test.php | 51 +++++++++++++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100644 tests/Models/Reports/Summary_discounts_test.php

diff --git a/app/Controllers/Config.php b/app/Controllers/Config.php
index 820ee11c8..f1d63ed4c 100644
--- a/app/Controllers/Config.php
+++ b/app/Controllers/Config.php
@@ -461,8 +461,9 @@ class Config extends Secure_Controller
     public function postSaveLocale(): ResponseInterface
     {
         $exploded = explode(":", $this->request->getPost('language'));
+        $currency_symbol = $this->request->getPost('currency_symbol');
         $batch_save_data = [
-            'currency_symbol'       => $this->request->getPost('currency_symbol'),
+            'currency_symbol'       => htmlspecialchars($currency_symbol ?? ''),
             'currency_code'         => $this->request->getPost('currency_code'),
             'language_code'         => $exploded[0],
             'language'              => $exploded[1],
diff --git a/app/Models/Reports/Summary_discounts.php b/app/Models/Reports/Summary_discounts.php
index f584ce4eb..53f05e340 100644
--- a/app/Models/Reports/Summary_discounts.php
+++ b/app/Models/Reports/Summary_discounts.php
@@ -28,9 +28,10 @@ class Summary_discounts extends Summary_report
         $builder = $this->db->table('sales_items AS sales_items');
 
         if ($inputs['discount_type'] == FIXED) {
-            $builder->select('SUM(sales_items.discount) AS total, MAX(CONCAT("' . $config['currency_symbol'] . '",sales_items.discount)) AS discount, count(*) AS count');
+            $currency_symbol = $this->db->escape($config['currency_symbol']);
+            $builder->select('SUM(sales_items.discount) AS total, MAX(CONCAT(' . $currency_symbol . ', sales_items.discount)) AS discount, count(*) AS count');
             $builder->where('discount_type', FIXED);
-        } elseif ($inputs['discount_type'] == PERCENT) {    // TODO: === ?
+        } elseif ($inputs['discount_type'] == PERCENT) {
             $builder->select('SUM(item_unit_price) * sales_items.discount / 100.0 AS total, MAX(CONCAT(sales_items.discount, "%")) AS discount, count(*) AS count');
             $builder->where('discount_type', PERCENT);
         }
diff --git a/tests/Models/Reports/Summary_discounts_test.php b/tests/Models/Reports/Summary_discounts_test.php
new file mode 100644
index 000000000..535f9006f
--- /dev/null
+++ b/tests/Models/Reports/Summary_discounts_test.php
@@ -0,0 +1,51 @@
+<?php
+
+namespace Tests\Models\Reports;
+
+use CodeIgniter\Test\CIUnitTestCase;
+use App\Models\Reports\Summary_discounts;
+
+class Summary_discounts_test extends CIUnitTestCase
+{
+    public function testCurrencySymbolEscaping(): void
+    {
+        $malicious_symbols = [
+            '"',
+            "'",
+            '" + SLEEP(5) + "',
+            '", SLEEP(5), "',
+            "' + (SELECT * FROM (SELECT(SLEEP(5)))a) + '",
+            '"; DROP TABLE ospos_sales_items; --',
+            '" OR 1=1 --'
+        ];
+
+        foreach ($malicious_symbols as $symbol) {
+            $escaped = $this->escapeCurrencySymbol($symbol);
+            
+            $this->assertStringNotContainsString('SLEEP', $escaped, "SQL injection attempt should be escaped: $symbol");
+            $this->assertStringNotContainsString('DROP', $escaped, "SQL injection attempt should be escaped: $symbol");
+            $this->assertStringNotContainsString(';', $escaped, "Query termination should be escaped: $symbol");
+        }
+    }
+
+    public function testNormalCurrencySymbolHandling(): void
+    {
+        $normal_symbols = ['$', '€', '£', '¥', '₹', '₩', '₽', 'kr', 'CHF'];
+        
+        foreach ($normal_symbols as $symbol) {
+            $escaped = $this->escapeCurrencySymbol($symbol);
+            $this->assertNotEmpty($escaped, "Normal currency symbol should be preserved: $symbol");
+        }
+    }
+
+    private function escapeCurrencySymbol(string $symbol): string
+    {
+        if (strlen($symbol) === 0) {
+            return "''";
+        }
+        
+        $symbol = addslashes($symbol);
+        
+        return "'" . $symbol . "'";
+    }
+}
\ No newline at end of file