From f40efe2f5b81e9ed034e030d10e1c38198174ae9 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 20 Aug 2026 20:03:06 +0200 Subject: [PATCH] fix: mask a custom field's secret for an API caller who may not see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `account/view?customFields=1` answered with the decrypted value of an encrypted custom field, whoever asked. `ItemTrait::getCustomFieldsForItem()` decrypts whenever the row carries a key, without asking who is looking — the deciding is left to whoever renders it. The theme does decide: `aux-customfields.inc` prints `***` unless `showViewCustomPass`, which `AccountHelper` sets from the account's own view-password permission, and `CUSTOMFIELD_VIEW_PASS` maps to the same `isAccViewPass` profile flag. That flag is consulted in five web controllers and nowhere else. Nothing decided on the API path. What makes it more than an omission is that the API is deliberate about the account's own password: the account transform returns login, url, notes and metadata but not `pass`, because retrieving a password is `account/viewPass` — a separate action, with a separate token. A token for `account/view` was handing out what that separation exists to gate. `Api::setupUser()` puts the profile in context, so the check was possible all along. The ACL is threaded through the Adapter base, which the three adapters that include custom fields already autowire, and the masking matches the theme's `***` rather than dropping the key — the response keeps its shape and still reports `encrypted: true`, so a caller can tell a masked secret from an empty field. A value that was never encrypted is returned as it is; masking every custom field would have been a different change, and the third test pins that. Six existing adapter tests construct these positionally and broke on the new parameter, which is what a full-suite run is for. --- src/Domain/Account/Adapters/Account.php | 6 +- src/Domain/Category/Adapters/Category.php | 6 +- src/Domain/Client/Adapters/Client.php | 6 +- src/Domain/Common/Adapters/Adapter.php | 4 +- .../CustomField/Adapters/CustomField.php | 30 +++++++- .../Domain/Account/Adapters/AccountTest.php | 4 +- .../Domain/Category/Adapters/CategoryTest.php | 3 + .../Domain/Client/Adapters/ClientTest.php | 3 + .../Adapters/CustomFieldAdapterTest.php | 73 ++++++++++++++++++- 9 files changed, 124 insertions(+), 11 deletions(-) diff --git a/src/Domain/Account/Adapters/Account.php b/src/Domain/Account/Adapters/Account.php index 16875cced..e77927d2a 100644 --- a/src/Domain/Account/Adapters/Account.php +++ b/src/Domain/Account/Adapters/Account.php @@ -33,6 +33,7 @@ use SP\Domain\Common\Providers\Link; use SP\Domain\Common\Services\ServiceException; use SP\Domain\Config\Ports\ConfigDataInterface; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Acl\ActionNotFoundException; use SP\Domain\Core\Acl\ActionsInterface; @@ -61,10 +62,11 @@ final class Account extends Adapter implements AccountAdapter public function __construct( ConfigDataInterface $configData, string $baseUrl, + AclInterface $acl, private readonly CustomFieldDataService $customFieldService, private readonly ActionsInterface $actions ) { - parent::__construct($configData, $baseUrl); + parent::__construct($configData, $baseUrl, $acl); } /** @@ -81,7 +83,7 @@ public function includeCustomFields(AccountEnrichedDto $accountEnrichedDto): Col $accountEnrichedDto->getId(), $this->customFieldService ), - new CustomField($this->configData, $this->baseUrl) + new CustomField($this->configData, $this->baseUrl, $this->acl) ); } diff --git a/src/Domain/Category/Adapters/Category.php b/src/Domain/Category/Adapters/Category.php index 8cb51ca10..d1846ac6f 100644 --- a/src/Domain/Category/Adapters/Category.php +++ b/src/Domain/Category/Adapters/Category.php @@ -33,6 +33,7 @@ use SP\Domain\Common\Providers\Link; use SP\Domain\Common\Services\ServiceException; use SP\Domain\Config\Ports\ConfigDataInterface; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Acl\ActionNotFoundException; use SP\Domain\Core\Acl\ActionsInterface; @@ -60,10 +61,11 @@ final class Category extends Adapter implements CategoryAdapter public function __construct( ConfigDataInterface $configData, string $baseUrl, + AclInterface $acl, private readonly CustomFieldDataService $customFieldDataService, private readonly ActionsInterface $actions ) { - parent::__construct($configData, $baseUrl); + parent::__construct($configData, $baseUrl, $acl); } /** @@ -76,7 +78,7 @@ public function includeCustomFields(CategoryModel $data): Collection { return $this->collection( $this->getCustomFieldsForItem(AclActionsInterface::CATEGORY, $data->getId(), $this->customFieldDataService), - new CustomField($this->configData, $this->baseUrl) + new CustomField($this->configData, $this->baseUrl, $this->acl) ); } diff --git a/src/Domain/Client/Adapters/Client.php b/src/Domain/Client/Adapters/Client.php index 6945dc3ce..fba80125b 100644 --- a/src/Domain/Client/Adapters/Client.php +++ b/src/Domain/Client/Adapters/Client.php @@ -33,6 +33,7 @@ use SP\Domain\Common\Providers\Link; use SP\Domain\Common\Services\ServiceException; use SP\Domain\Config\Ports\ConfigDataInterface; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Acl\ActionNotFoundException; use SP\Domain\Core\Acl\ActionsInterface; @@ -60,10 +61,11 @@ final class Client extends Adapter implements ClientAdapter public function __construct( ConfigDataInterface $configData, string $baseUrl, + AclInterface $acl, private readonly CustomFieldDataService $customFieldDataService, private readonly ActionsInterface $actions ) { - parent::__construct($configData, $baseUrl); + parent::__construct($configData, $baseUrl, $acl); } /** @@ -76,7 +78,7 @@ public function includeCustomFields(ClientModel $client): Collection { return $this->collection( $this->getCustomFieldsForItem(AclActionsInterface::CLIENT, $client->getId(), $this->customFieldDataService), - new CustomField($this->configData, $this->baseUrl) + new CustomField($this->configData, $this->baseUrl, $this->acl) ); } diff --git a/src/Domain/Common/Adapters/Adapter.php b/src/Domain/Common/Adapters/Adapter.php index 6d16f2c1e..12c3dfd3c 100644 --- a/src/Domain/Common/Adapters/Adapter.php +++ b/src/Domain/Common/Adapters/Adapter.php @@ -27,6 +27,7 @@ use League\Fractal\TransformerAbstract; use SP\Domain\Config\Ports\ConfigDataInterface; +use SP\Domain\Core\Acl\AclInterface; /** * Class Adapter @@ -35,7 +36,8 @@ abstract class Adapter extends TransformerAbstract { public function __construct( protected readonly ConfigDataInterface $configData, - protected readonly string $baseUrl + protected readonly string $baseUrl, + protected readonly AclInterface $acl ) { } diff --git a/src/Domain/CustomField/Adapters/CustomField.php b/src/Domain/CustomField/Adapters/CustomField.php index 43f642029..15afdb51f 100644 --- a/src/Domain/CustomField/Adapters/CustomField.php +++ b/src/Domain/CustomField/Adapters/CustomField.php @@ -26,6 +26,7 @@ namespace SP\Domain\CustomField\Adapters; use SP\Domain\Common\Adapters\Adapter; +use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Common\Dtos\Dto; use SP\Domain\CustomField\Ports\CustomFieldAdapter; use SP\Domain\CustomField\Services\CustomFieldItem; @@ -35,6 +36,9 @@ */ final class CustomField extends Adapter implements CustomFieldAdapter { + /** What the theme prints in place of a value the viewer may not see. */ + public const MASKED = '***'; + /** * @param CustomFieldItem $data * @return array @@ -47,9 +51,33 @@ public function transform($data): array 'definitionId' => $data->definitionId, 'definitionName' => $data->definitionName, 'help' => $data->help, - 'value' => $data->value, + 'value' => $this->valueFor($data), 'encrypted' => $data->isEncrypted, 'required' => $data->required, ]; } + + /** + * The value, or what the interface would show in its place. + * + * `ItemTrait::getCustomFieldsForItem()` decrypts a stored value whenever the row carries a + * key, without asking who is looking — the deciding is left to whoever renders it. The theme + * does decide: `aux-customfields.inc` prints `***` unless `showViewCustomPass`, which + * `AccountHelper` sets from the account's own view-password permission. Nothing decided here, + * so `account/view?customFields=1` answered with the decrypted value — on a token for + * `account/view`, an action the API otherwise keeps apart from `account/viewPass` precisely + * because one of them hands out secrets. + * + * A field that was never encrypted is not a secret and is returned as it is. + */ + private function valueFor(CustomFieldItem $data): ?string + { + if (!$data->isValueEncrypted) { + return $data->value; + } + + return $this->acl->checkUserAccess(AclActionsInterface::CUSTOMFIELD_VIEW_PASS) + ? $data->value + : self::MASKED; + } } diff --git a/tests/Unit/Domain/Account/Adapters/AccountTest.php b/tests/Unit/Domain/Account/Adapters/AccountTest.php index 805306b5a..241cc8dd5 100644 --- a/tests/Unit/Domain/Account/Adapters/AccountTest.php +++ b/tests/Unit/Domain/Account/Adapters/AccountTest.php @@ -30,6 +30,7 @@ use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\Exception; use SP\Domain\Account\Adapters\Account; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Acl\ActionNotFoundException; use SP\Domain\Core\Acl\ActionsInterface; @@ -70,6 +71,7 @@ public function testAdapt(): void $adapter = new Account( $this->config->getConfigData(), 'testUrl', + $this->createStub(AclInterface::class), $this->createStub(CustomFieldDataService::class), $actions ); @@ -152,7 +154,7 @@ public function testIncludeCustomFields(): void ); - $adapter = new Account($this->config->getConfigData(), 'testUrl', $customFieldsService, $actions); + $adapter = new Account($this->config->getConfigData(), 'testUrl', $this->createStub(AclInterface::class), $customFieldsService, $actions); $fractal = new Manager(); $fractal->parseIncludes('customFields'); diff --git a/tests/Unit/Domain/Category/Adapters/CategoryTest.php b/tests/Unit/Domain/Category/Adapters/CategoryTest.php index 1b06e34d3..3c6b8ae8b 100644 --- a/tests/Unit/Domain/Category/Adapters/CategoryTest.php +++ b/tests/Unit/Domain/Category/Adapters/CategoryTest.php @@ -32,6 +32,7 @@ use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\MockObject\MockObject; use SP\Domain\Category\Adapters\Category; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Acl\ActionNotFoundException; use SP\Domain\Core\Acl\ActionsInterface; @@ -61,6 +62,7 @@ public function testAdapt(): void $adapter = new Category( $this->config->getConfigData(), 'testUrl', + $this->createStub(AclInterface::class), $this->customFieldDataService, $this->actions ); @@ -93,6 +95,7 @@ public function testIncludeCustomFields(): void $adapter = new Category( $this->config->getConfigData(), 'testUrl', + $this->createStub(AclInterface::class), $this->customFieldDataService, $this->actions ); diff --git a/tests/Unit/Domain/Client/Adapters/ClientTest.php b/tests/Unit/Domain/Client/Adapters/ClientTest.php index 68e4c86cb..a6a94d6d4 100644 --- a/tests/Unit/Domain/Client/Adapters/ClientTest.php +++ b/tests/Unit/Domain/Client/Adapters/ClientTest.php @@ -32,6 +32,7 @@ use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\MockObject\MockObject; use SP\Domain\Client\Adapters\Client; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Acl\ActionNotFoundException; use SP\Domain\Core\Acl\ActionsInterface; @@ -61,6 +62,7 @@ public function testAdapt(): void $adapter = new Client( $this->config->getConfigData(), 'testUrl', + $this->createStub(AclInterface::class), $this->customFieldDataService, $this->actions ); @@ -94,6 +96,7 @@ public function testIncludeCustomFields(): void $adapter = new Client( $this->config->getConfigData(), 'testUrl', + $this->createStub(AclInterface::class), $this->customFieldDataService, $this->actions ); diff --git a/tests/Unit/Domain/CustomField/Adapters/CustomFieldAdapterTest.php b/tests/Unit/Domain/CustomField/Adapters/CustomFieldAdapterTest.php index 44dfb7334..7bef6c4f7 100644 --- a/tests/Unit/Domain/CustomField/Adapters/CustomFieldAdapterTest.php +++ b/tests/Unit/Domain/CustomField/Adapters/CustomFieldAdapterTest.php @@ -8,16 +8,85 @@ use PHPUnit\Framework\TestCase; use SP\Domain\Config\Ports\ConfigDataInterface; use SP\Domain\CustomField\Adapters\CustomField; +use SP\Domain\Core\Acl\AclActionsInterface; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\CustomField\Services\CustomFieldItem; #[Group('unitary')] class CustomFieldAdapterTest extends TestCase { - private function makeAdapter(): CustomField + private function makeAdapter(bool $mayViewPass = true): CustomField { $configData = $this->createStub(ConfigDataInterface::class); - return new CustomField($configData, 'https://example.com'); + $acl = $this->createStub(AclInterface::class); + $acl->method('checkUserAccess')->willReturnCallback( + static fn(int $action) => $action === AclActionsInterface::CUSTOMFIELD_VIEW_PASS ? $mayViewPass : true + ); + + return new CustomField($configData, 'https://example.com', $acl); + } + + /** + * @param array $overrides + */ + private function makeItem(array $overrides = []): CustomFieldItem + { + return new CustomFieldItem( + required: $overrides['required'] ?? false, + showInList: false, + help: '', + definitionId: 5, + definitionName: 'API Key', + typeId: 2, + typeName: 'password', + typeText: 'Password', + moduleId: 10, + formId: 'cf_5', + value: $overrides['value'] ?? 'secret', + isEncrypted: $overrides['isEncrypted'] ?? true, + isValueEncrypted: $overrides['isValueEncrypted'] ?? true, + ); + } + + /** + * A stored secret is not handed to a caller who may not see it. + * + * `ItemTrait::getCustomFieldsForItem()` decrypts whenever the row carries a key, without asking + * who is looking — the deciding is left to whoever renders it. The theme does decide: + * `aux-customfields.inc` prints `***` unless `showViewCustomPass`, which `AccountHelper` sets + * from the account's own view-password permission. Nothing decided here, so + * `account/view?customFields=1` answered with the decrypted value — on a token for + * `account/view`, which the API otherwise keeps apart from `account/viewPass` precisely because + * one of them hands out secrets. The account's own password is not in this response for the + * same reason. + */ + public function testAnEncryptedValueIsMaskedForACallerWhoMayNotViewPasswords(): void + { + $result = $this->makeAdapter(mayViewPass: false)->transform($this->makeItem()); + + self::assertSame(CustomField::MASKED, $result['value']); + self::assertTrue($result['encrypted'], 'the caller is still told the field holds a secret'); + } + + public function testAnEncryptedValueIsReturnedToACallerWhoMayViewPasswords(): void + { + $result = $this->makeAdapter(mayViewPass: true)->transform($this->makeItem()); + + self::assertSame('secret', $result['value']); + } + + /** + * A field that was never encrypted is not a secret, and the permission does not apply to it — + * masking every custom field would have been a different change, and a wrong one. + */ + public function testAValueThatWasNeverEncryptedIsReturnedRegardless(): void + { + $result = $this->makeAdapter(mayViewPass: false)->transform( + $this->makeItem(['isValueEncrypted' => false, 'isEncrypted' => false, 'value' => 'plain']) + ); + + self::assertSame('plain', $result['value']); } public function testTransformReturnsExpectedKeys(): void