From d36767afbdffe1a49354a7172d8589886025087f Mon Sep 17 00:00:00 2001 From: Florian Langer Date: Wed, 22 Jul 2026 19:23:58 +0200 Subject: [PATCH] [SECURITY] Fix SQL injection via company size/revenue class filter CompanyRepository::findByFilter()/findAmountByFilter() (and PagevisitRepository::findLatestPagevisitsWithCompanies()) build raw SQL by concatenating FilterDto::getSizeClass() and getRevenueClass() unquoted into the WHERE clause. Those getters only apply the StringUtility::sanitizeString() denylist, which leaves spaces, commas, digits and SQL keywords intact. A backend user with access to the Lead module could therefore inject SQL through the company size/revenue class filter (e.g. UNION-based reads of other tables and bypass of the per-site scoping enforced via getSitesForFilter()). Cast both values to int at the point of concatenation, mirroring the existing branch_code handling in the same class. The filter only ever carries numeric class codes and the previous unquoted comparison was already interpreted numerically by the database, so behaviour is unchanged for legitimate input. Adds a unit test that proves the size_class/revenue_class fragments are no longer injectable. --- .../Domain/Repository/AbstractRepository.php | 4 +- .../Repository/AbstractRepositoryTest.php | 86 +++++++++++++++++++ .../Repository/AbstractRepositoryFixture.php | 29 +++++++ 3 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 Tests/Unit/Domain/Repository/AbstractRepositoryTest.php create mode 100644 Tests/Unit/Fixtures/Domain/Repository/AbstractRepositoryFixture.php diff --git a/Classes/Domain/Repository/AbstractRepository.php b/Classes/Domain/Repository/AbstractRepository.php index 3b7acee1..9b31d367 100644 --- a/Classes/Domain/Repository/AbstractRepository.php +++ b/Classes/Domain/Repository/AbstractRepository.php @@ -343,7 +343,7 @@ protected function extendWhereClauseWithFilterSizeClass(FilterDto $filter, strin if ($table !== '') { $table .= '.'; } - $sql .= ' and ' . $table . 'size_class = ' . $filter->getSizeClass(); + $sql .= ' and ' . $table . 'size_class = ' . (int)$filter->getSizeClass(); } return $sql; } @@ -355,7 +355,7 @@ protected function extendWhereClauseWithFilterRevenueClass(FilterDto $filter, st if ($table !== '') { $table .= '.'; } - $sql .= ' and ' . $table . 'revenue_class = ' . $filter->getRevenueClass(); + $sql .= ' and ' . $table . 'revenue_class = ' . (int)$filter->getRevenueClass(); } return $sql; } diff --git a/Tests/Unit/Domain/Repository/AbstractRepositoryTest.php b/Tests/Unit/Domain/Repository/AbstractRepositoryTest.php new file mode 100644 index 00000000..58d1382e --- /dev/null +++ b/Tests/Unit/Domain/Repository/AbstractRepositoryTest.php @@ -0,0 +1,86 @@ +repository = new AbstractRepositoryFixture(); + } + + public static function sizeAndRevenueClassDataProvider(): array + { + return [ + 'empty value produces no clause' => [ + '', + '', + ], + 'numeric class code is kept' => [ + '3', + ' and c.size_class = 3', + ], + 'zero padded class code is normalised to integer' => [ + '01', + ' and c.size_class = 1', + ], + 'union based sql injection collapses to integer' => [ + '0 union select 99999999,1 order by 1 desc', + ' and c.size_class = 0', + ], + 'boolean based sql injection collapses to integer' => [ + '1 or 1', + ' and c.size_class = 1', + ], + 'injection with denylist characters collapses to integer' => [ + '1) union select password from be_users -- ', + ' and c.size_class = 1', + ], + ]; + } + + #[DataProvider('sizeAndRevenueClassDataProvider')] + public function testExtendWhereClauseWithFilterSizeClassIsNotInjectable( + string $sizeClass, + string $expectedSql + ): void { + $filter = new FilterDto(); + $filter->setSizeClass($sizeClass); + self::assertSame( + $expectedSql, + $this->repository->callExtendWhereClauseWithFilterSizeClass($filter, 'c') + ); + } + + #[DataProvider('sizeAndRevenueClassDataProvider')] + public function testExtendWhereClauseWithFilterRevenueClassIsNotInjectable( + string $revenueClass, + string $expectedSql + ): void { + $filter = new FilterDto(); + $filter->setRevenueClass($revenueClass); + self::assertSame( + str_replace('size_class', 'revenue_class', $expectedSql), + $this->repository->callExtendWhereClauseWithFilterRevenueClass($filter, 'c') + ); + } +} diff --git a/Tests/Unit/Fixtures/Domain/Repository/AbstractRepositoryFixture.php b/Tests/Unit/Fixtures/Domain/Repository/AbstractRepositoryFixture.php new file mode 100644 index 00000000..8a6c496c --- /dev/null +++ b/Tests/Unit/Fixtures/Domain/Repository/AbstractRepositoryFixture.php @@ -0,0 +1,29 @@ +extendWhereClauseWithFilterSizeClass($filter, $table); + } + + public function callExtendWhereClauseWithFilterRevenueClass(FilterDto $filter, string $table = ''): string + { + return $this->extendWhereClauseWithFilterRevenueClass($filter, $table); + } +}