From 4173d7f35023f8a5225f78675ed9829c934be168 Mon Sep 17 00:00:00 2001 From: jekkos Date: Thu, 4 Jun 2026 10:09:35 +0200 Subject: [PATCH 1/4] fix: Allow searching by Sale ID in Takings/Daily Sales view (#4569) When searching in the Takings view, entering a plain Sale ID (like '123') did not return any results. The search only worked with customer names or with the 'POS 123' format. The issue was that is_valid_receipt() only recognized 'POS ####' format or invoice numbers, so plain numeric Sale IDs fell through to the customer name search branch which doesn't search sale_id. This fix adds sale_id to the search conditions when the search term is numeric (ctype_digit check), allowing direct Sale ID searches. Fixes #4567 Co-authored-by: Ollama --- app/Models/Sale.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/Models/Sale.php b/app/Models/Sale.php index a73519d52..0e5ddffe2 100644 --- a/app/Models/Sale.php +++ b/app/Models/Sale.php @@ -237,6 +237,9 @@ class Sale extends Model $builder->orLike('customer_p.first_name', $search); // Customer first name $builder->orLike('CONCAT(customer_p.first_name, " ", customer_p.last_name)', $search); // Customer first and last name $builder->orLike('customer.company_name', $search); // Customer company name + if (ctype_digit($search)) { + $builder->orWhere('sales.sale_id', $search); // Sale ID + } $builder->groupEnd(); } } @@ -1477,6 +1480,9 @@ class Sale extends Model $builder->orLike('CONCAT(customer_p.first_name, " ", customer_p.last_name)', $search); // Customer company name $builder->orLike('customer.company_name', $search); + if (ctype_digit($search)) { + $builder->orWhere('sales.sale_id', $search); // Sale ID + } $builder->groupEnd(); } } From 450c0866b5a8e23c3bc2f1dffe0cca7f13746cae Mon Sep 17 00:00:00 2001 From: objecttothis <17935339+objecttothis@users.noreply.github.com> Date: Sat, 6 Jun 2026 02:02:42 +0400 Subject: [PATCH 2/4] Add Guards to Database Migration (#4571) * Properly replace key in config file when encryption key is updated This fixes a break caused if there is a commented out key in the .env. It's a more robust replacement. Signed-off-by: objec * Guard against dropping constraint that doesn't exist - Updated wording in migration_helper.php PHPdoc - Use migration_helper function to drop key which only drops the constraint if it exists. The core logic was not changed here. It only adds a safety mechanism. Signed-off-by: objec * Remove duplicate call to getResultArray in attribute_links loop Signed-off-by: objec * PSR refactoring Signed-off-by: objec * Remove dead parameter from reassignDuplicateAttributeValues method The attribute value was not needed and is never used since we have the attribute_ids and those are unique. Signed-off-by: objec * Create potentially missing primary keys before attempting to add constraints. Signed-off-by: objec * Guard datetime creation Signed-off-by: objec * Update regex pattern to handle double-quoted and non-quoted encryption keys Signed-off-by: objec * Issue warning and fallback on unparseable attribute_date during Signed-off-by: objec --------- Signed-off-by: objec --- .../20210422000000_database_optimizations.php | 113 +++++++++++------- .../3.4.0_database_optimizations.sql | 19 --- app/Helpers/migration_helper.php | 2 +- app/Helpers/security_helper.php | 9 +- 4 files changed, 77 insertions(+), 66 deletions(-) diff --git a/app/Database/Migrations/20210422000000_database_optimizations.php b/app/Database/Migrations/20210422000000_database_optimizations.php index 4c5c496ae..b8a63d449 100644 --- a/app/Database/Migrations/20210422000000_database_optimizations.php +++ b/app/Database/Migrations/20210422000000_database_optimizations.php @@ -18,12 +18,35 @@ class Migration_database_optimizations extends Migration { log_message('info', 'Migrating database optimizations.'); + helper('migration'); + + dropForeignKeyConstraints(['ospos_customers_ibfk_1'], 'customers'); + dropForeignKeyConstraints(['ospos_customers_points_ibfk_1'], 'customers_points'); + dropForeignKeyConstraints(['ospos_sales_ibfk_2'], 'sales'); + dropForeignKeyConstraints(['ospos_sales_payments_ibfk_2'], 'sales_payments'); + dropForeignKeyConstraints(['ospos_sales_ibfk_1'], 'sales'); + dropForeignKeyConstraints(['ospos_receivings_ibfk_1'], 'receivings'); + dropForeignKeyConstraints(['ospos_inventory_ibfk_2'], 'inventory'); + dropForeignKeyConstraints(['ospos_grants_ibfk_2'], 'grants'); + dropForeignKeyConstraints(['ospos_expenses_ibfk_2'], 'expenses'); + dropForeignKeyConstraints(['ospos_employees_ibfk_1'], 'employees'); + dropForeignKeyConstraints(['ospos_cash_up_ibfk_1'], 'cash_up'); + dropForeignKeyConstraints(['ospos_cash_up_ibfk_2'], 'cash_up'); + dropForeignKeyConstraints(['ospos_items_ibfk_1'], 'items'); + dropForeignKeyConstraints(['ospos_expenses_ibfk_3'], 'expenses'); + dropForeignKeyConstraints(['ospos_receivings_ibfk_2'], 'receivings'); + dropForeignKeyConstraints(['ospos_suppliers_ibfk_1'], 'suppliers'); + + createPrimaryKey('customers', 'person_id'); + createPrimaryKey('employees', 'person_id'); + createPrimaryKey('suppliers', 'person_id'); + $attribute = model(Attribute::class); $attribute->deleteOrphanedValues(); - $this->migrate_duplicate_attribute_values(DECIMAL); - $this->migrate_duplicate_attribute_values(DATE); + $this->migrateDuplicateAttributeValues(DECIMAL); + $this->migrateDuplicateAttributeValues(DATE); // Select all attributes that have data in more than one column $builder = $this->db->table('attribute_values'); @@ -36,51 +59,60 @@ class Migration_database_optimizations extends Migration $builder->where('attribute_value IS NOT NULL'); $builder->where('attribute_decimal IS NOT NULL'); $builder->groupEnd(); - $attribute_values = $builder->get(); + $attributeValues = $builder->get(); $this->db->transStart(); // Clean up Attribute values table where there is an attribute value and an attribute_date/attribute_decimal - foreach ($attribute_values->getResultArray() as $attribute_value) { + foreach ($attributeValues->getResultArray() as $attributeValue) { $builder = $this->db->table('attribute_values'); - $builder->delete(['attribute_id' => $attribute_value['attribute_id']]); + $builder->delete(['attribute_id' => $attributeValue['attribute_id']]); $builder = $this->db->table('attribute_links'); $builder->select('links.definition_id, links.item_id, links.attribute_id, defs.definition_type'); $builder->join('attribute_definitions defs', 'defs.definition_id = links.definition_id'); - $builder->where('attribute_id', $attribute_value['attribute_id']); - $attribute_links = $builder->get(); + $builder->where('attribute_id', $attributeValue['attribute_id']); + $attributeLinks = $builder->get(); - if ($attribute_links) { + if ($attributeLinks) { $builder = $this->db->table('attribute_links'); - $attribute_links = $attribute_links->getResultArray() ?: []; + $attributeLinks = $attributeLinks->getResultArray() ?: []; - foreach ($attribute_links->getResultArray() as $attribute_link) { - $builder->where('attribute_id', $attribute_link['attribute_id']); - $builder->where('item_id', $attribute_link['item_id']); + foreach ($attributeLinks as $attributeLink) { + $builder->where('attribute_id', $attributeLink['attribute_id']); + $builder->where('item_id', $attributeLink['item_id']); $builder->delete(); - switch ($attribute_link['definition_type']) { + switch ($attributeLink['definition_type']) { case DECIMAL: - $value = $attribute_value['attribute_decimal']; + $value = $attributeValue['attribute_decimal']; break; case DATE: $config = config(OSPOS::class)->settings; - $attribute_date = DateTime::createFromFormat('Y-m-d', $attribute_value['attribute_date']); - $value = $attribute_date->format($config['dateformat']); + $attributeDate = DateTime::createFromFormat('Y-m-d', (string) $attributeValue['attribute_date']); + + if ($attributeDate === false) { + log_message('warning', 'Migration 20210422000000: unparseable attribute_date "' . $attributeValue['attribute_date'] . '" for attribute_id ' . $attributeValue['attribute_id'] . ' — preserving raw value.'); + $value = (string) $attributeValue['attribute_date']; + } else { + $dateFormat = empty($config['dateformat']) ? 'Y-m-d' : $config['dateformat']; + if (empty($config['dateformat'])) { + log_message('warning', 'Migration 20210422000000: dateformat config empty, falling back to Y-m-d for attribute_id ' . $attributeValue['attribute_id'] . '.'); + } + $value = $attributeDate->format($dateFormat); + } break; default: - $value = $attribute_value['attribute_value']; + $value = $attributeValue['attribute_value']; break; } - $attribute->saveAttributeValue($value, $attribute_link['definition_id'], $attribute_link['item_id'], false, $attribute_link['definition_type']); + $attribute->saveAttributeValue($value, $attributeLink['definition_id'], $attributeLink['item_id'], false, $attributeLink['definition_type']); } } } $this->db->transComplete(); - helper('migration'); execute_script(APPPATH . 'Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql'); log_message('info', 'Finished migrating database optimizations.'); } @@ -88,58 +120,57 @@ class Migration_database_optimizations extends Migration /** * Given the type of attribute, deletes any duplicates it finds in the attribute_values table and reassigns those */ - private function migrate_duplicate_attribute_values($attribute_type): void + private function migrateDuplicateAttributeValues(string $attributeType): void { // Remove duplicate attribute values needed to make attribute_decimals and attribute_dates unique $this->db->transStart(); - $column = 'attribute_' . strtolower($attribute_type); + $column = 'attribute_' . strtolower($attributeType); $builder = $this->db->table('attribute_values'); $builder->select("$column"); $builder->groupBy($column); $builder->having("COUNT($column) > 1"); - $duplicated_values = $builder->get(); + $duplicatedValues = $builder->get(); - foreach ($duplicated_values->getResultArray() as $duplicated_value) { - $subquery_builder = $this->db->table('attribute_values'); - $subquery_builder->select('attribute_id'); - $subquery_builder->where($column, $duplicated_value[$column]); - $subquery = $subquery_builder->getCompiledSelect(); + foreach ($duplicatedValues->getResultArray() as $duplicatedValue) { + $subqueryBuilder = $this->db->table('attribute_values'); + $subqueryBuilder->select('attribute_id'); + $subqueryBuilder->where($column, $duplicatedValue[$column]); + $subquery = $subqueryBuilder->getCompiledSelect(); $builder = $this->db->table('attribute_values'); $builder->select('attribute_id'); - $builder->where($column, $duplicated_value[$column]); + $builder->where($column, $duplicatedValue[$column]); $builder->where("attribute_id IN ($subquery)", null, false); - $attribute_ids_to_fix = $builder->get(); + $attributeIdsToFix = $builder->get(); - $this->reassign_duplicate_attribute_values($attribute_ids_to_fix, $duplicated_value); + $this->reassignDuplicateAttributeValues($attributeIdsToFix); } $this->db->transComplete(); } /** - * Updates the attribute_id in all attribute_link rows with duplicated attribute_ids then deletes unneeded rows from attribute_values + * Updates the attribute_id in all attribute_link rows with duplicated attributeIds then deletes unneeded rows from attributeValues * - * @param ResultInterface $attribute_ids_to_fix All attribute_ids that need to parsed - * @param array $attribute_value The attribute value in question. + * @param ResultInterface $attributeIdsToFix All attributeIds that need to parsed */ - private function reassign_duplicate_attribute_values(ResultInterface $attribute_ids_to_fix, array $attribute_value): void + private function reassignDuplicateAttributeValues(ResultInterface $attributeIdsToFix): void { - $attribute_ids = $attribute_ids_to_fix->getResultArray(); - $retain_attribute_id = $attribute_ids[0]['attribute_id']; + $attributeIds = $attributeIdsToFix->getResultArray(); + $retainAttributeId = $attributeIds[0]['attribute_id']; - foreach ($attribute_ids as $attribute_id) { + foreach ($attributeIds as $attributeId) { // Update attribute_link with the attribute_id we are keeping $builder = $this->db->table('attribute_links'); - $builder->where('attribute_id', $attribute_id['attribute_id']); - $builder->update(['attribute_id' => $retain_attribute_id]); + $builder->where('attribute_id', $attributeId['attribute_id']); + $builder->update(['attribute_id' => $retainAttributeId]); // Delete the row from attribute_values if it isn't our keeper - if ($attribute_id['attribute_id'] !== $retain_attribute_id) { + if ($attributeId['attribute_id'] !== $retainAttributeId) { $builder = $this->db->table('attribute_values'); - $builder->delete(['attribute_id' => $attribute_id['attribute_id']]); + $builder->delete(['attribute_id' => $attributeId['attribute_id']]); } } } diff --git a/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql b/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql index 50f702b95..86f3e189e 100644 --- a/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql +++ b/app/Database/Migrations/sqlscripts/3.4.0_database_optimizations.sql @@ -11,10 +11,6 @@ ALTER TABLE `ospos_attribute_definitions` ADD INDEX(`definition_type`); ALTER TABLE `ospos_cash_up` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; #ospos_customers table -ALTER TABLE `ospos_customers` DROP FOREIGN KEY `ospos_customers_ibfk_1`; -ALTER TABLE `ospos_customers_points` DROP FOREIGN KEY `ospos_customers_points_ibfk_1`; -ALTER TABLE `ospos_sales` DROP FOREIGN KEY `ospos_sales_ibfk_2`; - ALTER TABLE `ospos_customers` MODIFY `taxable` tinyint(1) DEFAULT 1 NOT NULL; ALTER TABLE `ospos_customers` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; ALTER TABLE `ospos_customers` MODIFY `discount_type` tinyint(1) DEFAULT 0 NOT NULL; @@ -31,16 +27,6 @@ ALTER TABLE `ospos_dinner_tables` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL ALTER TABLE `ospos_dinner_tables` ADD INDEX(`status`); #ospos_employees table -ALTER TABLE `ospos_sales_payments` DROP FOREIGN KEY `ospos_sales_payments_ibfk_2`; -ALTER TABLE `ospos_sales` DROP FOREIGN KEY `ospos_sales_ibfk_1`; -ALTER TABLE `ospos_receivings` DROP FOREIGN KEY `ospos_receivings_ibfk_1`; -ALTER TABLE `ospos_inventory` DROP FOREIGN KEY `ospos_inventory_ibfk_2`; -ALTER TABLE `ospos_grants` DROP FOREIGN KEY `ospos_grants_ibfk_2`; -ALTER TABLE `ospos_expenses` DROP FOREIGN KEY `ospos_expenses_ibfk_2`; -ALTER TABLE `ospos_employees` DROP FOREIGN KEY `ospos_employees_ibfk_1`; -ALTER TABLE `ospos_cash_up` DROP FOREIGN KEY `ospos_cash_up_ibfk_1`; -ALTER TABLE `ospos_cash_up` DROP FOREIGN KEY `ospos_cash_up_ibfk_2`; - ALTER TABLE `ospos_employees` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; ALTER TABLE `ospos_employees` MODIFY `hash_version` tinyint(1) DEFAULT 2 NOT NULL; @@ -67,7 +53,6 @@ ALTER TABLE `ospos_expense_categories` ADD INDEX(`category_description`); ALTER TABLE `ospos_giftcards` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; #ospos_items table -ALTER TABLE `ospos_items` DROP FOREIGN KEY `ospos_items_ibfk_1`; ALTER TABLE `ospos_items` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; ALTER TABLE `ospos_items` MODIFY `stock_type` tinyint(1) DEFAULT 0 NOT NULL; ALTER TABLE `ospos_items` MODIFY `item_type` tinyint(1) DEFAULT 0 NOT NULL; @@ -112,10 +97,6 @@ ALTER TABLE `ospos_sessions` ADD INDEX(`ip_address`); ALTER TABLE `ospos_stock_locations` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; #ospos_suppliers table -ALTER TABLE `ospos_expenses` DROP FOREIGN KEY `ospos_expenses_ibfk_3`; -ALTER TABLE `ospos_receivings` DROP FOREIGN KEY `ospos_receivings_ibfk_2`; -ALTER TABLE `ospos_suppliers` DROP FOREIGN KEY `ospos_suppliers_ibfk_1`; - ALTER TABLE `ospos_suppliers` MODIFY `deleted` tinyint(1) DEFAULT 0 NOT NULL; ALTER TABLE `ospos_suppliers` MODIFY `category` tinyint(1) NOT NULL; ALTER TABLE `ospos_suppliers` ADD INDEX(`category`); diff --git a/app/Helpers/migration_helper.php b/app/Helpers/migration_helper.php index ef056404e..6baa9f24c 100644 --- a/app/Helpers/migration_helper.php +++ b/app/Helpers/migration_helper.php @@ -88,7 +88,7 @@ function executeScriptWithTransaction(string $path): bool } /** - * Drops provided foreign key constraints from given table. + * Drops provided foreign key constraints from a given table if the constraint exists. * This is required to successfully create the generated unique constraint. * * @param array $foreignKeys names of the foreign key constraints to drop diff --git a/app/Helpers/security_helper.php b/app/Helpers/security_helper.php index ea52be64d..5311b4ce9 100644 --- a/app/Helpers/security_helper.php +++ b/app/Helpers/security_helper.php @@ -40,17 +40,16 @@ function check_encryption(): bool $config_file = file_get_contents($config_path); - if (strpos($config_file, 'encryption.key') !== false) { - $config_file = preg_replace("/(encryption\.key.*=.*)('.*')/", "$1'$key'", $config_file); + if (preg_match('/^\s*encryption\.key\s*=/m', $config_file)) { + $config_file = preg_replace("/^(\s*encryption\.key\s*=\s*).*/m", "\$1'$key'", $config_file, 1); } else { $config_file .= "\nencryption.key = '$key'\n"; } if (!empty($old_key)) { $old_line = "# encryption.key = '$old_key' REMOVE IF UNNEEDED\r\n"; - $insertion_point = stripos($config_file, 'encryption.key'); - if ($insertion_point !== false) { - $config_file = substr_replace($config_file, $old_line, $insertion_point, 0); + if (preg_match('/^encryption\.key\s*=/m', $config_file, $matches, PREG_OFFSET_CAPTURE)) { + $config_file = substr_replace($config_file, $old_line, $matches[0][1], 0); } } From 4d6ebbafdd5eb55cb4603c1dd08eed641756e6c0 Mon Sep 17 00:00:00 2001 From: jekkos Date: Sat, 6 Jun 2026 22:50:51 +0200 Subject: [PATCH 3/4] fix: tax rate inputs blank with comma-decimal locales (#4555) * fix: tax rate inputs blank with comma-decimal locales The to_tax_decimals() function returns locale-formatted values (e.g. "18,00" for comma-decimal locales like fr_FR, de_DE). Browsers reject comma-decimal values in and render the field blank. Use raw float value instead - PHP serializes floats with period decimal regardless of locale. The parse_tax() on the save side already handles locale-aware parsing, so round-tripping works correctly. Fixes #4553 Regression from commit 42ba39d29 * fix: tax rate input locale handling - save path The display fix (using (float) instead of to_tax_decimals()) was correct but incomplete. The save path in Config.php also needed fixing because parse_tax() misinterprets dot-decimal values from type="number" inputs when locale uses comma as decimal separator. Root cause: Browsers submit type="number" inputs as dot-decimal (e.g., "5.5") regardless of locale. With comma-decimal locales like de_DE, parse_tax() treats the dot as thousands separator, causing 5.5 to be saved as 5. Fix: Replace parse_tax() with direct (float) cast for these inputs since type="number" already guarantees dot-decimal format. Includes tests for tax rate handling with various decimal values. Fixes #4553 * revert: remove type=number from tax rate inputs Resolution from PR #4555 review: Revert to text inputs for locale-specific tax rate fields. The type='number' attribute was added in commit 42ba39d29, but it caused issues with locale-specific decimal separators. Browsers submit type='number' inputs as dot-decimal regardless of locale, which breaks comma-decimal locales. Solution: Revert to text inputs which use to_tax_decimals() for display and parse_tax() for saving, correctly handling locale-specific formatting. Changes: - tax_config.php: Remove type='number', step, min, max attributes - tax_config.php: Restore to_tax_decimals() for value display - Config.php: Restore parse_tax() for tax rate parsing - ConfigTest.php: Remove tests added for the type='number' approach Fixes #4553 --------- Co-authored-by: Ollama --- app/Views/configs/tax_config.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/Views/configs/tax_config.php b/app/Views/configs/tax_config.php index 7b970e6a6..23248082c 100644 --- a/app/Views/configs/tax_config.php +++ b/app/Views/configs/tax_config.php @@ -51,10 +51,6 @@
'number', - 'step' => 'any', - 'min' => '0', - 'max' => '100', 'name' => 'default_tax_1_rate', 'id' => 'default_tax_1_rate', 'class' => 'form-control input-sm', @@ -76,10 +72,6 @@
'number', - 'step' => 'any', - 'min' => '0', - 'max' => '100', 'name' => 'default_tax_2_rate', 'id' => 'default_tax_2_rate', 'class' => 'form-control input-sm', From 84aeeb52fed6cc3c5c859852a5d68b9d1becce98 Mon Sep 17 00:00:00 2001 From: jekkos Date: Tue, 9 Jun 2026 17:58:52 +0200 Subject: [PATCH 4/4] fix(security): Fix DOMPDF RCE and customer email sanitization (#4568) * fix(security): Fix DOMPDF RCE and customer email sanitization - Disable isPhpEnabled in DOMPDF to prevent RCE via embedded PHP in HTML - Disable isRemoteEnabled to prevent SSRF attacks - Add email validation and sanitization in CSV import (FILTER_SANITIZE_EMAIL, FILTER_VALIDATE_EMAIL) - Reject invalid email formats during customer import * fix(security): Escape email addresses in mailto() to prevent XSS Email columns in bootstrap tables had escaping disabled (line 52) and mailto() function doesn't escape its parameters. This fix escapes email addresses before passing to mailto() in: - get_person_data_row() (employees) - get_customer_data_row() (customers) - get_supplier_data_row() (suppliers) Attack vector: Malicious email via CSV import renders XSS in table view. * test(security): Add tests for customer CSV import email validation Tests cover: - Valid email acceptance - Invalid email rejection with row-specific error - XSS payload sanitization in email field - Mixed valid/invalid email handling - Email with special characters sanitization Verifies fixes for customer email import vulnerability. * fix(security): Allow empty email addresses in customer import - Empty emails are now allowed (customers may not have email addresses) - Validation only applies when email is non-empty - Added test case for empty email acceptance This fixes a regression where FILTER_VALIDATE_EMAIL rejected empty strings, breaking imports for customers without email addresses. --------- Co-authored-by: Ollama --- app/Controllers/Customers.php | 10 +- app/Helpers/dompdf_helper.php | 9 +- app/Helpers/tabular_helper.php | 6 +- tests/Controllers/CustomersCsvImportTest.php | 266 +++++++++++++++++++ 4 files changed, 285 insertions(+), 6 deletions(-) create mode 100644 tests/Controllers/CustomersCsvImportTest.php diff --git a/app/Controllers/Customers.php b/app/Controllers/Customers.php index b4adfb455..55a872a61 100644 --- a/app/Controllers/Customers.php +++ b/app/Controllers/Customers.php @@ -419,7 +419,15 @@ class Customers extends Persons $consent = $data[3] == '' ? 0 : 1; if (sizeof($data) >= 16 && $consent) { - $email = strtolower($data[4]); + $email = filter_var(strtolower($data[4]), FILTER_SANITIZE_EMAIL); + + // Empty email is allowed, but if provided it must be valid + if ($email !== '' && !filter_var($email, FILTER_VALIDATE_EMAIL)) { + $failCodes[] = 'Row ' . $i . ': Invalid email format'; + $i++; + continue; + } + $person_data = [ 'first_name' => $data[0], 'last_name' => $data[1], diff --git a/app/Helpers/dompdf_helper.php b/app/Helpers/dompdf_helper.php index 3baf0a5ea..fcd9a3c1f 100644 --- a/app/Helpers/dompdf_helper.php +++ b/app/Helpers/dompdf_helper.php @@ -5,8 +5,13 @@ */ function create_pdf(string $html, string $filename = ''): string { - // Need to enable magic quotes for the - $dompdf = new Dompdf\Dompdf(['isRemoteEnabled' => true, 'isPhpEnabled' => true]); + // Security: Disable PHP execution in PDFs to prevent RCE attacks + // Security: Disable remote file access to prevent SSRF attacks + // Only local files referenced in HTML are allowed + $dompdf = new Dompdf\Dompdf([ + 'isRemoteEnabled' => false, + 'isPhpEnabled' => false + ]); $dompdf->loadHtml(str_replace(['\n', '\r'], '', $html)); $dompdf->render(); diff --git a/app/Helpers/tabular_helper.php b/app/Helpers/tabular_helper.php index d1b3b9281..7c6e2c927 100644 --- a/app/Helpers/tabular_helper.php +++ b/app/Helpers/tabular_helper.php @@ -226,7 +226,7 @@ function get_person_data_row(object $person): array 'people.person_id' => $person->person_id, 'last_name' => $person->last_name, 'first_name' => $person->first_name, - 'email' => empty($person->email) ? '' : mailto($person->email, $person->email), + 'email' => empty($person->email) ? '' : mailto(esc($person->email), esc($person->email)), 'phone_number' => $person->phone_number, 'messages' => empty($person->phone_number) ? '' @@ -292,7 +292,7 @@ function get_customer_data_row(object $person, object $stats): array 'people.person_id' => $person->person_id, 'last_name' => $person->last_name, 'first_name' => $person->first_name, - 'email' => empty($person->email) ? '' : mailto($person->email, $person->email), + 'email' => empty($person->email) ? '' : mailto(esc($person->email), esc($person->email)), 'phone_number' => $person->phone_number, 'total' => to_currency($stats->total), 'messages' => empty($person->phone_number) @@ -363,7 +363,7 @@ function get_supplier_data_row(object $supplier): array 'category' => $supplier->category, 'last_name' => $supplier->last_name, 'first_name' => $supplier->first_name, - 'email' => empty($supplier->email) ? '' : mailto($supplier->email, $supplier->email), + 'email' => empty($supplier->email) ? '' : mailto(esc($supplier->email), esc($supplier->email)), 'phone_number' => $supplier->phone_number, 'messages' => empty($supplier->phone_number) ? '' diff --git a/tests/Controllers/CustomersCsvImportTest.php b/tests/Controllers/CustomersCsvImportTest.php new file mode 100644 index 000000000..4c071ebfc --- /dev/null +++ b/tests/Controllers/CustomersCsvImportTest.php @@ -0,0 +1,266 @@ +customer = model(Customer::class); + $this->employee = model(Employee::class); + + helper('test'); + } + + protected function tearDown(): void + { + parent::tearDown(); + } + + protected function loginAsEmployee(): void + { + $session = Services::session(); + $session->set('person_id', 1); + $session->set('menu_group', 'office'); + } + + protected function createCsvFile(array $rows): string + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_'); + + $handle = fopen($tempFile, 'w'); + foreach ($rows as $row) { + fputcsv($handle, $row); + } + fclose($handle); + + return $tempFile; + } + + public function testValidEmailIsAccepted(): void + { + $this->loginAsEmployee(); + + $csvContent = [ + ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'], + ['John', 'Doe', '1', '1', 'john.doe@example.com', '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', ''] + ]; + + $tempFile = $this->createCsvFile($csvContent); + + $_FILES['file_path'] = [ + 'name' => 'test.csv', + 'type' => 'text/csv', + 'tmp_name' => $tempFile, + 'error' => UPLOAD_ERR_OK, + 'size' => filesize($tempFile) + ]; + + $result = $this->post('/customers/importCsvFile'); + + $result->assertOK(); + $result->assertJSONExact(['success' => true, 'message' => 'Customers imported successfully']); + + $importedCustomer = $this->customer->where('email', 'john.doe@example.com')->first(); + $this->assertNotNull($importedCustomer); + + unlink($tempFile); + } + + public function testInvalidEmailIsRejected(): void + { + $this->loginAsEmployee(); + + $csvContent = [ + ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'], + ['John', 'Doe', '1', '1', 'not-an-email', '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', ''] + ]; + + $tempFile = $this->createCsvFile($csvContent); + + $_FILES['file_path'] = [ + 'name' => 'test.csv', + 'type' => 'text/csv', + 'tmp_name' => $tempFile, + 'error' => UPLOAD_ERR_OK, + 'size' => filesize($tempFile) + ]; + + $result = $this->post('/customers/importCsvFile'); + + $result->assertOK(); + + $resultBody = json_decode($result->getJSON(), true); + $this->assertFalse($resultBody['success'], 'Import should fail for invalid email'); + $this->assertStringContainsString('Row 1', $resultBody['message'], 'Error message should reference failing row'); + $this->assertStringContainsString('Invalid email format', $resultBody['message'], 'Error message should mention email validation'); + + $importedCustomer = $this->customer->where('email', 'not-an-email')->first(); + $this->assertNull($importedCustomer, 'Customer with invalid email should not be imported'); + + unlink($tempFile); + } + + public function testXssPayloadInEmailIsSanitized(): void + { + $this->loginAsEmployee(); + + $maliciousEmail = '@example.com'; + + $csvContent = [ + ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'], + ['John', 'Doe', '1', '1', $maliciousEmail, '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', ''] + ]; + + $tempFile = $this->createCsvFile($csvContent); + + $_FILES['file_path'] = [ + 'name' => 'test.csv', + 'type' => 'text/csv', + 'tmp_name' => $tempFile, + 'error' => UPLOAD_ERR_OK, + 'size' => filesize($tempFile) + ]; + + $result = $this->post('/customers/importCsvFile'); + + $result->assertOK(); + + $importedCustomer = $this->customer->where('email LIKE', '%example.com')->first(); + + $this->assertNotNull($importedCustomer, 'Customer should be imported after sanitization'); + $this->assertStringNotContainsString('', $importedCustomer->email, 'Script tags should be removed'); + + unlink($tempFile); + } + + public function testMixedValidAndInvalidEmails(): void + { + $this->loginAsEmployee(); + + $csvContent = [ + ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'], + ['Valid', 'User', '1', '1', 'valid@example.com', '555-1111', '123 Main St', '', 'City1', 'ST', '12345', 'US', '', '', '', '', '', ''], + ['Invalid', 'User', '1', '1', 'invalid-email', '555-2222', '456 Oak Ave', '', 'City2', 'ST', '23456', 'US', '', '', '', '', '', ''], + ['Another', 'Valid', '1', '1', 'another@example.com', '555-3333', '789 Pine Rd', '', 'City3', 'ST', '34567', 'US', '', '', '', '', '', ''] + ]; + + $tempFile = $this->createCsvFile($csvContent); + + $_FILES['file_path'] = [ + 'name' => 'test.csv', + 'type' => 'text/csv', + 'tmp_name' => $tempFile, + 'error' => UPLOAD_ERR_OK, + 'size' => filesize($tempFile) + ]; + + $result = $this->post('/customers/importCsvFile'); + + $result->assertOK(); + + $validCustomer1 = $this->customer->where('email', 'valid@example.com')->first(); + $this->assertNotNull($validCustomer1, 'Valid customer should be imported'); + + $validCustomer2 = $this->customer->where('email', 'another@example.com')->first(); + $this->assertNotNull($validCustomer2, 'Another valid customer should be imported'); + + $invalidCustomer = $this->customer->where('email', 'invalid-email')->first(); + $this->assertNull($invalidCustomer, 'Invalid email customer should not be imported'); + + unlink($tempFile); + } + + public function testEmailWithSpecialCharactersIsSanitized(): void + { + $this->loginAsEmployee(); + + $emailWithSpecialChars = 'test"user@example.com'; + $csvContent = [ + ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'], + ['Test', 'User', '1', '1', $emailWithSpecialChars, '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', ''] + ]; + + $tempFile = $this->createCsvFile($csvContent); + + $_FILES['file_path'] = [ + 'name' => 'test.csv', + 'type' => 'text/csv', + 'tmp_name' => $tempFile, + 'error' => UPLOAD_ERR_OK, + 'size' => filesize($tempFile) + ]; + + $result = $this->post('/customers/importCsvFile'); + + $result->assertOK(); + + $importedCustomer = $this->customer->where('email LIKE', '%example.com')->first(); + + $this->assertNotNull($importedCustomer, 'Sanitized email should be imported'); + $this->assertStringNotContainsString('"', $importedCustomer->email, 'Quote characters should be sanitized'); + + unlink($tempFile); + } + + public function testEmptyEmailIsAccepted(): void + { + $this->loginAsEmployee(); + + // Empty email should be allowed - customers may not have email addresses + $csvContent = [ + ['First Name', 'Last Name', 'Gender', 'Consent', 'Email', 'Phone', 'Address 1', 'Address 2', 'City', 'State', 'Zip', 'Country', 'Comments', 'Company', 'Account Number', 'Discount', 'Discount Type', 'Taxable'], + ['John', 'Doe', '1', '1', '', '555-1234', '123 Main St', '', 'Springfield', 'IL', '62701', 'US', '', '', '', '', '', ''] + ]; + + $tempFile = $this->createCsvFile($csvContent); + + $_FILES['file_path'] = [ + 'name' => 'test.csv', + 'type' => 'text/csv', + 'tmp_name' => $tempFile, + 'error' => UPLOAD_ERR_OK, + 'size' => filesize($tempFile) + ]; + + $result = $this->post('/customers/importCsvFile'); + + $result->assertOK(); + + $resultBody = json_decode($result->getJSON(), true); + $this->assertTrue($resultBody['success'], 'Import should succeed with empty email'); + + // Find customer by name since email is empty + $importedCustomer = $this->customer->select('customers.*, people.*') + ->join('people', 'people.person_id = customers.person_id') + ->where('first_name', 'John') + ->where('last_name', 'Doe') + ->first(); + + $this->assertNotNull($importedCustomer, 'Customer with empty email should be imported'); + $this->assertEquals('', $importedCustomer->email, 'Email should be empty string'); + + unlink($tempFile); + } +} \ No newline at end of file