From 9aa36008aab3b5257f0f2134e4db252023cc5b76 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 19 Aug 2026 01:22:40 +0200 Subject: [PATCH] fix: treat a negative page size as an empty page, not a syntax error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit How far into a list to start, and how much of it to take, both come from the query string, and nothing narrowed them on the way to the database: ?count=-1 -> Filter::getInt('-1') = -1 -> no clamp -> LIMIT -1 ?start=-5 -> OFFSET -5 which MariaDB answers with `ERROR 1064 ... syntax error near '-1'`. Asking for a page came back as a database failure. Nothing was disclosed and nothing was harmed, but a value an ordinary request can carry should not reach the server as unparseable SQL. There are two ways in, so there are two places to clamp. The item grids and every API search build an `ItemSearchDto`; the account search reads `start` and `rpp` straight from the request into `AccountSearchFilterDto`. Clamping only the first would have left the search people actually use still broken. The third test asserts the generated statement carries no negative LIMIT or OFFSET, because the getters were never the problem — the string that reached MariaDB was. The upper end is deliberately left alone: `count` has no maximum, but a large one is authenticated, returns only rows the caller may already see, and capping it would change behaviour for legitimate large queries. --- .../Account/Dtos/AccountSearchFilterDto.php | 8 +- src/Domain/Core/Dtos/ItemSearchDto.php | 15 ++- .../Core/Dtos/PaginationIsNotNegativeTest.php | 118 ++++++++++++++++++ 3 files changed, 137 insertions(+), 4 deletions(-) create mode 100644 tests/Unit/Domain/Core/Dtos/PaginationIsNotNegativeTest.php diff --git a/src/Domain/Account/Dtos/AccountSearchFilterDto.php b/src/Domain/Account/Dtos/AccountSearchFilterDto.php index 6ab96ba3f..61c38d997 100644 --- a/src/Domain/Account/Dtos/AccountSearchFilterDto.php +++ b/src/Domain/Account/Dtos/AccountSearchFilterDto.php @@ -143,7 +143,11 @@ public function getLimitStart(): int public function setLimitStart(int $limitStart): AccountSearchFilterDto { - $this->limitStart = $limitStart; + // `start` and `rpp` come straight off the query string here, without passing through + // ItemSearchDto — and a negative one reached the server as `LIMIT -1 OFFSET -5`, which + // MariaDB answers with `ERROR 1064 ... syntax error`. A page nobody has is an empty page, + // not a database failure. + $this->limitStart = max(0, $limitStart); return $this; } @@ -155,7 +159,7 @@ public function getLimitCount(): ?int public function setLimitCount(?int $limitCount): AccountSearchFilterDto { - $this->limitCount = $limitCount; + $this->limitCount = $limitCount === null ? null : max(0, $limitCount); return $this; } diff --git a/src/Domain/Core/Dtos/ItemSearchDto.php b/src/Domain/Core/Dtos/ItemSearchDto.php index d86f8e036..cb9d6e71f 100644 --- a/src/Domain/Core/Dtos/ItemSearchDto.php +++ b/src/Domain/Core/Dtos/ItemSearchDto.php @@ -32,14 +32,25 @@ */ class ItemSearchDto { + private readonly int $limitStart; + private readonly int $limitCount; + public function __construct( private ?string $searchString = null, - private readonly ?int $limitStart = 0, - private readonly ?int $limitCount = 0, + ?int $limitStart = 0, + ?int $limitCount = 0, ) { if (!empty($searchString)) { $this->searchString = Filter::safeSearchString($searchString); } + + // How far into a list to start, and how much of it to take, both come from the query + // string — and a negative one reached the server as `LIMIT -1 OFFSET -5`, which is not + // SQL: MariaDB answers `ERROR 1064 ... syntax error`, so the page a caller asked for came + // back as a database failure rather than a page. Nothing was harmed, but nothing an + // ordinary request can say should end up as a syntax error either. + $this->limitStart = max(0, $limitStart ?? 0); + $this->limitCount = max(0, $limitCount ?? 0); } public function getSearchString(): ?string diff --git a/tests/Unit/Domain/Core/Dtos/PaginationIsNotNegativeTest.php b/tests/Unit/Domain/Core/Dtos/PaginationIsNotNegativeTest.php new file mode 100644 index 000000000..0f5f163e6 --- /dev/null +++ b/tests/Unit/Domain/Core/Dtos/PaginationIsNotNegativeTest.php @@ -0,0 +1,118 @@ +. + */ + +namespace SP\Tests\Unit\Domain\Core\Dtos; + +use Aura\SqlQuery\QueryFactory; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use SP\Domain\Account\Dtos\AccountSearchFilterDto; +use SP\Domain\Core\Dtos\ItemSearchDto; + +/** + * How far into a list to start, and how much of it to take, both come from the query string. + * + * `analyzeInt()` reads a negative as a negative — `Filter::getInt('-1')` is `-1` — and nothing + * between the request and the query narrowed it, so `?count=-1` was built into `LIMIT -1` and + * `?start=-5` into `OFFSET -5`. That is not SQL: + * + * ``` + * ERROR 1064 (42000): You have an error in your SQL syntax ... near '-1' + * ``` + * + * so asking for a page came back as a database failure. Nothing was harmed by it and nothing was + * disclosed, but a value an ordinary request can carry should not end up as a syntax error. + * + * There are two ways in, which is why there are two places to clamp: the item grids and the API + * searches build an `ItemSearchDto`, while the account search reads `start` and `rpp` straight + * from the request into `AccountSearchFilterDto`. + */ +#[Group('unitary')] +class PaginationIsNotNegativeTest extends TestCase +{ + /** + * @return array + */ + public static function negativeProvider(): array + { + return [ + 'minus one' => [-1, 0], + 'far negative' => [-999999, 0], + 'zero stays zero' => [0, 0], + 'a real page size is untouched' => [50, 50], + ]; + } + + #[Test] + #[DataProvider('negativeProvider')] + public function anItemSearchNeverAsksForANegativePage(int $given, int $expected): void + { + $dto = new ItemSearchDto('', $given, $given); + + self::assertSame($expected, $dto->getLimitStart()); + self::assertSame($expected, $dto->getLimitCount()); + } + + #[Test] + #[DataProvider('negativeProvider')] + public function anAccountSearchNeverAsksForANegativePage(int $given, int $expected): void + { + $filter = AccountSearchFilterDto::build('') + ->setLimitStart($given) + ->setLimitCount($given); + + self::assertSame($expected, $filter->getLimitStart()); + self::assertSame($expected, $filter->getLimitCount()); + } + + /** + * The statement the server would be sent is a statement it can parse. + * + * Asserted on the SQL rather than on the getters, because the getters were never the problem: + * what went wrong was the string that reached MariaDB. + */ + #[Test] + public function theQueryBuiltFromThoseValuesIsValidSql(): void + { + $dto = new ItemSearchDto('', -5, -1); + + $statement = (new QueryFactory('mysql')) + ->newSelect() + ->cols(['id']) + ->from('User') + ->limit($dto->getLimitCount()) + ->offset($dto->getLimitStart()) + ->getStatement(); + + self::assertDoesNotMatchRegularExpression( + '/LIMIT\s+-|OFFSET\s+-/', + $statement, + 'a negative LIMIT or OFFSET is a syntax error, not an empty page' + ); + } +}