fix: don't save a masked custom field over the secret it stands for

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,

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.