mirror of
https://github.com/opensourcepos/opensourcepos.git
synced 2026-03-25 18:32:17 -04:00
ce411707b4
* 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>
58 lines
2.1 KiB
PHP
58 lines
2.1 KiB
PHP
<?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');
|
|
}
|
|
}
|