From 84aeeb52fed6cc3c5c859852a5d68b9d1becce98 Mon Sep 17 00:00:00 2001 From: jekkos Date: Tue, 9 Jun 2026 17:58:52 +0200 Subject: [PATCH] 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