fix(items): add explicit sentinel value for clearing supplier in bulk edit (#4617)

* fix(items): add explicit sentinel value for clearing supplier in bulk edit

This fixes a regression introduced in the fix for [REDACTED]
Introduce `Item::CLEAR_SUPPLIER_OPTION = 'NONE'` to distinguish between
\"leave supplier_id unchanged\" (empty string) and \"clear supplier_id\"
(sentinel). Previously, empty string was ambiguous.

- Add `CLEAR_SUPPLIER_OPTION` constant with doc comment explaining intent
- Update supplier dropdown to include sentinel as first real option
- Shift empty string to mean \"do nothing\" across all bulk edit fields

* test(items): add regression tests for mass assignment in bulk edit

Cover [REDACTED]: Item::update_multiple() bypasses model
$allowedFields via Query Builder, allowing unintended field writes
during bulk edit operations.

* fix(items): add type validation to bulk-edit field filter

filterBulkEditFields now validates field values before accepting them:
- Non-scalar values are rejected (array injection guard)
- Price/quantity fields are locale-parsed to floats, invalid strings skipped
- Boolean fields must be 0 or 1, other values skipped
- supplier_id must be numeric; CLEAR_SUPPLIER_OPTION still nulls it

Update tests to assert parsed types (float for prices, int for
supplier_id) and replace the fill-all-fields fixture with a realistic
input that only covers fields a form would actually submit.

* test(items): add supplier cleanup and helper methods to bulk update tests

- Track created supplier person IDs for teardown cleanup
- Delete supplier records in tearDown to prevent test pollution
- Extract item/supplier creation into reusable helper methods

* style(tests): rename variables to camelCase in ItemBulkUpdateTest

* refactor(items): rename snake_case variables to camelCase

Convert Item model, Items controller, and bulk update tests to
PSR-compliant camelCase naming per project conventions.

- Rename update_multiple to updateMultiple in Item model
- Rename local variables (item_data, items_to_update, tax_names, etc.)
  to camelCase across Items controller and Item model
- Update ItemBulkUpdateTest to use new updateMultiple method name
- Reorder and update AGENTS.md naming conventions

* style(tests): convert snake_case variables to camelCase in ItemBulkUpdateTest

Rename local variables and property names to camelCase for PSR-12
consistency, matching convention used elsewhere in new test code.
This commit is contained in:
objecttothis authored and GitHub committed 2026-08-20 11:24:50 +04:00
1 parent 61bb1a2c2a
commit cc93c31355
4 files changed
+441 -28

No files matched your search

+2
View File
@@ -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()`)
+18 -24
View File
@@ -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')]);
}
+68 -4
View File
@@ -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);
}
/**
+353
View File
@@ -0,0 +1,353 @@
<?php
namespace Tests\Models;
use App\Models\Item;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
/**
* Regression coverage for GHSA-49mq-h2g4-grr9 (mass assignment in bulk edit).
*
* Item::update_multiple() writes through the Query Builder, which bypasses the
* model's $allowedFields, so these assertions go straight to the items table.
*/
class ItemBulkUpdateTest extends CIUnitTestCase
{
use DatabaseTestTrait;
/**
* The schema is taken as it stands rather than migrated, so these tests run
* against an existing test database without dropping it. Every row created
* here is rolled back in tearDown().
*/
protected $migrate = false;
protected $refresh = false;
protected $namespace = null;
protected Item $item;
/** @var list<int> */
private array $createdItemIds = [];
/** @var list<int> */
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'
);
}
}