From f90adab49390efff01a0fc089fc08816f87bcb1a Mon Sep 17 00:00:00 2001 From: jekkos Date: Wed, 7 Oct 2026 23:14:48 +0200 Subject: [PATCH] fix(taxes): reject invalid characters in tax code, category, and jurisdiction names (#4733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(taxes): reject invalid characters in tax code, category, and jurisdiction names Apply the same unicode_alpha_numeric_punct validation already used for item tax names to the three tax save endpoints, so stored-XSS payloads (<, >) are rejected with a validation error instead of being saved. Mirrors the Items fix and adds a TaxesControllerTest regression suite. * fix(taxes): permit slash and CJK in tax names, add regression tests The unicode_alpha_numeric_punct guard rejected '/' (char type Po), which blocks legitimate slash-separated tax names such as 'GST/HST' and 'VAT/GST' from being saved. Add '/' to the allowed punctuation set and document it. Also add regression tests proving acceptance of a slash name (GST/HST) and a CJK name (消費税) across the tax save endpoints. Addresses CodeRabbit review on #4679. * test(4679): adopt test{Method}_{Purpose} naming convention Align test method names with the convention from #4730 (testPostSaveTaxCodes_RejectsMaliciousName, etc.), per review. * fix(taxes): allow parentheses in names, make name optional Address CodeRabbit review on #4733: - Permit parentheses in tax names (e.g. "VAT (20%)"); slash was already allowed, so both "GST/HST" and "VAT (20%)" now pass while < and > are still rejected. - Drop the rule from tax_code_name, jurisdiction_name and tax_category so a code with a blank name can be saved, matching the form (the form does not require the name). - Add regression tests for the parentheses and blank-name cases. --------- Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com> --- app/Config/Validation/OSPOSRules.php | 6 +- app/Controllers/Taxes.php | 51 ++++ tests/Controllers/TaxesControllerTest.php | 271 ++++++++++++++++++++++ 3 files changed, 325 insertions(+), 3 deletions(-) create mode 100644 tests/Controllers/TaxesControllerTest.php diff --git a/app/Config/Validation/OSPOSRules.php b/app/Config/Validation/OSPOSRules.php index b87251da3..dd9d7164c 100644 --- a/app/Config/Validation/OSPOSRules.php +++ b/app/Config/Validation/OSPOSRules.php @@ -212,8 +212,8 @@ class OSPOSRules * legitimate non-English text (e.g. accented or CJK characters). Allows unicode letters, * combining marks (so base+diacritic sequences pass), and digits in place of `A-Z0-9`, and * reuses the exact same punctuation set as the original rule (`~!#$%&*-_+=|:.` plus space), - * extended with `'` and `,` to accommodate real-world tax names (e.g. "O'Brien's Tax", - * "Impôt, incl."). `<` and `>` are deliberately absent from the punctuation set, same as in + * extended with `'`, `,`, `/`, `(` and `)` to accommodate real-world tax names (e.g. "O'Brien's Tax", + * "Impôt, incl.", "GST/HST", "VAT (20%)"). `<` and `>` are deliberately absent from the punctuation set, same as in * the original rule, so this also serves as a defense-in-depth backstop against HTML * injection (the primary fix is escaping at render time). * @@ -224,7 +224,7 @@ class OSPOSRules */ public function unicode_alpha_numeric_punct(string $candidate, ?string &$error = null): bool { - $allowedPunctuation = ['~', '!', '#', '$', '%', '&', '*', '-', '_', '+', '=', '|', ':', '.', ' ', "'", ',']; + $allowedPunctuation = ['~', '!', '#', '$', '%', '&', '*', '-', '_', '+', '=', '|', ':', '.', ' ', "'", ',', '/', '(', ')']; $allowedCategories = [ IntlChar::CHAR_CATEGORY_UPPERCASE_LETTER, diff --git a/app/Controllers/Taxes.php b/app/Controllers/Taxes.php index 29627c7fe..93eeb5cfa 100644 --- a/app/Controllers/Taxes.php +++ b/app/Controllers/Taxes.php @@ -440,6 +440,23 @@ class Taxes extends Secure_Controller $city = $this->request->getPost('city', FILTER_SANITIZE_FULL_SPECIAL_CHARS); $state = $this->request->getPost('state', FILTER_SANITIZE_FULL_SPECIAL_CHARS); + if (!empty($tax_code_name)) { + $rules = [ + 'tax_code_name.*' => 'max_length[255]|unicode_alpha_numeric_punct', + ]; + $messages = [ + 'tax_code_name.*' => [ + 'max_length' => lang('Taxes.tax_code_invalid_chars'), + 'unicode_alpha_numeric_punct' => lang('Taxes.tax_code_invalid_chars'), + ], + ]; + + $error = $this->validateFields($rules, $messages); + if ($error !== null) { + return $error; + } + } + $array_save = []; // TODO: the naming of this variable is not good. foreach ($tax_code_id as $key => $val) { $array_save[] = [ @@ -475,6 +492,23 @@ class Taxes extends Secure_Controller $tax_group_sequence = $this->request->getPost('tax_group_sequence', FILTER_SANITIZE_NUMBER_INT); $cascade_sequence = $this->request->getPost('cascade_sequence', FILTER_SANITIZE_NUMBER_INT); + if (!empty($jurisdiction_name)) { + $rules = [ + 'jurisdiction_name.*' => 'max_length[255]|unicode_alpha_numeric_punct', + ]; + $messages = [ + 'jurisdiction_name.*' => [ + 'max_length' => lang('Taxes.tax_jurisdiction_invalid_chars'), + 'unicode_alpha_numeric_punct' => lang('Taxes.tax_jurisdiction_invalid_chars'), + ], + ]; + + $error = $this->validateFields($rules, $messages); + if ($error !== null) { + return $error; + } + } + $array_save = []; $unique_tax_groups = []; @@ -519,6 +553,23 @@ class Taxes extends Secure_Controller $tax_category = $this->request->getPost('tax_category', FILTER_SANITIZE_FULL_SPECIAL_CHARS); $tax_group_sequence = $this->request->getPost('tax_group_sequence', FILTER_SANITIZE_NUMBER_INT); + if (!empty($tax_category)) { + $rules = [ + 'tax_category.*' => 'max_length[255]|unicode_alpha_numeric_punct', + ]; + $messages = [ + 'tax_category.*' => [ + 'max_length' => lang('Taxes.tax_category_invalid_chars'), + 'unicode_alpha_numeric_punct' => lang('Taxes.tax_category_invalid_chars'), + ], + ]; + + $error = $this->validateFields($rules, $messages); + if ($error !== null) { + return $error; + } + } + $array_save = []; foreach ($tax_category_id as $key => $val) { diff --git a/tests/Controllers/TaxesControllerTest.php b/tests/Controllers/TaxesControllerTest.php new file mode 100644 index 000000000..72bab38db --- /dev/null +++ b/tests/Controllers/TaxesControllerTest.php @@ -0,0 +1,271 @@ +DBGroup)->call('App\Database\Seeds\TestDatabaseBootstrapSeeder'); + Config::connect($this->DBGroup)->close(); + + self::$doneBootstrap = true; + } + + parent::setUp(); + } + + protected function tearDown(): void + { + parent::tearDown(); + } + + protected function createTaxesEmployee(): int + { + return $this->createEmployee( + grants: [ + ['permission_id' => 'taxes', 'menu_group' => 'office'], + ] + ); + } + + protected function loginAsTaxesEmployee(int $personId): void + { + $this->withSession([ + 'person_id' => $personId, + 'menu_group' => 'office', + ]); + } + + /** + * Regression test: `tax_code_name[]` containing `<`/`>` (the stored-XSS + * vector) must be rejected by postSave_tax_codes before anything is saved. + */ + public function testPostSaveTaxCodes_RejectsMaliciousName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_codes', [ + 'tax_code_id' => ['-1'], + 'tax_code' => ['TC' . uniqid()], + 'tax_code_name' => [''], + 'city' => [''], + 'state' => [''], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + } + + /** + * Legitimate unicode tax code names must not be rejected by the XSS guard. + */ + public function testPostSaveTaxCodes_AcceptsUnicodeName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_codes', [ + 'tax_code_id' => ['-1'], + 'tax_code' => ['TC' . uniqid()], + 'tax_code_name' => ["Impôt, incl."], + 'city' => [''], + 'state' => [''], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + /** + * Slash-separated tax names (e.g. "GST/HST") are legitimate and must not be + * rejected by the validation guard. + */ + public function testPostSaveTaxCodes_AcceptsSlashName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_codes', [ + 'tax_code_id' => ['-1'], + 'tax_code' => ['TC' . uniqid()], + 'tax_code_name' => ['GST/HST'], + 'city' => [''], + 'state' => [''], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + /** + * Parenthesised tax names (e.g. "VAT (20%)") are legitimate and must not be + * rejected by the validation guard. + */ + public function testPostSaveTaxCodes_AcceptsParenthesesName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_codes', [ + 'tax_code_id' => ['-1'], + 'tax_code' => ['TC' . uniqid()], + 'tax_code_name' => ['VAT (20%)'], + 'city' => [''], + 'state' => [''], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + /** + * A tax code whose name is left blank is legitimate (the form does not + * require a name) and must not be rejected by the validation guard. + */ + public function testPostSaveTaxCodes_AcceptsBlankName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_codes', [ + 'tax_code_id' => ['-1'], + 'tax_code' => ['TC' . uniqid()], + 'tax_code_name' => [''], + 'city' => [''], + 'state' => [''], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + /** + * Regression test: `tax_category[]` containing `<`/`>` must be rejected. + */ + public function testPostSaveTaxCategories_RejectsMaliciousName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_categories', [ + 'tax_category_id' => ['-1'], + 'tax_category' => [''], + 'tax_group_sequence' => ['1'], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + } + + /** + * Legitimate unicode tax category names must not be rejected. + */ + public function testPostSaveTaxCategories_AcceptsUnicodeName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_categories', [ + 'tax_category_id' => ['-1'], + 'tax_category' => ["Impôt, incl."], + 'tax_group_sequence' => ['1'], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + /** + * CJK tax names (e.g. "消費税", the Japanese consumption tax) are legitimate + * and must not be rejected by the validation guard. + */ + public function testPostSaveTaxCategories_AcceptsCjkName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_categories', [ + 'tax_category_id' => ['-1'], + 'tax_category' => ['消費税'], + 'tax_group_sequence' => ['1'], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } + + /** + * Regression test: `jurisdiction_name[]` containing `<`/`>` must be rejected. + */ + public function testPostSaveTaxJurisdictions_RejectsMaliciousName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_jurisdictions', [ + 'jurisdiction_id' => ['-1'], + 'jurisdiction_name' => [''], + 'tax_group' => ['1'], + 'tax_type' => ['0'], + 'reporting_authority' => [''], + 'tax_group_sequence' => ['1'], + 'cascade_sequence' => ['1'], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertFalse($result['success']); + } + + /** + * Legitimate unicode jurisdiction names must not be rejected. + */ + public function testPostSaveTaxJurisdictions_AcceptsUnicodeName(): void + { + $employeeId = $this->createTaxesEmployee(); + $this->loginAsTaxesEmployee($employeeId); + + $response = $this->post('/taxes/save_tax_jurisdictions', [ + 'jurisdiction_id' => ['-1'], + 'jurisdiction_name' => ["Impôt, incl."], + 'tax_group' => ['1'], + 'tax_type' => ['0'], + 'reporting_authority' => [''], + 'tax_group_sequence' => ['1'], + 'cascade_sequence' => ['1'], + ]); + + $response->assertStatus(200); + $result = json_decode($response->getJSON(), true); + $this->assertTrue($result['success']); + } +}