From 1d8751ba31141f2441bca93bf32dd15c5107d47b Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 17 Aug 2026 21:09:03 +0200 Subject: [PATCH] Stop the API accepting a link expiry it discards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a public link over the API accepted `dateExpire` and `maxCountViews`, advertised both in its own help, and threw both away: requested: dateExpire=1787003922 maxCountViews=7 stored: dateExpire=1786992179 maxCountViews=3 <- configuration echoed back: 1787003922, 7 <- never stored `PublicLink::buildPublicLink()` sets both from configuration on the way to the database, so neither parameter ever did anything. What made that more than dead weight is the response, which echoed the request back: a caller asking for a link that expired at a particular time was told it would. For a feature that hands an account's password to whoever holds the URL, the expiry is the wrong thing to be wrong about. They are removed rather than honoured because `getPublinksMaxTime()` is the administrator's **maximum**, and a caller who could set the expiry could outlive it — implementing the documented behaviour would have turned a limit into a suggestion. Neither web path offers a way to set either; `SaveEditController` preserves both across an edit. The response now reports what was stored, by reading the created link back, which is what `Account\CreateController` and `AuthToken\CreateController` already do. This endpoint was the one echoing its input. The test asserts both halves. Only the second — that the answer describes the link that exists — fails without the fix; the first would pass against the old code too, since the values were discarded either way. --- .../Api/Controllers/Help/PublicLinkHelp.php | 6 +- .../PublicLink/CreateController.php | 19 +++++- .../Controllers/PublicLinkControllerTest.php | 64 +++++++++++++++++++ 3 files changed, 83 insertions(+), 6 deletions(-) diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php b/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php index 3881c11e3..7572ec6ea 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/Help/PublicLinkHelp.php @@ -53,9 +53,9 @@ public static function create(): array { return [ self::getItem('itemId', __('Account Id'), true), - self::getItem('notify', __('Notify when the link is used')), - self::getItem('dateExpire', __('Expiry date')), - self::getItem('maxCountViews', __('Maximum views')) + self::getItem('notify', __('Notify when the link is used')) + // No expiry or view limit: both come from the configuration, which is where the + // maximum lives, and a caller who could set them could outlive it. ]; } diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php b/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php index 8868fdbdf..8c1f236cf 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/PublicLink/CreateController.php @@ -24,12 +24,25 @@ public function createAction(): ApiResponse // call fail on the insert. 'typeId' => PublicLinkType::Account->value, 'notify' => (bool) $this->apiService->getParamInt('notify'), - 'dateExpire' => $this->apiService->getParamInt('dateExpire'), - 'maxCountViews' => $this->apiService->getParamInt('maxCountViews'), ]); $id = $this->publicLinkService->create($linkData); - $linkData = $linkData->mutate(['id' => $id]); + + // When the link expires and how often it may be opened are the administrator's + // configuration — `getPublinksMaxTime()` is a maximum, and neither web path lets anyone + // choose either. The service sets both, so the `dateExpire` and `maxCountViews` this + // endpoint used to accept were discarded on the way to the database. What made that + // costly rather than merely useless is that the response echoed the request back, so a + // caller was told a link would expire when they asked, and it did not. + $stored = $this->publicLinkService->getById($id); + + $linkData = $linkData->mutate( + [ + 'id' => $id, + 'dateExpire' => $stored->getDateExpire(), + 'maxCountViews' => $stored->getMaxCountViews(), + ] + ); $this->eventDispatcher->notify(new Event( 'create.publicLink', diff --git a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php index 1b9d46d97..7e97964d5 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/PublicLinkControllerTest.php @@ -26,11 +26,14 @@ namespace SP\Tests\Integration\Infrastructure\Adapter\In\Api\Controllers; +use PDO; use PHPUnit\Framework\Attributes\Group; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Tests\Integration\Infrastructure\Adapter\In\Api\ApiTestCase; use stdClass; +use function SP\Tests\getDbHandler; + /** * Covers the public-link endpoints over the REST API. None of them had tests. * @@ -59,6 +62,67 @@ public function testCreateAction(): void $this->assertGreaterThan(0, $r->body->itemId); } + /** + * A caller cannot choose when a link expires, or how many times it may be opened. + * + * `getPublinksMaxTime()` and `getPublinksMaxViews()` are the administrator's configuration — + * a maximum — and neither web path offers a way to set either. The endpoint nonetheless + * accepted `dateExpire` and `maxCountViews`, advertised them in its own help, and discarded + * them on the way to the database. + * + * What made that costly rather than merely useless is the response: it echoed the request + * back, so a caller asking for a link that expired at a particular time was told it would, + * while the stored link expired at whatever the configuration said. For a feature that hands + * out credentials, being wrong about the expiry in the safe direction is luck, not design. + */ + public function testTheExpiryAndViewLimitComeFromTheConfigurationNotTheCaller(): void + { + $wanted = time() + 12345; + + $r = $this->callApi(AclActionsInterface::PUBLICLINK_CREATE, [ + 'itemId' => $this->accountId(), + 'dateExpire' => $wanted, + 'maxCountViews' => 7, + ]); + + $this->assertSame(201, $r->status); + + [$storedExpiry, $storedViews] = $this->storedLimitsFor($r->body->itemId); + + $this->assertNotSame($wanted, $storedExpiry, 'the caller must not be able to set the expiry'); + $this->assertNotSame(7, $storedViews, 'nor the view limit'); + + // The half that actually misled anyone: the answer has to describe the link that exists. + $this->assertSame( + $storedExpiry, + $r->body->data->dateExpire, + 'the response reported an expiry the link does not have' + ); + $this->assertSame( + $storedViews, + $r->body->data->maxCountViews, + 'the response reported a view limit the link does not have' + ); + } + + /** + * The expiry and the view limit as the row holds them. + * + * @return array{int, int} + */ + private function storedLimitsFor(int $id): array + { + $statement = getDbHandler()->getConnection() + ->prepare('SELECT `dateExpire`, `maxCountViews` FROM `PublicLink` WHERE `id` = :id'); + $statement->execute(['id' => $id]); + + $row = $statement->fetch(PDO::FETCH_ASSOC); + + $this->assertNotFalse($row, sprintf('No PublicLink row with id %d', $id)); + + return [(int)$row['dateExpire'], (int)$row['maxCountViews']]; + } + /** * A link with no account behind it would hand out nothing, so the account is required. */