From eb902121a153207b983a634bf6bd73783f31edbe Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 00:05:34 +0200 Subject: [PATCH] fix: don't save a masked custom field over the secret it stands for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A custom field of type password is rendered with `value="***"` when the viewer lacks CUSTOMFIELD_VIEW_PASS (aux-customfields.inc), and the browser posts that back verbatim whenever the form is saved without the field being touched. Nothing on the save path recognised it, so the mask was encrypted and stored over the real value — which is not recoverable. The consequence is not limited to custom field forms. Every item that carries custom fields goes through this trait, so editing anything at all about an account, user, client, category, group, profile or auth token destroyed every custom field secret on it that the editor was not allowed to see. The editor got no indication: the save succeeded, and the field still showed `***` afterwards, because that is what it now contained. The rule was already settled here. ConfigLdap, ConfigGeneral and ConfigMail each refuse the same value on the way in before saving a password, and the mask itself already has a name — CustomField::MASKED — used by the API adapter that produces it. This applies it at the door that had never been given it, comparing against that constant rather than a fourth copy of the literal. Both paths are guarded, not just the update. A copy is created rather than updated, and the copy form is prefilled from the original, so a secret the copier may not see arrives at the create path masked and would become the new item's stored value. The cost is that a custom field cannot deliberately be set to exactly three asterisks. That is the trade the three config forms already make. Checked by removing both guards: all three new tests fail. The first attempt at the create test did not — it asserted on updateOrCreate, which is the update path's method; the create path calls create(). It is included as written now, asserting the method that is actually reached, and the third test pins that the fields either side of a masked one are still saved, so a guard that dropped everything would not satisfy these. --- src/Domain/Common/Adapters/ItemTrait.php | 24 +++++++ .../Domain/Common/Adapters/ItemTraitTest.php | 68 +++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/src/Domain/Common/Adapters/ItemTrait.php b/src/Domain/Common/Adapters/ItemTrait.php index 1e2d537fb..e7a95df22 100644 --- a/src/Domain/Common/Adapters/ItemTrait.php +++ b/src/Domain/Common/Adapters/ItemTrait.php @@ -30,6 +30,7 @@ use SP\Domain\Common\Services\ServiceException; use SP\Domain\Core\Dtos\ItemSearchDto; use SP\Domain\Core\Exceptions\SPException; +use SP\Domain\CustomField\Adapters\CustomField; use SP\Domain\CustomField\Models\CustomFieldData as CustomFieldDataModel; use SP\Application\CustomField\Ports\CustomFieldDataService; use SP\Domain\CustomField\Services\CustomFieldItem; @@ -128,6 +129,14 @@ protected function addCustomFieldsForItem( if (!empty($customFields)) { foreach ($customFields as $id => $value) { + // Reached with a mask when an item is copied rather than created from blank: the + // copy form is prefilled from the original, so a secret the copier may not see + // arrives here masked and would become the new item's stored value. Same rule as + // the update path below, which explains it. + if ($value === CustomField::MASKED) { + continue; + } + $customFieldData = new CustomFieldDataModel( [ 'itemId' => $itemId, @@ -199,6 +208,21 @@ protected function updateCustomFieldsForItem( if (!empty($customFields)) { foreach ($customFields as $id => $value) { + // The form shows a secret the viewer may not see as CustomField::MASKED, and the + // browser posts that back verbatim whenever the form is saved without the field + // being touched. Storing it encrypts the mask over the secret it stood in for, and + // the original is not recoverable: editing anything else about an item destroyed + // every custom field secret on it that the editor was not allowed to see. + // + // The three config forms that mask a password already refuse it on the way in — + // ConfigLdap, ConfigGeneral and ConfigMail each compare against this same value — + // so this is a rule the codebase had already settled, applied at the door that was + // never given it. The cost is that a field cannot deliberately be set to exactly + // the mask, which is the trade those three already make. + if ($value === CustomField::MASKED) { + continue; + } + $customFieldData = new CustomFieldDataModel( [ 'itemId' => $itemId, diff --git a/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php b/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php index 273329925..53f31afaf 100644 --- a/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php +++ b/tests/Unit/Domain/Common/Adapters/ItemTraitTest.php @@ -34,6 +34,7 @@ use SP\Domain\Common\Models\Simple; use SP\Domain\Core\Dtos\ItemSearchDto; use SP\Domain\CustomField\Models\CustomFieldData; +use SP\Domain\CustomField\Adapters\CustomField; use SP\Domain\CustomField\Services\CustomFieldItem; use SP\Domain\Http\Ports\RequestService; use SP\Tests\Support\UnitaryTestCase; @@ -239,6 +240,73 @@ public function clearingAFieldDeletesItRatherThanSavingItEmpty() $this->host->updateCustomFields(self::MODULE_ID, 7, $this->givenThePostedFields([4 => '']), $service); } + /** + * A masked value is not saved over the secret it stands in for. + * + * The form renders a secret the viewer may not see as `***`, and the browser posts that back + * verbatim when the form is saved without the field being touched. Storing it encrypted the + * mask over the real value, unrecoverably — so editing anything else about an item destroyed + * every custom field secret on it that the editor was not allowed to see. + */ + #[Test] + public function updatingDoesNotSaveTheMaskOverTheSecretItStandsFor() + { + $service = $this->createMock(CustomFieldDataService::class); + $service->expects(self::never())->method('updateOrCreate'); + $service->expects(self::never())->method('delete'); + + $this->host->updateCustomFields( + self::MODULE_ID, + 7, + $this->givenThePostedFields([4 => CustomField::MASKED]), + $service + ); + } + + /** + * Nor is one copied into a new item. The copy form is prefilled from the original, so a secret + * the copier may not see arrives masked and would otherwise become the copy's stored value. + */ + #[Test] + public function creatingDoesNotSaveTheMaskOverTheSecretItStandsFor() + { + $service = $this->createMock(CustomFieldDataService::class); + $service->expects(self::never())->method('create'); + + $this->host->addCustomFields( + self::MODULE_ID, + 7, + $this->givenThePostedFields([4 => CustomField::MASKED]), + $service + ); + } + + /** + * The fields either side of a masked one are still saved — the mask is skipped, not the + * request. Without this the test above is satisfied by a guard that drops everything. + */ + #[Test] + public function aMaskedFieldDoesNotStopTheOthersBeingSaved() + { + $saved = []; + + $service = $this->createStub(CustomFieldDataService::class); + $service->method('updateOrCreate')->willReturnCallback( + static function ($customFieldData) use (&$saved): void { + $saved[] = $customFieldData->getDefinitionId(); + } + ); + + $this->host->updateCustomFields( + self::MODULE_ID, + 7, + $this->givenThePostedFields([4 => 'kept', 5 => CustomField::MASKED, 6 => 'also kept']), + $service + ); + + self::assertSame([4, 6], $saved); + } + /** * Deleting an item takes its fields with it, and a single id is accepted where the service * wants a list.