diff --git a/AGENTS.md b/AGENTS.md index b0ce62bb6..d36edae57 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,8 @@ This document provides guidance for AI agents working on the Open Source Point o - **PSR-12** enforced via PHP-CS-Fixer (config: `.php-cs-fixer.no-header.php`) - Follow PHP CodeIgniter 4 coding standards - `camelCase` for variables and methods; `PascalCase` for classes; `UPPER_CASE` for constants +- When editing existing code containing non-PSR-compliant local variable names, refactor those variable names to `camelCase` as part of the edit +- All newly written code (variables, classes, functions) must use PSR-compliant naming, regardless of surrounding code style - PHP 8.2+ features acceptable (named arguments, enums, readonly properties) - Write PHP 8.2+ compatible code with proper type declarations - Always import classes, functions, and constants with a `use` statement at the top of the file instead of referencing them inline via fully-qualified name (e.g. `use Config\Database;` then `Database::connect()`, not `\Config\Database::connect()`) diff --git a/app/Controllers/Items.php b/app/Controllers/Items.php index 25c9ae139..98ddc0391 100644 --- a/app/Controllers/Items.php +++ b/app/Controllers/Items.php @@ -594,7 +594,10 @@ class Items extends Secure_Controller */ public function getBulkEdit(): string { - $suppliers = ['' => lang('Items.none')]; + $suppliers = [ + '' => lang('Items.do_nothing'), + Item::CLEAR_SUPPLIER_OPTION => lang('Items.none') + ]; foreach ($this->supplier->get_all()->getResultArray() as $row) { $suppliers[$row['person_id']] = $row['company_name']; @@ -923,37 +926,28 @@ class Items extends Secure_Controller */ public function postBulkUpdate(): ResponseInterface { - $items_to_update = $this->request->getPost('item_ids'); - $item_data = []; - - foreach (Item::ALLOWED_BULK_EDIT_FIELDS as $field) { - $value = $this->request->getPost($field); - if ($field === 'supplier_id' && $value !== '') { - $item_data[$field] = $value; - } elseif ($value !== null && $value !== '') { - $item_data[$field] = $value; - } - } + $itemsToUpdate = $this->request->getPost('item_ids'); + $itemData = Item::filterBulkEditFields($this->request->getPost() ?? []); // Item data could be empty if tax information is being updated - if (empty($item_data) || $this->item->update_multiple($item_data, $items_to_update)) { - $items_taxes_data = []; - $tax_names = $this->request->getPost('tax_names'); - $tax_percents = $this->request->getPost('tax_percents'); - $tax_updated = false; + if (empty($itemData) || $this->item->updateMultiple($itemData, $itemsToUpdate)) { + $itemsTaxesData = []; + $taxNames = $this->request->getPost('tax_names'); + $taxPercents = $this->request->getPost('tax_percents'); + $taxUpdated = false; - foreach ($tax_percents as $tax_percent) { - if (!empty($tax_names[$tax_percent]) && is_numeric($tax_percents[$tax_percent])) { - $tax_updated = true; - $items_taxes_data[] = ['name' => $tax_names[$tax_percent], 'percent' => $tax_percents[$tax_percent]]; + foreach ($taxPercents as $tax_percent) { + if (!empty($taxNames[$tax_percent]) && is_numeric($taxPercents[$tax_percent])) { + $taxUpdated = true; + $itemsTaxesData[] = ['name' => $taxNames[$tax_percent], 'percent' => $taxPercents[$tax_percent]]; } } - if ($tax_updated) { - $this->item_taxes->save_multiple($items_taxes_data, $items_to_update); + if ($taxUpdated) { + $this->item_taxes->save_multiple($itemsTaxesData, $itemsToUpdate); } - return $this->response->setJSON(['success' => true, 'message' => lang('Items.successful_bulk_edit'), 'id' => $items_to_update]); + return $this->response->setJSON(['success' => true, 'message' => lang('Items.successful_bulk_edit'), 'id' => $itemsToUpdate]); } else { return $this->response->setJSON(['success' => false, 'message' => lang('Items.error_updating_multiple')]); } diff --git a/app/Models/Item.php b/app/Models/Item.php index efb8531d0..dac42e59c 100644 --- a/app/Models/Item.php +++ b/app/Models/Item.php @@ -20,6 +20,13 @@ class Item extends Model public const ALLOWED_SUGGESTIONS_COLUMNS = ['name', 'item_number', 'description', 'cost_price', 'unit_price']; public const ALLOWED_SUGGESTIONS_COLUMNS_WITH_EMPTY = ['', 'name', 'item_number', 'description', 'cost_price', 'unit_price']; + /** + * Sentinel posted by the bulk edit form to clear supplier_id, since an empty + * value there means "leave the column alone". Non-numeric so it can never + * collide with a suppliers.person_id. + */ + public const CLEAR_SUPPLIER_OPTION = 'NONE'; + public const ALLOWED_BULK_EDIT_FIELDS = [ 'name', 'category', @@ -467,15 +474,72 @@ class Item extends Model return $builder->update($item_data); } + /** + * Reduces raw bulk edit input to the columns that may be bulk updated. + * + * Keys outside ALLOWED_BULK_EDIT_FIELDS are dropped, and a field that is absent, + * empty, or invalid for its column is left untouched so it keeps its current + * value. Prices and quantities are locale-parsed the same way postSave() does, + * booleans must be 0/1, and supplier_id must be numeric. supplier_id is + * nullable, so CLEAR_SUPPLIER_OPTION is how the form asks for it to be cleared. + */ + public static function filterBulkEditFields(array $input): array + { + $itemData = []; + + foreach (self::ALLOWED_BULK_EDIT_FIELDS as $field) { + $value = $input[$field] ?? null; + + if ($value === null || $value === '' || !is_scalar($value)) { + continue; + } + + if ($field === 'supplier_id') { + if ($value === self::CLEAR_SUPPLIER_OPTION) { + $itemData[$field] = null; + } elseif (ctype_digit((string)$value)) { + $itemData[$field] = (int)$value; + } + + continue; + } + + if ($field === 'cost_price' || $field === 'unit_price') { + $value = parse_decimals((string)$value); + } elseif ($field === 'reorder_level') { + $value = parse_quantity((string)$value); + } elseif ($field === 'allow_alt_description' || $field === 'is_serialized') { + if (!in_array((string)$value, ['0', '1'], true)) { + continue; + } + } + + if ($value === false) { + continue; + } + + $itemData[$field] = $value; + } + + return $itemData; + } + /** * Updates multiple items at once */ - public function update_multiple(array $item_data, string $item_ids): bool + public function updateMultiple(array $itemData, string $itemIds): bool { - $builder = $this->db->table('items'); - $builder->whereIn('item_id', explode(':', $item_ids)); + // Query Builder bypasses $allowedFields, so the whitelist is enforced here (GHSA-49mq-h2g4-grr9) + $itemData = array_intersect_key($itemData, array_flip(self::ALLOWED_BULK_EDIT_FIELDS)); - return $builder->update($item_data); + if (empty($itemData)) { + return false; + } + + $builder = $this->db->table('items'); + $builder->whereIn('item_id', explode(':', $itemIds)); + + return $builder->update($itemData); } /** diff --git a/tests/Models/ItemBulkUpdateTest.php b/tests/Models/ItemBulkUpdateTest.php new file mode 100644 index 000000000..a48e2d547 --- /dev/null +++ b/tests/Models/ItemBulkUpdateTest.php @@ -0,0 +1,353 @@ + */ + private array $createdItemIds = []; + + /** @var list */ + private array $createdSupplierPersonIds = []; + + protected function setUp(): void + { + parent::setUp(); + + $this->item = model(Item::class); + } + + protected function tearDown(): void + { + if ($this->createdItemIds !== []) { + $this->db->table('items')->whereIn('item_id', $this->createdItemIds)->delete(); + $this->createdItemIds = []; + } + + if ($this->createdSupplierPersonIds !== []) { + $this->db->table('suppliers')->whereIn('person_id', $this->createdSupplierPersonIds)->delete(); + $this->db->table('people')->whereIn('person_id', $this->createdSupplierPersonIds)->delete(); + $this->createdSupplierPersonIds = []; + } + + parent::tearDown(); + } + + /** + * Creates an item directly and returns its id. + */ + private function createItem(array $overrides = []): int + { + $itemData = array_merge([ + 'name' => 'Bulk Edit Fixture', + 'category' => 'Fixtures', + 'cost_price' => 10.00, + 'unit_price' => 20.00, + 'reorder_level' => 1, + 'description' => 'fixture', + 'allow_alt_description' => 0, + 'is_serialized' => 0, + 'deleted' => 0, + 'item_type' => 0, + 'stock_type' => 0, + 'hsn_code' => '' + ], $overrides); + + $this->item->save_value($itemData); + + $itemId = (int)$itemData['item_id']; + $this->createdItemIds[] = $itemId; + + return $itemId; + } + + /** + * Creates a supplier (people + suppliers rows, to satisfy the items FK chain) + * and returns its person_id. + */ + private function createSupplier(): int + { + $this->db->table('people')->insert([ + 'first_name' => 'Bulk', + 'last_name' => 'Supplier', + 'phone_number' => '555-0100', + 'email' => 'bulk.supplier@example.test', + 'address_1' => '1 Fixture Way', + 'address_2' => '', + 'city' => 'Testville', + 'state' => 'TS', + 'zip' => '00000', + 'country' => 'US', + 'comments' => 'fixture' + ]); + + $personId = (int)$this->db->insertID(); + + $this->db->table('suppliers')->insert([ + 'person_id' => $personId, + 'company_name' => 'Bulk Edit Supplies', + 'agency_name' => '', + 'category' => 0 + ]); + + $this->createdSupplierPersonIds[] = $personId; + + return $personId; + } + + private function fetchItem(int $itemId): array + { + return (array)$this->db->table('items') + ->where('item_id', $itemId) + ->get() + ->getRow(); + } + + public function testUpdateMultipleIgnoresNonWhitelistedColumns(): void + { + $itemId = $this->createItem(); + + $result = $this->item->updateMultiple([ + 'unit_price' => 25.00, // legitimate + 'deleted' => 1, // injected — would hide the item + 'item_type' => 2, // injected — would change item semantics + 'stock_type' => 1 // injected + ], (string)$itemId); + + $this->assertTrue($result, 'The legitimate field should still be applied'); + + $row = $this->fetchItem($itemId); + + $this->assertEquals(25.00, (float)$row['unit_price'], 'Whitelisted field should be updated'); + $this->assertEquals(0, (int)$row['deleted'], 'Injected deleted must be ignored'); + $this->assertEquals(0, (int)$row['item_type'], 'Injected item_type must be ignored'); + $this->assertEquals(0, (int)$row['stock_type'], 'Injected stock_type must be ignored'); + } + + public function testUpdateMultipleWithOnlyNonWhitelistedColumnsIsNoOp(): void + { + $itemId = $this->createItem(); + + $result = $this->item->updateMultiple(['deleted' => 1], (string)$itemId); + + $this->assertFalse($result, 'An update of only disallowed columns should not run'); + $this->assertEquals(0, (int)$this->fetchItem($itemId)['deleted'], 'Item must not be soft deleted'); + } + + public function testUpdateMultipleAppliesToEveryColonSeparatedId(): void + { + $first = $this->createItem(); + $second = $this->createItem(); + + $this->item->updateMultiple(['category' => 'Regrouped'], "$first:$second"); + + $this->assertEquals('Regrouped', $this->fetchItem($first)['category']); + $this->assertEquals('Regrouped', $this->fetchItem($second)['category']); + } + + public function testUpdateMultipleClearsSupplierWhenPassedNull(): void + { + $supplierId = $this->createSupplier(); + $itemId = $this->createItem(['supplier_id' => $supplierId]); + + $this->assertEquals( + $supplierId, + (int)$this->fetchItem($itemId)['supplier_id'], + 'precondition: the item must start with a supplier' + ); + + $filtered = Item::filterBulkEditFields(['supplier_id' => Item::CLEAR_SUPPLIER_OPTION]); + $this->item->updateMultiple($filtered, (string)$itemId); + + $this->assertNull($this->fetchItem($itemId)['supplier_id'], 'supplier_id should be cleared'); + } + + public function testBulkEditWhitelistExcludesSensitiveColumns(): void + { + foreach (['deleted', 'item_type', 'stock_type', 'item_number', 'pic_filename', 'tax_category_id'] as $column) { + $this->assertNotContains( + $column, + Item::ALLOWED_BULK_EDIT_FIELDS, + "Sensitive column should not be bulk editable: $column" + ); + } + } + + public function testBulkEditWhitelistFieldsAreAllRealColumns(): void + { + foreach (Item::ALLOWED_BULK_EDIT_FIELDS as $field) { + $this->assertContains( + $field, + $this->db->getFieldNames('items'), + "Whitelisted bulk edit field must exist on the items table: $field" + ); + } + } + + // ========== Item::filterBulkEditFields() — the controller's input filter ========== + + public function testFilterBulkEditFieldsDropsInjectedColumns(): void + { + $filtered = Item::filterBulkEditFields([ + 'item_ids' => '1', + 'unit_price' => '25.00', + 'deleted' => '1', + 'item_type' => '2', + 'stock_type' => '1' + ]); + + $this->assertSame(['unit_price' => 25.0], $filtered); + } + + public function testFilterBulkEditFieldsOmitsAbsentSupplier(): void + { + $filtered = Item::filterBulkEditFields(['unit_price' => '30.00']); + + $this->assertArrayNotHasKey( + 'supplier_id', + $filtered, + 'An absent supplier_id must not be written, or bulk edits would clear suppliers' + ); + } + + public function testFilterBulkEditFieldsTreatsEmptySupplierAsDoNothing(): void + { + $filtered = Item::filterBulkEditFields(['supplier_id' => '', 'unit_price' => '30.00']); + + $this->assertArrayNotHasKey('supplier_id', $filtered, 'An empty supplier_id means do nothing'); + } + + public function testFilterBulkEditFieldsClearsSupplierWithSentinel(): void + { + $filtered = Item::filterBulkEditFields(['supplier_id' => Item::CLEAR_SUPPLIER_OPTION]); + + $this->assertArrayHasKey('supplier_id', $filtered); + $this->assertNull($filtered['supplier_id'], 'The sentinel should become a NULL write'); + } + + public function testFilterBulkEditFieldsKeepsSelectedSupplier(): void + { + $filtered = Item::filterBulkEditFields(['supplier_id' => '7']); + + $this->assertSame(['supplier_id' => 7], $filtered); + } + + public function testFilterBulkEditFieldsSkipsEmptyValuesForEveryField(): void + { + $input = array_fill_keys(Item::ALLOWED_BULK_EDIT_FIELDS, ''); + + $this->assertSame([], Item::filterBulkEditFields($input), 'An untouched form should update nothing'); + } + + public function testFilterBulkEditFieldsAcceptsEveryWhitelistedField(): void + { + $input = [ + 'name' => 'Renamed', + 'category' => 'Regrouped', + 'supplier_id' => '7', + 'cost_price' => '1.50', + 'unit_price' => '2.50', + 'reorder_level' => '3', + 'description' => 'described', + 'allow_alt_description' => '1', + 'is_serialized' => '1' + ]; + + $this->assertSame( + Item::ALLOWED_BULK_EDIT_FIELDS, + array_keys(Item::filterBulkEditFields($input)), + 'Every whitelisted field should still be editable' + ); + } + + public function testFilterBulkEditFieldsDropsArrayValues(): void + { + $input = array_fill_keys(Item::ALLOWED_BULK_EDIT_FIELDS, ['x']); + + $this->assertSame([], Item::filterBulkEditFields($input), 'Array values must never reach the update'); + } + + public function testFilterBulkEditFieldsDropsNonNumericPricesAndQuantities(): void + { + $filtered = Item::filterBulkEditFields([ + 'cost_price' => 'abc', + 'unit_price' => 'DROP TABLE', + 'reorder_level' => 'lots', + 'name' => 'Renamed' + ]); + + $this->assertSame(['name' => 'Renamed'], $filtered, 'Unparseable numbers must be omitted'); + } + + public function testFilterBulkEditFieldsDropsInvalidBooleanValues(): void + { + $filtered = Item::filterBulkEditFields([ + 'allow_alt_description' => '2', + 'is_serialized' => 'yes' + ]); + + $this->assertSame([], $filtered, 'Booleans other than 0/1 must be omitted'); + } + + public function testFilterBulkEditFieldsDropsNonNumericSupplier(): void + { + $filtered = Item::filterBulkEditFields(['supplier_id' => '7; DROP TABLE items']); + + $this->assertSame([], $filtered, 'A non-numeric supplier_id other than the sentinel must be omitted'); + } + + public function testFilterBulkEditFieldsAcceptsZeroValues(): void + { + $filtered = Item::filterBulkEditFields(['allow_alt_description' => '0', 'is_serialized' => '0']); + + $this->assertSame(['allow_alt_description' => '0', 'is_serialized' => '0'], $filtered); + } + + public function testFilterBulkEditFieldsOutputIsSafeForUpdateMultiple(): void + { + $itemId = $this->createItem(); + + $filtered = Item::filterBulkEditFields([ + 'item_ids' => (string)$itemId, + 'deleted' => '1', + 'name' => 'Renamed' + ]); + + $this->item->updateMultiple($filtered, (string)$itemId); + + $row = $this->fetchItem($itemId); + $this->assertEquals('Renamed', $row['name']); + $this->assertEquals(0, (int)$row['deleted'], 'Injected deleted must never reach the table'); + } + + public function testClearSupplierSentinelCannotCollideWithAPersonId(): void + { + $this->assertFalse( + is_numeric(Item::CLEAR_SUPPLIER_OPTION), + 'The clear-supplier sentinel must be non-numeric so it cannot match a person_id' + ); + } +}