From 9b9895e2e127580ecd8363c2f46a031a4e22eff6 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sat, 31 Oct 2026 23:43:37 +0100 Subject: [PATCH] fix(Model): protect fields against mass assignment in replace() --- system/BaseModel.php | 35 +++++- system/Model.php | 36 ++++++ tests/system/Models/ReplaceModelTest.php | 111 ++++++++++++++++++ user_guide_src/source/changelogs/v4.7.6.rst | 6 + .../source/installation/upgrade_476.rst | 13 ++ 5 files changed, 199 insertions(+), 2 deletions(-) diff --git a/system/BaseModel.php b/system/BaseModel.php index ad79f6f42ec6..a5f9800fbdd8 100644 --- a/system/BaseModel.php +++ b/system/BaseModel.php @@ -1265,6 +1265,8 @@ public function onlyDeleted() * @param bool $returnSQL `true` means SQL is returned, `false` will execute the query. * * @return BaseResult|false|Query|string + * + * @throws DataException */ public function replace(?array $row = null, bool $returnSQL = false) { @@ -1274,8 +1276,20 @@ public function replace(?array $row = null, bool $returnSQL = false) } $row = (array) $row; - $row = $this->setCreatedField($row, $this->setDate()); - $row = $this->setUpdatedField($row, $this->setDate()); + + // Must be called first, so we don't + // strip out created_at values. + $row = $this->doProtectFieldsForReplace($row); + + // doProtectFields() can further remove elements from + // $row, so we need to check for empty dataset again + if ($row === []) { + throw DataException::forEmptyDataset('replace'); + } + + $date = $this->setDate(); + $row = $this->setCreatedField($row, $date); + $row = $this->setUpdatedField($row, $date); return $this->doReplace($row, $returnSQL); } @@ -1414,6 +1428,23 @@ protected function doProtectFieldsForInsert(array $row): array return $this->doProtectFields($row); } + /** + * Ensures that only the fields that are allowed to be replaced are in + * the data array. + * + * @used-by replace() to protect against mass assignment vulnerabilities. + * + * @param row_array $row + * + * @return row_array + * + * @throws DataException + */ + protected function doProtectFieldsForReplace(array $row): array + { + return $this->doProtectFields($row); + } + /** * Sets the timestamp or current timestamp if null value is passed. * diff --git a/system/Model.php b/system/Model.php index c73cdba448c7..999f7644fbd5 100644 --- a/system/Model.php +++ b/system/Model.php @@ -695,6 +695,42 @@ protected function doProtectFieldsForInsert(array $row): array return $row; } + /** + * Ensures that only the fields that are allowed to be replaced are in + * the data array. + * + * Used by replace() to protect against mass assignment vulnerabilities. + * + * @param row_array $row + * + * @return row_array + * + * @throws DataException + */ + protected function doProtectFieldsForReplace(array $row): array + { + if (! $this->protectFields) { + return $row; + } + + if ($this->allowedFields === []) { + throw DataException::forInvalidAllowedFields(static::class); + } + + foreach (array_keys($row) as $key) { + // Do not remove the primary key data. + if ($this->primaryKey !== '' && $key === $this->primaryKey) { + continue; + } + + if (! in_array($key, $this->allowedFields, true)) { + unset($row[$key]); + } + } + + return $row; + } + public function update($id = null, $row = null): bool { if (isset($this->tempData['data'])) { diff --git a/tests/system/Models/ReplaceModelTest.php b/tests/system/Models/ReplaceModelTest.php index 776fcf7647fb..d5200d6af2f0 100644 --- a/tests/system/Models/ReplaceModelTest.php +++ b/tests/system/Models/ReplaceModelTest.php @@ -13,6 +13,7 @@ namespace CodeIgniter\Models; +use CodeIgniter\Database\Exceptions\DataException; use PHPUnit\Framework\Attributes\Group; use Tests\Support\Models\UserModel; @@ -47,4 +48,114 @@ public function testReplaceRespectsUseTimestamps(): void $this->assertStringContainsString('created_at', (string) $sql); $this->assertStringContainsString('updated_at', (string) $sql); } + + public function testReplaceProtectsFields(): void + { + $this->createModel(UserModel::class); + + $data = [ + 'name' => 'Amanda Holmes', + 'email' => 'amanda@holmes.com', + 'country' => 'US', + 'not_allowed' => 'hack', + ]; + + $sql = $this->model->replace($data, true); + $this->assertStringNotContainsString('not_allowed', (string) $sql); + $this->assertStringContainsString('email', (string) $sql); + } + + public function testReplacePreservesPrimaryKeyEvenIfNotInAllowedFields(): void + { + $this->createModel(UserModel::class); + + $data = [ + 'id' => 1, + 'name' => 'Amanda Holmes', + 'email' => 'amanda@holmes.com', + 'country' => 'US', + 'not_allowed' => 'hack', + ]; + + $sql = $this->model->replace($data, true); + $this->assertStringContainsString('id', (string) $sql); + $this->assertStringNotContainsString('not_allowed', (string) $sql); + } + + public function testReplaceWithProtectFalseAllowsAllFields(): void + { + $this->createModel(UserModel::class); + + $data = [ + 'name' => 'Amanda Holmes', + 'email' => 'amanda@holmes.com', + 'country' => 'US', + 'not_allowed' => 'hack', + ]; + + $sql = $this->model->protect(false)->replace($data, true); + $this->assertStringContainsString('not_allowed', (string) $sql); + } + + public function testReplaceThrowsDataExceptionWhenNoAllowedFields(): void + { + $this->createModel(UserModel::class); + $this->setPrivateProperty($this->model, 'allowedFields', []); + + $this->expectException(DataException::class); + $this->expectExceptionMessage('Allowed fields must be specified for model: "Tests\Support\Models\UserModel"'); + + $this->model->replace(['id' => 1, 'name' => 'Amanda']); + } + + public function testReplaceThrowsDataExceptionWhenDatasetEmpty(): void + { + $this->createModel(UserModel::class); + + $this->expectException(DataException::class); + $this->expectExceptionMessage('There is no data to replace.'); + + $this->model->replace([]); + } + + public function testReplaceThrowsDataExceptionWhenAllFieldsProtected(): void + { + $this->createModel(UserModel::class); + + $this->expectException(DataException::class); + $this->expectExceptionMessage('There is no data to replace.'); + + $this->model->replace(['not_allowed' => 'hack']); + } + + public function testReplaceLiveExecutionWithUnallowedFields(): void + { + $this->createModel(UserModel::class); + + $data = [ + 'name' => 'Amanda Holmes', + 'email' => 'amanda@holmes.com', + 'country' => 'US', + ]; + + $id = $this->model->insert($data); + + $replaceData = [ + 'id' => $id, + 'name' => 'Amanda Updated', + 'email' => 'amanda@holmes.com', + 'country' => 'UK', + 'not_allowed' => 'ignored', + ]; + + $result = $this->model->replace($replaceData); + $this->assertNotFalse($result); + + $this->seeInDatabase('user', [ + 'id' => $id, + 'name' => 'Amanda Updated', + 'email' => 'amanda@holmes.com', + 'country' => 'UK', + ]); + } } diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index a609e8cbd9f3..e83aa4fb7d27 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -14,6 +14,11 @@ Release Date: Unreleased BREAKING ******** +Behavior Changes +================ + +- **Model:** ``BaseModel::replace()`` now enforces ``$allowedFields`` protection identically to ``insert()`` and ``update()``. Applications passing fields not listed in ``$allowedFields`` will have those fields rejected or throw a ``DataException`` if no valid fields remain. See :ref:`upgrade-476-model-replace-allowed-fields` for details. + *************** Message Changes *************** @@ -52,6 +57,7 @@ Bugs Fixed - **HTTP:** Fixed a bug where ``Negotiate`` could select a value that the request header excluded with ``q=0`` through a wildcard or a broader range. A ``q=0`` range now excludes every value it matches, such as ``text/html`` and ``text/html;charset=utf-8`` for ``text/*;q=0``, or ``en-US`` for ``en;q=0``, unless a more specific range in the header accepts the value. For example, ``Accept-Encoding: gzip;q=0, *`` no longer selects ``gzip``. - **HTTP:** Fixed stale ``$_REQUEST`` values after ``SiteURIFactory`` normalizes GET parameters, so ``Validation::withRequest()`` reads the normalized values while unrelated REQUEST entries are preserved. - **I18n:** Fixed a regression in v4.7.1 where ``Time`` and ``TimeLegacy`` lost their timezone, locale and ``__toString()`` format after ``unserialize()``, so methods such as ``difference()`` and ``getTimezoneName()`` threw an ``Error``. +- **Model:** Fixed a bug where ``BaseModel::replace()`` did not protect fields against mass assignment or check for empty datasets, passing raw input data to the builder. - **Routing:** Corrected the PHPDoc callback type of ``RouteCollection::group()`` to preserve the concrete collection type, allowing callbacks typed to a ``RouteCollection`` subclass to pass static analysis. - **Security:** Fixed a bug where a valid JSON request body that is not an object (for example a JSON array) was corrupted after CSRF verification when the token was sent via the ``X-CSRF-TOKEN`` header. diff --git a/user_guide_src/source/installation/upgrade_476.rst b/user_guide_src/source/installation/upgrade_476.rst index 6e6e0118aa07..a75392832e60 100644 --- a/user_guide_src/source/installation/upgrade_476.rst +++ b/user_guide_src/source/installation/upgrade_476.rst @@ -20,6 +20,19 @@ Mandatory File Changes Breaking Changes **************** +.. _upgrade-476-model-replace-allowed-fields: + +Model::replace() Enforces Allowed Fields +======================================== + +For security and consistency with ``insert()`` and ``update()``, ``BaseModel::replace()`` now enforces mass assignment protection via ``$allowedFields``. + +Previously, unlisted fields were passed directly to the database query without filtering. If your application intentionally passes fields not defined in ``$allowedFields`` to ``replace()``, either add those fields to ``$allowedFields`` or temporarily disable protection before calling ``replace()``: + +.. code-block:: php + + $model->protect(false)->replace($data); + ********************* Breaking Enhancements *********************