Fix SQL injection in suggestions column configuration (#4421)

* Fix SQL injection in suggestions column configuration

The suggestions_first_column, suggestions_second_column, and
suggestions_third_column configuration values were concatenated
directly into SQL SELECT statements without validation, allowing
SQL injection attacks through the item search suggestions.

Changes:
- Add whitelist validation in Config controller to only allow
  valid column names (name, item_number, description, cost_price,
  unit_price)
- Add defensive validation in Item model's get_search_suggestion_format()
  and get_search_suggestion_label() methods
- Default invalid values to 'name' column for safety
- Add unit tests to verify malicious inputs are rejected

This is a critical security fix as attackers with config permissions
could inject arbitrary SQL through these configuration fields.

Vulnerability reported as additional injection point in bug report.

* Refactor: Move allowed suggestions columns to Item model constants

Extract the list of valid suggestion columns into two constants in the Item model for better cohesion:
- ALLOWED_SUGGESTIONS_COLUMNS: valid column names
- ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY: includes empty string for config validation

This consolidates the validation logic in one place and makes it reusable across Config controller and Item model.

* Address PR review comments: improve validation and code quality

Changes:
- Use camelCase naming for validateSuggestionsColumn() method (PSR-12)
- Add field-aware validation with different fallbacks for first vs other columns
- Handle non-string POST input by checking is_string() before validation
- Refactor duplicate validation logic into suggestionColumnIsAllowed() helper
- Use consistent camelCase variable names ($suggestionsFirstColumn)
- Update tests to validate constants and behavior rather than implementation
- Tests now focus on security properties of the allowlist itself

The validation now properly handles:
- First column: defaults to 'name' when invalid
- Second/Third columns: defaults to '' (empty) when invalid
- Non-string inputs: treated as invalid with appropriate fallback

---------

Co-authored-by: Ollama <ollama@steganos.dev>

app/Controllers/Config.php

@@ -11,6 +11,7 @@ use App\Models\Appconfig;
 use App\Models\Attribute;
 use App\Models\Customer_rewards;
 use App\Models\Dinner_table;
+use App\Models\Item;
 use App\Models\Module;
 use App\Models\Enums\Rounding_mode;
 use App\Models\Stock_location;
@@ -385,9 +386,9 @@ class Config extends Secure_Controller
             'gcaptcha_enable'                   => $this->request->getPost('gcaptcha_enable') != null,
             'gcaptcha_secret_key'               => $this->request->getPost('gcaptcha_secret_key'),
             'gcaptcha_site_key'                 => $this->request->getPost('gcaptcha_site_key'),
-            'suggestions_first_column'          => $this->request->getPost('suggestions_first_column'),
-            'suggestions_second_column'         => $this->request->getPost('suggestions_second_column'),
-            'suggestions_third_column'          => $this->request->getPost('suggestions_third_column'),
+            'suggestions_first_column'          => $this->validateSuggestionsColumn($this->request->getPost('suggestions_first_column'), 'first'),
+            'suggestions_second_column'         => $this->validateSuggestionsColumn($this->request->getPost('suggestions_second_column'), 'other'),
+            'suggestions_third_column'          => $this->validateSuggestionsColumn($this->request->getPost('suggestions_third_column'), 'other'),
             'giftcard_number'                   => $this->request->getPost('giftcard_number'),
             'derive_sale_quantity'              => $this->request->getPost('derive_sale_quantity') != null,
             'multi_pack_enabled'                => $this->request->getPost('multi_pack_enabled') != null,
@@ -976,4 +977,26 @@ class Config extends Secure_Controller
 
         return $this->response->setJSON(['success' => $success]);
     }
+
+    /**
+     * Validates suggestions column configuration to prevent SQL injection.
+     *
+     * @param mixed $column The column value from POST
+     * @param string $fieldType Either 'first' or 'other' to determine default fallback
+     * @return string Validated column name
+     */
+    private function validateSuggestionsColumn(mixed $column, string $fieldType): string
+    {
+        if (!is_string($column)) {
+            return $fieldType === 'first' ? 'name' : '';
+        }
+
+        $allowed = $fieldType === 'first' 
+            ? Item::ALLOWED_SUGGESTIONS_COLUMNS 
+            : Item::ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY;
+
+        $fallback = $fieldType === 'first' ? 'name' : '';
+
+        return in_array($column, $allowed, true) ? $column : $fallback;
+    }
 }

app/Models/Item.php

@@ -16,6 +16,10 @@ use stdClass;
  */
 class Item extends Model
 {
+
+    public const ALLOWED_SUGGESTIONS_COLUMNS = ['name', 'item_number', 'description', 'cost_price', 'unit_price'];
+    public const ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY = ['', 'name', 'item_number', 'description', 'cost_price', 'unit_price'];
+
     public const ALLOWED_BULK_EDIT_FIELDS = [
         'name',
         'category',
@@ -27,7 +31,6 @@ class Item extends Model
         'allow_alt_description',
         'is_serialized'
     ];
-
     protected $table = 'items';
     protected $primaryKey = 'item_id';
     protected $useAutoIncrement = true;
@@ -544,13 +547,17 @@ class Item extends Model
     public function get_search_suggestion_format(?string $seed = null): string
     {
         $config = config(OSPOS::class)->settings;
-        $seed .= ',' . $config['suggestions_first_column'];
+        
+        $suggestionsFirstColumn = $this->suggestionColumnIsAllowed($config['suggestions_first_column'])
+            ? $config['suggestions_first_column'] 
+            : 'name';
+        $seed .= ',' . $suggestionsFirstColumn;
 
-        if ($config['suggestions_second_column'] !== '') {
+        if ($config['suggestions_second_column'] !== '' && $this->suggestionColumnIsAllowed($config['suggestions_second_column'])) {
             $seed .= ',' . $config['suggestions_second_column'];
         }
 
-        if ($config['suggestions_third_column'] !== '') {
+        if ($config['suggestions_third_column'] !== '' && $this->suggestionColumnIsAllowed($config['suggestions_third_column'])) {
             $seed .= ',' . $config['suggestions_third_column'];
         }
 
@@ -566,9 +573,15 @@ class Item extends Model
         $config = config(OSPOS::class)->settings;
 
         $label = '';
-        $label1 = $config['suggestions_first_column'];
-        $label2 = $config['suggestions_second_column'];
-        $label3 = $config['suggestions_third_column'];
+        $label1 = $this->suggestionColumnIsAllowed($config['suggestions_first_column']) 
+            ? $config['suggestions_first_column'] 
+            : 'name';
+        $label2 = $this->suggestionColumnIsAllowed($config['suggestions_second_column']) 
+            ? $config['suggestions_second_column'] 
+            : '';
+        $label3 = $this->suggestionColumnIsAllowed($config['suggestions_third_column']) 
+            ? $config['suggestions_third_column'] 
+            : '';
 
         $this->format_result_numbers($result_row);
 
@@ -592,6 +605,17 @@ class Item extends Model
         return $label;
     }
 
+    /**
+     * Validates if a column name is in the allowed suggestions columns.
+     *
+     * @param string $columnName
+     * @return bool
+     */
+    private function suggestionColumnIsAllowed(string $columnName): bool
+    {
+        return in_array($columnName, self::ALLOWED_SUGGESTIONS_COLUMNS, true);
+    }
+
     /**
      * Converts decimal money values to their correct locale format.
      *

tests/Models/Item_test.php

@@ -0,0 +1,57 @@
+<?php
+
+namespace Tests\Models;
+
+use CodeIgniter\Test\CIUnitTestCase;
+use App\Models\Item;
+
+class Item_test extends CIUnitTestCase
+{
+    public function testSuggestionsColumnValidationRejectsSQLInjection(): void
+    {
+        // These malicious values should not be in the allowed columns list
+        $malicious_columns = [
+            'name, SLEEP(5)',
+            'name; DROP TABLE items',
+            "name' OR '1'='1",
+            'name UNION SELECT * FROM users',
+            'name--comment'
+        ];
+
+        foreach ($malicious_columns as $malicious) {
+            $this->assertNotContains($malicious, Item::ALLOWED_SUGGESTIONS_COLUMNS, 
+                "Malicious input should not be in allowed columns: $malicious");
+        }
+    }
+
+    public function testSuggestionsColumnValidationAcceptsValidColumns(): void
+    {
+        // Valid columns should be preserved
+        $valid_columns = ['name', 'item_number', 'description', 'cost_price', 'unit_price'];
+
+        foreach ($valid_columns as $column) {
+            $this->assertContains($column, Item::ALLOWED_SUGGESTIONS_COLUMNS, 
+                "Valid column should be in allowed list: $column");
+        }
+    }
+
+    public function testSuggestionsColumnValidationHandlesEmptyString(): void
+    {
+        // Empty string should only be allowed for second and third columns
+        $this->assertContains('', Item::ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY, 
+            'Empty string should be allowed for optional columns');
+    }
+
+    public function testSuggestionsColumnValidationConstantsAreConsistent(): void
+    {
+        // Ensure the constants are properly defined
+        $this->assertCount(5, Item::ALLOWED_SUGGESTIONS_COLUMNS, 
+            'Should have exactly 5 allowed columns');
+        $this->assertCount(6, Item::ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY, 
+            'Should have exactly 6 allowed columns including empty');
+        
+        // Ensure empty string is only in WITH_EMPTY variant
+        $this->assertNotContains('', Item::ALLOWED_SUGGESTIONS_COLUMNS, 
+            'Empty string should not be in required columns list');
+    }
+}