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.