From c10e1c5bc3f58b217c88be2bb9662eaa594f435d Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 18 Aug 2026 08:58:26 +0200 Subject: [PATCH] Make spending a public link's view the thing that guards it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A public link issued for a single view could be followed twice. The counter increment was already atomic — `countViews + 1` in SQL, which is what stops a lost update — but the limit was checked in PHP, against a row that had already been read: && $publicLink->getCountViews() < $publicLink->getMaxCountViews() // read $this->publicLinkService->addLinkView($publicLink); // then write Two requests arriving together both read `countViews = max - 1`, both pass, and both are served. An atomic increment keeps the counter right; it never stopped the limit being exceeded. `addLinkView()` now carries both conditions — `COALESCE(countViews, 0) < maxCountViews AND dateExpire > :now` — and reports whether a row was affected, so the moment the link is charged for and the moment it is handed over are the same moment. COALESCE because `countViews` is nullable, and a NULL comparison would refuse every link nobody had followed yet. The password-reset flow already does exactly this: `toggleUsedByHash()` is a conditional update that throws when it affects nothing, before the password is changed. The pattern was in the codebase; this path predated it. The race is pinned without concurrency. A second simultaneous request is precisely a process holding a stale copy of the row, so the test reads the link, exhausts it through the normal path, and presents that copy — which under the old code still said "views remaining" and spent the link past its limit. Two refusal tests in ViewLinkRefusalsTest broke, correctly: they run on the mocked-database harness, which answers every update with one row affected, so with the check in SQL the double stopped refusing anything. It now answers the way a server would. The link read moved into the same closure, because `databaseQueryResolver` is consulted first and short-circuits the mapper ones. The `useInfo` list is still assembled from the row as read, so two views landing together record one entry between them. The counter is exact — that is what decides access — while the list is a log. Recorded at the call site. --- .../Account/Ports/PublicLinkService.php | 8 +- .../Account/Services/PublicLink.php | 13 ++- .../Account/ViewLinkController.php | 13 +-- .../Out/Account/Repositories/PublicLink.php | 12 ++- .../Account/PublicLinkRoundTripTest.php | 89 ++++++++++++++++++- .../Account/ViewLinkRefusalsTest.php | 27 +++++- 6 files changed, 147 insertions(+), 15 deletions(-) diff --git a/src/Application/Account/Ports/PublicLinkService.php b/src/Application/Account/Ports/PublicLinkService.php index 7be8c44c2..ccada6194 100644 --- a/src/Application/Account/Ports/PublicLinkService.php +++ b/src/Application/Account/Ports/PublicLinkService.php @@ -114,13 +114,17 @@ public function create(PublicLink $publicLink): int; public function getAll(): array; /** - * Increments the visit counter of a link + * Records a view of a link, if it still has one to give. + * + * The view limit and the expiry are conditions of the update itself, so this is the moment the + * link is spent — and the answer is whether it was. False means the link was exhausted or had + * expired, and the caller must not hand out the account. * * @throws NoSuchItemException * @throws ConstraintException * @throws QueryException */ - public function addLinkView(PublicLink $publicLink): void; + public function addLinkView(PublicLink $publicLink): bool; /** * @throws SPException diff --git a/src/Application/Account/Services/PublicLink.php b/src/Application/Account/Services/PublicLink.php index a47e70372..2ffdf68be 100644 --- a/src/Application/Account/Services/PublicLink.php +++ b/src/Application/Account/Services/PublicLink.php @@ -279,7 +279,7 @@ public function getAll(): array * @throws QueryException * @throws ServiceException */ - public function addLinkView(PublicLinkModel $publicLink): void + public function addLinkView(PublicLinkModel $publicLink): bool { $useInfo = []; @@ -297,7 +297,16 @@ public function addLinkView(PublicLinkModel $publicLink): void $useInfo[] = self::getUseInfo($publicLink->getHash(), $this->request); - $this->publicLinkRepository->addLinkView($publicLink->mutate(['useInfo' => Serde::serialize($useInfo)])); + // False when the link is exhausted or expired: the repository makes both conditions of the + // update, so this answers whether the view was actually recorded, and the caller must not + // hand out the account unless it was. + // + // The usage list itself is still assembled from the row as it was read, so two views + // landing together record one entry between them. The counter is exact — that is the one + // that decides access — while the list is a log. + return $this->publicLinkRepository->addLinkView( + $publicLink->mutate(['useInfo' => Serde::serialize($useInfo)]) + ); } /** diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkController.php b/src/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkController.php index 249ca734f..c2e853fee 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkController.php @@ -101,12 +101,13 @@ public function viewLinkAction(string $hash): ActionResponse logger(sprintf('Public link not found: %s', $hash)); } - if ($publicLink !== null - && time() < $publicLink->getDateExpire() - && $publicLink->getCountViews() < $publicLink->getMaxCountViews() - ) { - $this->publicLinkService->addLinkView($publicLink); - + // Spending the view is what decides whether to serve. The expiry and the view limit used + // to be tested here, against a row that had already been read, and the counter was then + // incremented — so two requests arriving together on a link with one view left both got + // past the test and both were served. A link issued to be followed once handed the + // account out twice. Both conditions now live in that update, and it reports whether it + // applied. + if ($publicLink !== null && $this->publicLinkService->addLinkView($publicLink)) { $this->accountService->incrementViewCounter($publicLink->getItemId()); $this->accountService->incrementDecryptCounter($publicLink->getItemId()); diff --git a/src/Infrastructure/Adapter/Out/Account/Repositories/PublicLink.php b/src/Infrastructure/Adapter/Out/Account/Repositories/PublicLink.php index cfa47e52d..cf8ffdc1d 100644 --- a/src/Infrastructure/Adapter/Out/Account/Repositories/PublicLink.php +++ b/src/Infrastructure/Adapter/Out/Account/Repositories/PublicLink.php @@ -258,6 +258,14 @@ private function checkDuplicatedOnAdd(int $id): bool */ public function addLinkView(PublicLinkModel $publicLinkData): bool { + // The limit and the expiry are conditions of the update, not something checked beforehand. + // They used to be tested in PHP against a row that had already been read, so two requests + // arriving together on a link with one view left both passed the check and both were + // served — a link issued for a single view handed the account out twice. Here the server + // decides, and a link that is exhausted or expired matches no row. + // + // COALESCE because `countViews` is nullable: a NULL would make the comparison NULL and + // refuse a link that has simply never been followed. $query = $this->queryFactory ->newUpdate() ->table('PublicLink') @@ -265,7 +273,9 @@ public function addLinkView(PublicLinkModel $publicLinkData): bool ->set('totalCountViews', '(totalCountViews + 1)') ->col('useInfo', $publicLinkData->getUseInfo()) ->where('hash = :hash') - ->bindValues(['hash' => $publicLinkData->getHash()]); + ->where('COALESCE(countViews, 0) < maxCountViews') + ->where('dateExpire > :now') + ->bindValues(['hash' => $publicLinkData->getHash(), 'now' => time()]); $queryData = QueryData::build($query)->setOnErrorMessage(__u('Error while updating the link')); diff --git a/tests/Integration/Application/Account/PublicLinkRoundTripTest.php b/tests/Integration/Application/Account/PublicLinkRoundTripTest.php index b3c0eaffa..0db7b076d 100644 --- a/tests/Integration/Application/Account/PublicLinkRoundTripTest.php +++ b/tests/Integration/Application/Account/PublicLinkRoundTripTest.php @@ -230,6 +230,88 @@ public function testALinkStopsYieldingTheAccountOnceItsViewLimitIsReached(): voi ); } + /** + * A copy of the link read before it was spent cannot be used to spend it again. + * + * This is the concurrent case, made deterministic. Two requests arriving together each read + * the row before either has written to it, so each holds a copy saying the link still has + * views left — and the check used to be made against that copy: + * + * ```php + * && $publicLink->getCountViews() < $publicLink->getMaxCountViews() + * ``` + * + * Both passed, both were served, and a link issued for a single view handed the account out + * twice. Holding a stale copy is exactly what the second request has, so exhausting the link + * and then presenting the copy reproduces it without needing two processes. + * + * The limit and the expiry are conditions of the update now, so the server is what refuses, + * and a copy of any age cannot talk it round. + */ + public function testAStaleCopyOfALinkCannotSpendItPastItsLimit(): void + { + $publicLinkService = $this->dic->get(PublicLinkServiceInterface::class); + + $suffix = bin2hex(random_bytes(4)); + $accountId = $this->createAccount('race-' . $suffix, 'RacePass!' . bin2hex(random_bytes(6))); + + $hash = $this->createLinkFor($accountId); + + // The copy the second request would be holding: read before anything has been spent. + $staleCopy = $publicLinkService->getByHash($hash); + + $maxCountViews = $staleCopy->getMaxCountViews(); + self::assertGreaterThan(0, $maxCountViews, 'setup: the configured view limit must be positive'); + + for ($view = 1; $view <= $maxCountViews; $view++) { + self::assertNotNull($this->followLink($hash), sprintf('setup: view %d should be allowed', $view)); + } + + self::assertFalse( + $publicLinkService->addLinkView($staleCopy), + 'a copy read before the link was exhausted must not be able to spend it again' + ); + + self::assertSame( + $maxCountViews, + $publicLinkService->getByHash($hash)->getCountViews(), + 'the refused attempt must not have moved the counter past the limit' + ); + } + + /** + * The same for an expiry that passed while the request was in flight. + * + * The expiry was the other half of the same check, read from the same stale copy, so it is + * refused by the same update rather than by a comparison made before it. + */ + public function testALinkThatExpiresBeforeTheViewIsRecordedIsRefused(): void + { + $publicLinkService = $this->dic->get(PublicLinkServiceInterface::class); + + $suffix = bin2hex(random_bytes(4)); + $accountId = $this->createAccount('exp-' . $suffix, 'ExpPass!' . bin2hex(random_bytes(6))); + + $hash = $this->createLinkFor($accountId); + $publicLink = $publicLinkService->getByHash($hash); + + // Expire it behind the copy the request is holding, which still says it is good. + $statement = getDbHandler()->getConnection() + ->prepare('UPDATE `PublicLink` SET `dateExpire` = :expired WHERE `hash` = :hash'); + $statement->execute(['expired' => time() - 1, 'hash' => $hash]); + + self::assertGreaterThan( + time(), + $publicLink->getDateExpire(), + 'setup: the copy in hand must still believe the link is live' + ); + + self::assertFalse( + $publicLinkService->addLinkView($publicLink), + 'a link that expired before the view was recorded must not be spent' + ); + } + /** * A link whose expiry has already passed does not yield the account either, even though it is * nowhere near its view limit. The expiry is moved into the past through @@ -306,12 +388,13 @@ private function followLink(string $hash): ?string $publicLink = $publicLinkService->getByHash($hash); - if (time() >= $publicLink->getDateExpire() || $publicLink->getCountViews() >= $publicLink->getMaxCountViews()) { + // Spending the view is the guard, exactly as the controller now has it: the expiry and the + // limit are conditions of the update, so this answers whether there was a view left to + // take. It used to be a pair of comparisons here, against the row just read. + if (!$publicLinkService->addLinkView($publicLink)) { return null; } - $publicLinkService->addLinkView($publicLink); - $accountService->incrementViewCounter($publicLink->getItemId()); $accountService->incrementDecryptCounter($publicLink->getItemId()); diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkRefusalsTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkRefusalsTest.php index ce5606d3a..7e55b4d38 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkRefusalsTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/Account/ViewLinkRefusalsTest.php @@ -42,6 +42,7 @@ use SP\Domain\Core\Exceptions\CryptException; use SP\Domain\Crypt\Vault; use SP\Infrastructure\Crypt\Crypt; +use SP\Infrastructure\Database\QueryData; use SP\Infrastructure\Events\EventDispatcher; use SP\Tests\Support\BodyChecker; use SP\Tests\Support\Generators\AccountDataGenerator; @@ -258,7 +259,31 @@ private function givenALink(array $properties): void ) ); - $this->addDatabaseMapperResolver(PublicLink::class, new QueryResult([$publicLink])); + // Whether the link still has a view to give is decided by the update that spends it: its + // WHERE clause carries the limit and the expiry, so an exhausted or expired link matches + // no row and the update reports nothing affected. The double has to answer the way the + // server would, or a test for a refusal only shows that the fixture said "expired" + // somewhere — the guard it is aiming at moved out of PHP precisely so that two + // simultaneous requests cannot both pass it. + $spent = (int)($publicLink->getCountViews() ?? 0) >= (int)($publicLink->getMaxCountViews() ?? 0) + || time() >= (int)$publicLink->getDateExpire(); + + // One resolver rather than a mapper resolver beside it: `databaseQueryResolver` is + // consulted first and short-circuits the mapper ones, so the read has to be answered here + // too. Not a static closure — the harness binds it with Closure::call(). + $this->databaseQueryResolver = function (QueryData $queryData) use ($spent, $publicLink): QueryResult { + $statement = $queryData->getQuery()->getStatement(); + + if (str_contains($statement, 'UPDATE') && str_contains($statement, 'PublicLink')) { + return new QueryResult([], $spent ? 0 : 1); + } + + if ($queryData->getMapClassName() === PublicLink::class) { + return new QueryResult([$publicLink]); + } + + return new QueryResult([], 1, 100); + }; } /**