fix: mask an encrypted custom field whatever its type

CLAUDE.md

@@ -512,6 +512,20 @@ is left behind when the run fails, since an interrupted backup leaves its interm
 indefinitely. `config/config.xml` is the exception that is fine: it is `0644` itself, but
 `ConfigUtil` holds its *directory* at `0750`, and that is the control.
 
+**A guard inside one arm of a switch, on a flag that does not follow the switch.** The web's custom
+field partial masked the value inside its `typeName === 'password'` branch, and `isEncrypted` is a
+property of the *definition*, set by a checkbox that sits beside the type select and is independent
+of it. So an encrypted textarea — a recovery phrase, an API key, a signed URL — was decrypted by
+`ItemTrait::getCustomFieldsForItem()` and then printed in full to anyone who could open the item,
+whatever `CUSTOMFIELD_VIEW_PASS` said. Four of the five branches leaked; the fifth was the one the
+guard was written in.
+
+This is the two-doors shape with both doors in the same file, and it is easier to miss for that: the
+masking is visibly *there*. **When a guard sits inside a branch, ask what the branch is switching on
+and whether the thing being guarded varies with it.** Here it did not — which is why the API, whose
+`CustomField::valueFor()` decides on `isValueEncrypted` and never looks at the type, was right all
+along. The fix computes the decision once per field, above the switch.
+
 **A guard on the read but not on the write.** `Notification` has the rule written down and named —
 `checkUserAccess()`, admins may reach any notification and regular users only their own, answering
 "not found" so ids cannot be enumerated by the difference. It was called from `getById()` and

public/themes/material-blue/views/common/aux-customfields.inc

@@ -37,7 +37,23 @@ use function SP\__;
 
 $customFields = $_getvar('customFields');
 
-foreach ($customFields as $index => $field):?>
+foreach ($customFields as $index => $field):
+    // What to print in place of the value, decided once for every type rather than only inside
+    // the password branch.
+    //
+    // Masking used to live in that branch alone, and `isEncrypted` is set independently of the
+    // type — the definition form's "Encrypted" checkbox is not tied to the type select — so an
+    // encrypted textarea, or an encrypted text, url or number field, was rendered in the clear to
+    // anybody who could see the item, whatever CUSTOMFIELD_VIEW_PASS said. The API has always got
+    // this right: `CustomField::valueFor()` decides on `isValueEncrypted` and never looks at the
+    // type.
+    //
+    // A password-typed field stays masked whether or not the row is encrypted, which is what this
+    // did before and is the safer reading of somebody having called it a password.
+    $isSecret = $field->isValueEncrypted || $field->typeName === 'password';
+    $showsValue = (bool)$_getvar('showViewCustomPass');
+    $displayValue = !$showsValue && $isSecret && !empty($field->value) ? '***' : $field->value;
+    ?>
     <tr>
         <td class="descField">
             <?php
@@ -89,7 +105,7 @@ foreach ($customFields as $index => $field):?>
             if ($field->typeName === 'color' && $_getvar('isView')): ?>
                 <span class="round custom-input-color"
                       style="background-color: <?php
-                      echo $_e($field->value); ?>;"></span>
+                      echo $_e($displayValue); ?>;"></span>
             <?php
             elseif ($field->typeName === 'password'): ?>
                 <div class="mdl-textfield mdl-js-textfield mdl-textfield--floating-label">
@@ -103,7 +119,7 @@ foreach ($customFields as $index => $field):?>
                            data-clipboard="1"
                            maxlength="500"
                            value="<?php
-                           echo !$_getvar('showViewCustomPass') && !empty($field->value) ? '***' : $_e($field->value); ?>" <?php
+                           echo $_e($displayValue); ?>" <?php
                     echo $field->required ? 'required' : ''; ?> <?php
                     echo $_e($_getvar('readonly')); ?>>
                     <label class="mdl-textfield__label"
@@ -122,7 +138,7 @@ foreach ($customFields as $index => $field):?>
                             id="<?php
                             echo $field->formId; ?>" <?php
                     echo $_e($_getvar('readonly')); ?>><?php
-                        echo $_e($field->value); ?></textarea>
+                        echo $_e($displayValue); ?></textarea>
                     <label class="mdl-textfield__label"
                            for="<?php
                            echo $field->formId; ?>"><?php
@@ -140,7 +156,7 @@ foreach ($customFields as $index => $field):?>
                            class="mdl-textfield__input mdl-color-text--indigo-400"
                            maxlength="500"
                            value="<?php
-                           echo $_e($field->value); ?>" <?php
+                           echo $_e($displayValue); ?>" <?php
                     echo $field->required ? 'required' : ''; ?> <?php
                     echo $_e($_getvar('readonly')); ?>>
                     <label class="mdl-textfield__label"

tests/Unit/Infrastructure/Adapter/In/Web/View/EncryptedCustomFieldsAreMaskedTest.php

@@ -0,0 +1,219 @@
+<?php
+
+declare(strict_types=1);
+/*
+ * sysPass
+ *
+ * @author nuxsmin
+ * @link https://syspass.org
+ * @copyright 2012-2024, Rubén Domínguez nuxsmin@$syspass.org
+ *
+ * This file is part of sysPass.
+ *
+ * sysPass is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * sysPass is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with sysPass.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+namespace SP\Tests\Unit\Infrastructure\Adapter\In\Web\View;
+
+use PHPUnit\Framework\Attributes\Group;
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\Attributes\TestWith;
+use SP\Domain\Core\Bootstrap\UriContextInterface;
+use SP\Domain\Core\Exceptions\FileNotFoundException;
+use SP\Domain\Core\UI\ThemeIconsInterface;
+use SP\Domain\CustomField\Services\CustomFieldItem;
+use SP\Infrastructure\Adapter\In\Web\View\OutputHandler;
+use SP\Infrastructure\Adapter\In\Web\View\Template;
+use SP\Infrastructure\Adapter\In\Web\View\TemplateResolverInterface;
+use SP\Tests\Support\UnitaryTestCase;
+
+/**
+ * Whether a custom field's value is a secret is decided by the row, not by the field's type.
+ *
+ * `isEncrypted` is a property of the *definition* and the type is a separate select beside it, so
+ * "Encrypted" can be — and is — turned on for a textarea holding a recovery phrase, a text field
+ * holding an API key, or a url holding a signed one. `ItemTrait::getCustomFieldsForItem()` decrypts
+ * every such row before the view sees it, so by the time it reaches this partial the plaintext is
+ * simply sitting in `$field->value`.
+ *
+ * The partial masked inside the `typeName === 'password'` branch alone, so every other type
+ * rendered that plaintext in full to anybody who could open the item, whatever
+ * `CUSTOMFIELD_VIEW_PASS` said about them. The API is the sibling that has always had it right:
+ * `CustomField::valueFor()` masks on `isValueEncrypted` and never looks at the type — so the same
+ * field was withheld from a REST caller and handed over by the web page.
+ *
+ * These render the real partial through a real `Template`, the way `ViewpassEscapesTest` does,
+ * because the masking is a property of the template file and of nothing else.
+ */
+#[Group('unitary')]
+class EncryptedCustomFieldsAreMaskedTest extends UnitaryTestCase
+{
+    private const SECRET = 'correct-horse-battery-staple';
+
+    /**
+     * Every type an encrypted value can be stored under is masked, not just `password`.
+     *
+     * `text`, `url`, `number`, `email` and `date` all fall through to the same generic `<input>`;
+     * one of them stands for that branch and `textarea` and `color` have branches of their own.
+     *
+     * @throws FileNotFoundException
+     */
+    #[Test]
+    #[TestWith(['textarea'])]
+    #[TestWith(['text'])]
+    #[TestWith(['url'])]
+    #[TestWith(['color'])]
+    #[TestWith(['password'])]
+    public function anEncryptedValueIsMaskedWhateverItsType(string $typeName): void
+    {
+        $html = $this->render($this->field($typeName, isEncrypted: true, isValueEncrypted: true));
+
+        self::assertStringNotContainsString(self::SECRET, $html);
+        self::assertStringContainsString('***', $html);
+    }
+
+    /**
+     * The permission is what the mask is for, so with it granted the value is shown — otherwise
+     * every assertion above would also be satisfied by a partial that never printed a value at all.
+     *
+     * @throws FileNotFoundException
+     */
+    #[Test]
+    #[TestWith(['textarea'])]
+    #[TestWith(['text'])]
+    #[TestWith(['url'])]
+    #[TestWith(['color'])]
+    #[TestWith(['password'])]
+    public function theValueIsShownToSomebodyWhoMayViewIt(string $typeName): void
+    {
+        $html = $this->render(
+            $this->field($typeName, isEncrypted: true, isValueEncrypted: true),
+            showViewCustomPass: true
+        );
+
+        self::assertStringContainsString(self::SECRET, $html);
+    }
+
+    /**
+     * A field that is not a secret is not masked either. Reading `isValueEncrypted` type-agnostically
+     * would be worth nothing if it also swallowed the ordinary text fields most items are made of.
+     *
+     * @throws FileNotFoundException
+     */
+    #[Test]
+    #[TestWith(['textarea'])]
+    #[TestWith(['text'])]
+    #[TestWith(['url'])]
+    public function anUnencryptedValueIsLeftAlone(string $typeName): void
+    {
+        $html = $this->render($this->field($typeName, isEncrypted: false, isValueEncrypted: false));
+
+        self::assertStringContainsString(self::SECRET, $html);
+    }
+
+    /**
+     * A `password` field is masked whether or not the row was encrypted. Somebody who chose that
+     * type meant the value to be hidden, and this is what the partial did before the rule was
+     * widened — so widening it must not narrow this.
+     *
+     * @throws FileNotFoundException
+     */
+    #[Test]
+    public function aPasswordTypedFieldIsMaskedEvenWhenItsValueIsNotEncrypted(): void
+    {
+        $html = $this->render($this->field('password', isEncrypted: false, isValueEncrypted: false));
+
+        self::assertStringNotContainsString(self::SECRET, $html);
+        self::assertStringContainsString('***', $html);
+    }
+
+    /**
+     * An empty field stays empty rather than being masked into a value that was never there —
+     * `***` in an edit form's input would be saved back as the literal secret on the next save.
+     *
+     * @throws FileNotFoundException
+     */
+    #[Test]
+    public function anEmptyEncryptedFieldIsNotMaskedIntoAValue(): void
+    {
+        $html = $this->render(
+            new CustomFieldItem(
+                required: false,
+                showInList: false,
+                help: '',
+                definitionId: 1,
+                definitionName: 'Recovery phrase',
+                typeId: 1,
+                typeName: 'textarea',
+                typeText: 'Text area',
+                moduleId: 10,
+                formId: 'recovery_phrase',
+                value: '',
+                isEncrypted: true,
+                isValueEncrypted: false
+            )
+        );
+
+        self::assertStringNotContainsString('***', $html);
+    }
+
+    private function field(string $typeName, bool $isEncrypted, bool $isValueEncrypted): CustomFieldItem
+    {
+        return new CustomFieldItem(
+            required: false,
+            showInList: false,
+            help: '',
+            definitionId: 1,
+            definitionName: 'Recovery phrase',
+            typeId: 1,
+            typeName: $typeName,
+            typeText: ucfirst($typeName),
+            moduleId: 10,
+            formId: 'recovery_phrase',
+            value: self::SECRET,
+            isEncrypted: $isEncrypted,
+            isValueEncrypted: $isValueEncrypted
+        );
+    }
+
+    /**
+     * @throws FileNotFoundException
+     */
+    private function render(CustomFieldItem $field, bool $showViewCustomPass = false): string
+    {
+        $resolver = self::createStub(TemplateResolverInterface::class);
+        $resolver
+            ->method('getTemplateFor')
+            ->willReturn(REAL_APP_ROOT . '/public/themes/material-blue/views/common/aux-customfields.inc');
+
+        $template = new Template(
+            new OutputHandler(),
+            $resolver,
+            self::createStub(ThemeIconsInterface::class),
+            self::createStub(UriContextInterface::class),
+            $this->config->getConfigData(),
+            'test'
+        );
+
+        $template->addTemplate('aux-customfields');
+
+        // `isView` is what puts a `color` field down its own branch rather than the generic input.
+        $template->assign('customFields', [$field]);
+        $template->assign('showViewCustomPass', $showViewCustomPass);
+        $template->assign('isView', true);
+        $template->assign('readonly', '');
+
+        return $template->render();
+    }
+}