fix(taxes): reject invalid characters in tax code, category, and jurisdiction names (#4733)

* 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

@@ -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,

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) {

tests/Controllers/TaxesControllerTest.php

@@ -0,0 +1,271 @@
+<?php
+
+namespace Tests\Controllers;
+
+use CodeIgniter\Database\Config;
+use CodeIgniter\Test\CIUnitTestCase;
+use CodeIgniter\Test\DatabaseTestTrait;
+use CodeIgniter\Test\FeatureTestTrait;
+use Tests\Support\EmployeeFixtureTrait;
+
+class TaxesControllerTest extends CIUnitTestCase
+{
+    use DatabaseTestTrait;
+    use FeatureTestTrait;
+    use EmployeeFixtureTrait;
+
+    protected $migrate = true;
+    protected $migrateOnce = true;
+    protected $seedOnce = true;
+    protected $refresh = false;
+    protected $namespace = null;
+
+    private static $doneBootstrap = false;
+
+    protected function setUp(): void
+    {
+        if (self::$doneBootstrap === false) {
+            Config::seeder($this->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' => ['<svg onload=alert(1)>'],
+            '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'       => ['<svg onload=alert(1)>'],
+            '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'   => ['<svg onload=alert(1)>'],
+            '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']);
+    }
+}