From fc474c1e16d5d79e0a9096694b1dc5794679612b Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 8 Sep 2026 00:11:30 +0200 Subject: [PATCH] fix: requesting access does not name an account you could not list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ?r=account/requestAccess/ rendered any account's name and client to any signed-in user. ACCOUNT_REQUEST sits in the unconditional arm of Acl::checkUserAccess() beside the notification actions; getByIdEnriched() is a bare WHERE id = :id, unfiltered on purpose because most callers pair it with an explicit ACL check; and AccountRequestHelper is the one helper in its directory that makes no such check, where AccountHelper and AccountHistoryHelper both do. Private accounts were reached too — buildFilterPrivate() withholds those from everybody, isAdminApp included. The obvious fix would break the feature: requesting a modification exists to ask about an account you can see listed and cannot open — isShowRequest() is literally !isShow(), and under global search that is an account you have no relationship with. So the bound is the search filter, which decides listability, not the ACL. An account the caller could not have listed is refused with the same 'doesn't exist' as one genuinely absent. The view is aliased to Account because that is what the filter qualifies its conditions with; account_data_v exposes every column they read, confirmed against the database. --- CLAUDE.md | 17 +++++ .../Account/Ports/AccountService.php | 12 ++++ src/Application/Account/Services/Account.php | 31 +++++++++ .../Account/Ports/AccountRepository.php | 11 ++++ .../Account/RequestAccessController.php | 2 +- .../Account/SaveRequestController.php | 2 +- .../Out/Account/Repositories/Account.php | 36 ++++++++++ .../Web/Controllers/Account/AccountTest.php | 65 +++++++++++++++++++ 8 files changed, 174 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index dfa9cdd4d..d71fffb0e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -536,6 +536,23 @@ and whether the thing being guarded varies with it.** Here it did not — which `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. +**An unfiltered read whose caller was the one without a check.** `AccountRepository::getByIdEnriched()` +applies no filter on purpose — most callers pair it with an explicit per-account ACL check, and the +"swept and clean" note above records that every path to a secret does. `account/requestAccess` did +neither: `ACCOUNT_REQUEST` sits in the unconditional arm of `Acl::checkUserAccess()` beside the +notification actions, and `AccountRequestHelper` is the one helper in its directory that does not +call `checkAccess()`, where `AccountHelper` and `AccountHistoryHelper` both do. Any signed-in user +could walk the ids and read back each account's name and client — private accounts included, which +the search filter withholds from everybody, administrators among them. + +The fix is the interesting part, and the obvious one is wrong: **the usual per-account check would +have broken the feature.** Requesting a modification exists precisely for an account you can see +listed and cannot open, which under global search (`isFilterWithoutGlobalSearch()`) is an account +you have no relationship with at all — `AccountSearchItem::isShowRequest()` is literally +`!isShow()`. The bound is therefore the *search filter*, which is what decides listability, not the +ACL. **Ask what a feature is for before deciding which check it was missing**; a guard copied from +a sibling can be the wrong guard. + **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 diff --git a/src/Application/Account/Ports/AccountService.php b/src/Application/Account/Ports/AccountService.php index dc6be6a9b..49976f765 100644 --- a/src/Application/Account/Ports/AccountService.php +++ b/src/Application/Account/Ports/AccountService.php @@ -138,6 +138,18 @@ public function create(AccountCreateDto $accountCreateDto): int; */ public function getByIdEnriched(int $id): AccountView; + /** + * The same account, but only when the signed-in user could have found it by searching. + * + * @param int $id + * + * @return AccountView + * @throws QueryException + * @throws NoSuchItemException + * @throws ConstraintException + */ + public function getByIdEnrichedForUser(int $id): AccountView; + /** * @param int $id The account ID * diff --git a/src/Application/Account/Services/Account.php b/src/Application/Account/Services/Account.php index f19b0768b..bb8b0ea86 100644 --- a/src/Application/Account/Services/Account.php +++ b/src/Application/Account/Services/Account.php @@ -191,6 +191,37 @@ public function getByIdEnriched(int $id): AccountView return $result->getData(AccountView::class); } + /** + * The same account, but only when the signed-in user could have found it by searching. + * + * `getByIdEnriched()` applies no filter, on purpose: most of its callers pair it with an + * explicit per-account ACL check. The "request modification" flow has neither, and cannot use + * the usual check either — asking about an account you can see listed and cannot open is what + * the feature is for, and under global search that is an account you have no relationship + * with. The search filter is what decides listability, so it is the right bound, and it + * withholds a private account from everybody including administrators. + * + * An account the user could not have listed is refused with the same "doesn't exist" as one + * that is really absent, so the two cannot be told apart by the answer. + * + * @param int $id + * + * @return AccountView + * @throws ConstraintException + * @throws NoSuchItemException + * @throws QueryException + */ + public function getByIdEnrichedForUser(int $id): AccountView + { + $result = $this->accountRepository->getByIdEnrichedForUser($id); + + if ($result->getNumRows() === 0) { + throw new NoSuchItemException(__u('The account doesn\'t exist')); + } + + return $result->getData(AccountView::class); + } + /** * Update accounts in bulk mode * diff --git a/src/Domain/Account/Ports/AccountRepository.php b/src/Domain/Account/Ports/AccountRepository.php index ea25d8b3c..be7a406d9 100644 --- a/src/Domain/Account/Ports/AccountRepository.php +++ b/src/Domain/Account/Ports/AccountRepository.php @@ -159,6 +159,17 @@ public function incrementViewCounter(int $accountId): QueryResult; */ public function getDataForLink(int $accountId): QueryResult; + /** + * The enriched account, but only when the signed-in user could have found it by searching. + * + * @param int $accountId + * + * @return QueryResult + * @throws ConstraintException + * @throws QueryException + */ + public function getByIdEnrichedForUser(int $accountId): QueryResult; + /** * @param int|null $accountId * diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php b/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php index 0f19e0fae..5c041d162 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/Account/RequestAccessController.php @@ -75,7 +75,7 @@ public function requestAccessAction(int $id): ActionResponse $this->accountRequestHelper->initializeFor(AclActionsInterface::ACCOUNT_REQUEST); $this->accountRequestHelper->setIsView(true); $this->accountRequestHelper->setViewForRequest( - new AccountEnrichedDto($this->accountService->getByIdEnriched($id)) + new AccountEnrichedDto($this->accountService->getByIdEnrichedForUser($id)) ); $this->view->addTemplate('account-request'); diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php index f9eb574aa..5921ec04c 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/Account/SaveRequestController.php @@ -74,7 +74,7 @@ public function saveRequestAction(int $id): ActionResponse throw new ValidationException(__u('A description is needed')); } - $accountView = $this->accountService->getByIdEnriched($id); + $accountView = $this->accountService->getByIdEnrichedForUser($id); $baseUrl = ($this->configData->getApplicationUrl() ?: $this->uriContext->getWebUri()) . $this->uriContext->getSubUri(); diff --git a/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php b/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php index ac4ce852e..b0352fd0c 100644 --- a/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php +++ b/src/Infrastructure/Adapter/Out/Account/Repositories/Account.php @@ -639,6 +639,42 @@ public function getDataForLink(int $accountId): QueryResult return $this->db->runQuery($queryData); } + /** + * The enriched account, but only when the signed-in user could have found it by searching. + * + * `getByIdEnriched()` above applies no filter, on purpose: most of its callers pair it with an + * explicit per-account ACL check. The "request modification" flow has neither, and cannot use + * the usual check either — asking about an account you can see listed and cannot open is what + * that feature is *for*, and under global search that is an account you have no relationship + * with. So the bound is the search filter itself, which is what decides listability, and which + * withholds a private account from everybody, administrators included. + * + * The view is aliased to `Account` because that is the name `AccountFilter` qualifies its + * conditions with, and `account_data_v` exposes every column they read — `id`, `userId`, + * `userGroupId`, `isPrivate`, `isPrivateGroup`. + * + * @param int $accountId + * + * @return QueryResult + * @throws ConstraintException + * @throws QueryException + */ + public function getByIdEnrichedForUser(int $accountId): QueryResult + { + $query = $this->queryFactory + ->newSelect() + ->from(sprintf('%s AS Account', AccountViewModel::TABLE)) + ->cols(AccountViewModel::getCols()) + ->where('Account.id = :id') + ->bindValues(['id' => $accountId]) + ->limit(1); + + $queryData = QueryData::buildWithMapper($this->accountFilterUser->buildFilter(false, $query), AccountViewModel::class) + ->setOnErrorMessage(__u('Error while retrieving account\'s data')); + + return $this->db->runQuery($queryData); + } + /** * @param int|null $accountId * diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php index 01a998f8c..6d432ce61 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/AccountTest.php @@ -49,6 +49,7 @@ use SP\Domain\User\Models\ProfileData; use SP\Domain\User\Models\User as UserModel; use SP\Domain\Common\Dtos\QueryResult; +use SP\Infrastructure\Database\QueryData; use SP\Tests\Support\BodyChecker; use SP\Tests\Support\Generators\AccountDataGenerator; use SP\Tests\Support\Generators\PublicLinkDataGenerator; @@ -66,6 +67,8 @@ #[InjectVault] class AccountTest extends IntegrationTestCase { + private const UNLISTABLE_ACCOUNT_NAME = 'an-account-this-user-cannot-list'; + private const OWNER_NAME = 'Fixture Person'; private const GROUP_NAME = 'Fixture Team'; @@ -882,6 +885,60 @@ public function requestAccess() IntegrationTestCase::runApp($container); } + /** + * Requesting a modification does not name an account the caller could not have found. + * + * `ACCOUNT_REQUEST` answers `true` for every signed-in user — it is in the same unconditional + * arm as the notification actions — and the controller read the account with + * `getByIdEnriched()`, whose query is a bare `WHERE id = :id`. `AccountRequestHelper` is the + * one helper in its directory that does not call `checkAccess()`, where `AccountHelper` and + * `AccountHistoryHelper` both do. So any authenticated user could walk the ids and read back + * each account's name and client — including accounts marked private, which the search filter + * withholds from everybody, administrators included. + * + * The fix cannot be the usual per-account ACL check: this feature exists to ask about an + * account you can see listed and cannot open, which under global search is an account you have + * no relationship with. The bound is the search filter, which is what decides listability. + * + * The two queries are told apart by their statement rather than by their mapper, because both + * map to `AccountView` — only the filtered one joins `AccountToUser`, and on the old code no + * statement did, so the account was found and rendered. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerRequestAccessRefused')] + public function requestAccessDoesNotNameAnAccountTheUserCannotList() + { + $account = AccountDataGenerator::factory()->buildAccountDataView()->mutate( + ['name' => self::UNLISTABLE_ACCOUNT_NAME] + ); + + // Not a static closure: the harness binds it with Closure::call(). Everything other than + // the account read has to keep answering the way the harness's default does, or the + // request fails before it renders and the assertion below passes for the wrong reason. + $this->databaseQueryResolver = function (QueryData $queryData) use ($account): QueryResult { + if ($queryData->getMapClassName() !== AccountView::class) { + return new QueryResult([], 1, 100); + } + + if (str_contains($queryData->getQuery()->getStatement(), 'AccountToUser')) { + // The filtered read: this user could not have listed it. + return new QueryResult([]); + } + + return new QueryResult([$account]); + }; + + $container = $this->buildContainer( + IntegrationTestCase::buildRequest('get', 'index.php', ['r' => 'account/requestAccess/100']) + ); + + IntegrationTestCase::runApp($container); + } + /** * @throws ContainerExceptionInterface * @throws Exception @@ -1121,6 +1178,14 @@ private function outputCheckerSearch(string $output): void self::assertEquals('OK', $json->status); } + /** + * The account's name is nowhere in what was sent back. + */ + private function outputCheckerRequestAccessRefused(string $output): void + { + self::assertStringNotContainsString(self::UNLISTABLE_ACCOUNT_NAME, $output); + } + /** * @param string $output * @return void