diff --git a/AGENTS.md b/AGENTS.md index d36edae57..fdb3677af 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,6 +26,7 @@ This document provides guidance for AI agents working on the Open Source Point o - Run PHPUnit tests: `composer test` - Tests must pass before submitting changes +- **One test file per class under test.** A controller, model, library, or helper gets exactly one test file covering all of its behavior — `Item_kits.php` → `tests/Controllers/Item_kitsTest.php` (or `Item_kitsControllerTest.php`, matching this codebase's existing `*ControllerTest.php` suffix for controllers), `Sale_lib.php` → `tests/Libraries/Sale_libTest.php`, etc. Do not create feature- or endpoint-scoped test files alongside a class's main test file (e.g. no `Item_kitsBarcodeTest.php` next to `Item_kitsControllerTest.php`) — add the new test methods to the existing file for that class instead. If no test file exists yet for the class, create the one canonical file rather than a narrowly-scoped one. ## Build diff --git a/app/Controllers/Item_kits.php b/app/Controllers/Item_kits.php index f0cc57eb1..820db78a0 100644 --- a/app/Controllers/Item_kits.php +++ b/app/Controllers/Item_kits.php @@ -255,39 +255,39 @@ class Item_kits extends Secure_Controller /** * AJAX called function that generates barcodes for selected item_kits. * - * @param string $item_kit_ids Colon separated list of item_kit_id values to generate barcodes for. + * @param string $itemKitIds Colon separated list of item_kit_id values to generate barcodes for. * @return string * @noinspection PhpUnused */ - public function getGenerateBarcodes(string $item_kit_ids): string + public function getGenerateBarcodes(string $itemKitIds): string { - $barcode_lib = new Barcode_lib(); + $barcodeLib = new Barcode_lib(); $result = []; - $item_kit_ids = explode(':', $item_kit_ids); - foreach ($item_kit_ids as $item_kid_id) { + $itemKitIds = explode(':', $itemKitIds); + foreach ($itemKitIds as $itemKitId) { // Calculate the total cost and retail price of the Kit, so it can be added to the barcode text at the bottom - $item_kit = $this->_add_totals_to_item_kit($this->item_kit->get_info($item_kid_id)); + $itemKit = $this->_add_totals_to_item_kit($this->item_kit->get_info($itemKitId)); - $item_kid_id = 'KIT ' . urldecode($item_kid_id); + $itemKitId = 'KIT ' . $itemKitId; $result[] = [ - 'name' => $item_kit->name, - 'item_id' => $item_kid_id, - 'item_number' => $item_kid_id, - 'cost_price' => $item_kit->total_cost_price, - 'unit_price' => $item_kit->total_unit_price + 'name' => $itemKit->name, + 'item_id' => $itemKitId, + 'item_number' => $itemKitId, + 'cost_price' => $itemKit->total_cost_price, + 'unit_price' => $itemKit->total_unit_price ]; } $data['items'] = $result; - $barcode_config = $barcode_lib->get_barcode_config(); + $barcodeConfig = $barcodeLib->get_barcode_config(); // In case the selected barcode type is not Code39 or Code128 we set by default Code128 // The rationale for this is that EAN codes cannot have strings as seed, so 'KIT ' is not allowed - if ($barcode_config['barcode_type'] != 'C39' && $barcode_config['barcode_type'] != 'C128') { - $barcode_config['barcode_type'] = 'C128'; + if ($barcodeConfig['barcode_type'] != 'C39' && $barcodeConfig['barcode_type'] != 'C128') { + $barcodeConfig['barcode_type'] = 'C128'; } - $data['barcode_config'] = $barcode_config; + $data['barcode_config'] = $barcodeConfig; // Display barcodes return view("barcodes/barcode_sheet", $data); diff --git a/app/Libraries/Barcode_lib.php b/app/Libraries/Barcode_lib.php index 202457adc..e7f96a47d 100644 --- a/app/Libraries/Barcode_lib.php +++ b/app/Libraries/Barcode_lib.php @@ -146,10 +146,10 @@ class Barcode_lib if ((isset($item['item_number']) || isset($item['name'])) && isset($item['item_id'])) { $barcode = $this->generate_barcode($item, $barcode_config); $display_table = ''; - $display_table .= ''; + $display_table .= ''; $display_table .= ''; - $display_table .= ''; - $display_table .= ''; + $display_table .= ''; + $display_table .= ''; $display_table .= '
' . $this->manage_display_layout($barcode_config['barcode_first_row'], $item, $barcode_config) . '
' . $this->manageDisplayLayout($barcode_config['barcode_first_row'], $item, $barcode_config) . '
'.$barcode.'
' . $this->manage_display_layout($barcode_config['barcode_second_row'], $item, $barcode_config) . '
' . $this->manage_display_layout($barcode_config['barcode_third_row'], $item, $barcode_config) . '
' . $this->manageDisplayLayout($barcode_config['barcode_second_row'], $item, $barcode_config) . '
' . $this->manageDisplayLayout($barcode_config['barcode_third_row'], $item, $barcode_config) . '
'; return $display_table; @@ -159,30 +159,30 @@ class Barcode_lib } /** - * @param $layout_type + * @param string $layoutType * @param array $item - * @param array $barcode_config + * @param array $barcodeConfig * @return string */ - private function manage_display_layout($layout_type, array $item, array $barcode_config): string + private function manageDisplayLayout(string $layoutType, array $item, array $barcodeConfig): string { $result = ''; helper('text'); - if ($layout_type == 'name') { - $result = $item['name']; - } elseif ($layout_type == 'category' && isset($item['category'])) { + if ($layoutType == 'name') { + $result = esc($item['name']); + } elseif ($layoutType == 'category' && isset($item['category'])) { $result = lang('Items.category') . " " . esc($item['category']); - } elseif ($layout_type == 'cost_price' && isset($item['cost_price'])) { + } elseif ($layoutType == 'cost_price' && isset($item['cost_price'])) { $result = lang('Items.cost_price') . " " . to_currency($item['cost_price']); - } elseif ($layout_type == 'unit_price' && isset($item['unit_price'])) { + } elseif ($layoutType == 'unit_price' && isset($item['unit_price'])) { $result = lang('Items.unit_price') . " " . to_currency($item['unit_price']); - } elseif ($layout_type == 'company_name') { - $result = $barcode_config['company']; - } elseif ($layout_type == 'item_code') { - $result = $barcode_config['barcode_content'] !== "id" && isset($item['item_number']) - ? $item['item_number'] - : $item['item_id']; + } elseif ($layoutType == 'company_name') { + $result = esc($barcodeConfig['company']); + } elseif ($layoutType == 'item_code') { + $result = $barcodeConfig['barcode_content'] !== "id" && isset($item['item_number']) + ? esc($item['item_number']) + : esc($item['item_id']); } return character_limiter($result, 40); diff --git a/tests/Controllers/ItemKitsControllerTest.php b/tests/Controllers/ItemKitsControllerTest.php new file mode 100644 index 000000000..566516c40 --- /dev/null +++ b/tests/Controllers/ItemKitsControllerTest.php @@ -0,0 +1,139 @@ +DBGroup)->call('App\Database\Seeds\TestDatabaseBootstrapSeeder'); + Config::connect($this->DBGroup)->close(); + + self::$doneBootstrap = true; + } + + parent::setUp(); + + $ospos = new OSPOS(); + $ospos->settings = [ + 'company' => 'Test Co', + 'barcode_content' => 'id', + 'barcode_type' => 'C128', + 'barcode_font' => 'inconsolata.ttf', + 'barcode_font_size' => 10, + 'barcode_height' => 40, + 'barcode_width' => 2, + 'barcode_first_row' => 'item_code', + 'barcode_second_row' => 'none', + 'barcode_third_row' => 'none', + 'barcode_num_in_row' => 1, + 'barcode_page_width' => 8, + 'barcode_page_cellspacing' => 1, + 'barcode_generate_if_empty' => 0, + 'barcode_formats' => 'null', + ]; + Factories::injectMock('config', OSPOS::class, $ospos); + + $this->item = model(Item::class); + $this->itemKit = model(Item_kit::class); + } + + protected function tearDown(): void + { + Factories::reset(); + parent::tearDown(); + } + + protected function loginAsAdmin(): void + { + $this->withSession([ + 'person_id' => 1, + 'menu_group' => 'office' + ]); + } + + private function createItemKit(): int + { + $itemData = [ + 'item_id' => null, + 'name' => 'Kit Base Item', + 'category' => 'Test', + 'cost_price' => 10.00, + 'unit_price' => 20.00, + 'deleted' => 0 + ]; + $this->assertTrue($this->item->save_value($itemData)); + + $itemKitData = [ + 'name' => 'Test Kit', + 'description' => 'Test Kit Description', + 'item_id' => $itemData['item_id'], + 'kit_discount' => 0, + 'kit_discount_type' => 0, + 'price_option' => 0, + 'print_option' => 0 + ]; + $this->assertTrue($this->itemKit->save_value($itemKitData)); + + return (int) $itemKitData['item_kit_id']; + } + + /** + * @throws Exception + */ + public function testGenerateBarcodesDoesNotDecodeTripleEncodedPayload(): void + { + $itemKitId = $this->createItemKit(); + $this->loginAsAdmin(); + + // URL-encoded three times (GHSA-3vpv-jqr3-7256 PoC). + // The framework's router decodes this twice before routing; the controller used to apply + // a third urldecode(), turning the remaining %3C.../%3E into a live tag. + // With that urldecode() removed, the value must stay percent-encoded text and never + // become a raw '<' in the response. + $payload = $itemKitId . '%25253Csvg%252520onload%25253Dalert%252528document.domain%252529%25253E'; + + $response = $this->get('/item_kits/generateBarcodes/' . $payload); + $response->assertStatus(200); + + $body = $response->getBody(); + $this->assertStringNotContainsString('assertStringContainsString('%3Csvg', $body); + } + + public function testGenerateBarcodesWorksForPlainItemKitId(): void + { + $itemKitId = $this->createItemKit(); + $this->loginAsAdmin(); + + $response = $this->get('/item_kits/generateBarcodes/' . $itemKitId); + + $response->assertStatus(200); + $this->assertStringContainsString('KIT ' . $itemKitId, $response->getBody()); + } +} diff --git a/tests/Libraries/Barcode_libTest.php b/tests/Libraries/Barcode_libTest.php new file mode 100644 index 000000000..4e424d4a9 --- /dev/null +++ b/tests/Libraries/Barcode_libTest.php @@ -0,0 +1,127 @@ +barcodeLib = new Barcode_lib(); + } + + private function baseBarcodeConfig(string $layout): array + { + return [ + 'company' => 'Test Co', + 'barcode_content' => 'id', + 'barcode_type' => 'C128', + 'barcode_font' => 'inconsolata.ttf', + 'barcode_font_size' => 10, + 'barcode_height' => 40, + 'barcode_width' => 2, + 'barcode_first_row' => $layout, + 'barcode_second_row' => 'none', + 'barcode_third_row' => 'none', + 'barcode_num_in_row' => 1, + 'barcode_page_width' => 8, + 'barcode_page_cellspacing' => 1, + 'barcode_generate_if_empty' => 0, + 'barcode_formats' => [], + ]; + } + + public function testNamePayloadIsEscaped(): void + { + $item = [ + 'name' => '', + 'item_id' => '1', + ]; + + $result = $this->barcodeLib->display_barcode($item, $this->baseBarcodeConfig('name')); + + $this->assertStringNotContainsString('assertStringContainsString('<svg', $result); + } + + public function testItemCodeIdPayloadIsEscaped(): void + { + $item = [ + 'name' => 'Item Name', + 'item_id' => 'KIT 1', + ]; + + $config = $this->baseBarcodeConfig('item_code'); + $config['barcode_content'] = 'id'; + + $result = $this->barcodeLib->display_barcode($item, $config); + + $this->assertStringNotContainsString('assertStringContainsString('<svg', $result); + } + + public function testItemCodeNumberPayloadIsEscaped(): void + { + $item = [ + 'name' => 'Item Name', + 'item_id' => '1', + 'item_number' => 'KIT 1', + ]; + + $config = $this->baseBarcodeConfig('item_code'); + $config['barcode_content'] = 'item_number'; + + $result = $this->barcodeLib->display_barcode($item, $config); + + $this->assertStringNotContainsString('assertStringContainsString('<svg', $result); + } + + public function testCategoryPayloadIsEscaped(): void + { + $item = [ + 'name' => 'Item Name', + 'item_id' => '1', + 'category' => '', + ]; + + $result = $this->barcodeLib->display_barcode($item, $this->baseBarcodeConfig('category')); + + $this->assertStringNotContainsString('assertStringContainsString('<svg', $result); + } + + public function testCompanyNamePayloadIsEscaped(): void + { + $item = [ + 'name' => 'Item Name', + 'item_id' => '1', + ]; + + $config = $this->baseBarcodeConfig('company_name'); + $config['company'] = ''; + + $result = $this->barcodeLib->display_barcode($item, $config); + + $this->assertStringNotContainsString('assertStringContainsString('<svg', $result); + } + + public function testCleanNameIsUnaffected(): void + { + $item = [ + 'name' => 'Widget A', + 'item_id' => '1', + ]; + + $result = $this->barcodeLib->display_barcode($item, $this->baseBarcodeConfig('name')); + + $this->assertStringContainsString('Widget A', $result); + } +}