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']); + } +}