From cd000ec93ddb7cbddef154c2afafd5eb0f8cde70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20B=C3=BCrk?= Date: Sun, 14 May 2023 03:59:39 +0200 Subject: [PATCH] [BUGFIX] Add `ESCAPE` keyword for `like()` and `notLike() expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Values for `like` expressions should be escaping the corresponding like wildcards ($ and _). TYPO3 core provides a `escapeLikeWildcards()` in `QueryBuilder` and on the `Connection`. Under the hood, the php `addcslashes()` method is used to escape wildcards in the value before appending/prepending wildcards. It has been assumed, that `\` as escape character is always the default character throughout all database server vendors and versions - which makes `addcslashes()` the one-shot to use. It has been recently discovered, that if values in the database contains one of these wildcards, for example the underscore `_` and a like expression is built using the escape method, the row cannot be matched in all databases. This relates to the fact, that the generated like expressions do not contains the `ESCAPE` keyword to define which escape character has been used. `doctrine/dbal` has added a corresponding argument to the doctrine/dbal query ExpressionBuilder like() and notLike() method, so this can be set. TYPO3 uses a custom ExpressionBuilder, not extending the doctrine ExpressionBuilder (which will change with upcoming doctrine/dbal v4). This change always adds the `ESCAPE` keyword to like and not like expressions with the hardcoded `\` escape character - except for PostgresSQL. PostgresSQL doesn't like it when ILIKE/NOT ILIKE is used, which the ExpressionBuilder does to mimic case insensitive LIKE/NOT LIKE similar to MySql. This can be made configurable in a dedicated patch and must be done for upgrading to doctrine/dbal 4.x anyway. Resolves: #100874 Releases: main, 12.4, 11.5 Change-Id: Id7eb891ef52e8c6988a605eaadd0afcbcf5176bb Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/79491 Tested-by: Stefan Bürk Tested-by: core-ci Reviewed-by: Stefan Bürk --- .../Query/Expression/ExpressionBuilder.php | 6 +- .../TestExpressionBuilderLikeAndNotLike.csv | 3 + .../Expression/ExpressionBuilderTest.php | 132 +++++++++++++++++- .../Expression/ExpressionBuilderTest.php | 51 +++++-- .../Functional/Utility/LikeWildcardTest.php | 3 + 5 files changed, 176 insertions(+), 19 deletions(-) diff --git a/typo3/sysext/core/Classes/Database/Query/Expression/ExpressionBuilder.php b/typo3/sysext/core/Classes/Database/Query/Expression/ExpressionBuilder.php index a4613ae9a873..909c4c60de55 100644 --- a/typo3/sysext/core/Classes/Database/Query/Expression/ExpressionBuilder.php +++ b/typo3/sysext/core/Classes/Database/Query/Expression/ExpressionBuilder.php @@ -250,7 +250,8 @@ public function like(string $fieldName, $value): string // Note: SQLite does not properly work with non-ascii letters as search word for case-insensitive // matching, UPPER() and LOWER() have the same issue, it only works with ascii letters. // See: https://www.sqlite.org/src/doc/trunk/ext/icu/README.txt - return $this->comparison($this->connection->quoteIdentifier($fieldName), 'LIKE', $value); + return $this->comparison($this->connection->quoteIdentifier($fieldName), 'LIKE', $value) + . sprintf(' ESCAPE %s', $this->connection->quote('\\')); } /** @@ -269,7 +270,8 @@ public function notLike(string $fieldName, $value): string // Note: SQLite does not properly work with non-ascii letters as search word for case-insensitive // matching, UPPER() and LOWER() have the same issue, it only works with ascii letters. // See: https://www.sqlite.org/src/doc/trunk/ext/icu/README.txt - return $this->comparison($this->connection->quoteIdentifier($fieldName), 'NOT LIKE', $value); + return $this->comparison($this->connection->quoteIdentifier($fieldName), 'NOT LIKE', $value) + . sprintf(' ESCAPE %s', $this->connection->quote('\\')); } /** diff --git a/typo3/sysext/core/Tests/Functional/Database/Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv b/typo3/sysext/core/Tests/Functional/Database/Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv index acaab1f37ce5..072267bd5bdc 100644 --- a/typo3/sysext/core/Tests/Functional/Database/Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv +++ b/typo3/sysext/core/Tests/Functional/Database/Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv @@ -2,3 +2,6 @@ ,"uid","pid","aField","aCsvField" ,1,0,"likecasing","Fächer, Überraschungen sind Äußerungen" ,2,0,"likecasing","Kleingeschriebenes überlebt halt ältere" +,3,0,"likeescape","underscore_escape_can_be_matched,second_value" +,4,0,"likeescape","not_underscore_value" +,5,0,"likeescape","Some value with a % in it" diff --git a/typo3/sysext/core/Tests/Functional/Database/Query/Expression/ExpressionBuilderTest.php b/typo3/sysext/core/Tests/Functional/Database/Query/Expression/ExpressionBuilderTest.php index 14397b1e5d60..434a6774c2d5 100644 --- a/typo3/sysext/core/Tests/Functional/Database/Query/Expression/ExpressionBuilderTest.php +++ b/typo3/sysext/core/Tests/Functional/Database/Query/Expression/ExpressionBuilderTest.php @@ -756,7 +756,7 @@ public function likeReturnsExpectedDataSets(string $searchWord, array $expectedR $platform = $queryBuilder->getConnection()->getDatabasePlatform(); foreach ($excludePlatforms as $excludePlatform) { if ($platform instanceof $excludePlatform) { - self::markTestSkipped('Exculded platform ' . $excludePlatform); + self::markTestSkipped('Excluded platform ' . $excludePlatform); } } } @@ -819,6 +819,134 @@ public function notLikeReturnsExpectedDataSetsDataProvider(): \Generator ]; } + public static function likeWithWildcardValueCanBeMatchedDataProvider(): \Generator + { + yield 'escaped value with underscore matches properly' => [ + // addcslashes() is used in escapeLikeWildcards() + 'searchWord' => '%' . addcslashes('underscore_escape_can_be_matched', '%_') . '%', + 'expectedRows' => [ + 0 => [ + 'uid' => 3, + 'aCsvField' => 'underscore_escape_can_be_matched,second_value', + ], + ], + 'excludePlatforms' => [], + ]; + + yield 'escaped value with % matches properly' => [ + // addcslashes() is used in escapeLikeWildcards() + 'searchWord' => '%' . addcslashes('a % in', '%_') . '%', + 'expectedRows' => [ + 0 => [ + 'uid' => 5, + 'aCsvField' => 'Some value with a % in it', + ], + ], + 'excludePlatforms' => [], + ]; + } + + /** + * @test + * @dataProvider likeWithWildcardValueCanBeMatchedDataProvider + */ + public function likeWithWildcardValueCanBeMatched(string $searchWord, array $expectedRows, array $excludePlatforms): void + { + $this->importCSVDataSet(__DIR__ . '/../../Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv'); + $queryBuilder = (new ConnectionPool())->getQueryBuilderForTable('tx_expressionbuildertest'); + if ($excludePlatforms !== []) { + $platform = $queryBuilder->getConnection()->getDatabasePlatform(); + foreach ($excludePlatforms as $excludePlatform) { + if ($platform instanceof $excludePlatform) { + self::markTestSkipped('Excluded platform ' . $excludePlatform); + } + } + } + $rows = $queryBuilder + ->select('uid', 'aCsvField') + ->from('tx_expressionbuildertest') + ->where( + // narrow down result set + $queryBuilder->expr()->eq('aField', $queryBuilder->createNamedParameter('likeescape')), + // this is what we are testing + $queryBuilder->expr()->like('aCsvField', $queryBuilder->createNamedParameter($searchWord)) + ) + ->executeQuery() + ->fetchAllAssociative(); + if (is_array($rows)) { + $rows = $this->ensureRowsUidAsInteger($rows); + } + self::assertSame($expectedRows, $rows); + } + + public static function notLikeWithWildcardValueCanBeMatchedDataProvider(): \Generator + { + yield 'escaped value with underscore matches properly' => [ + // addcslashes() is used in escapeLikeWildcards() + 'searchWord' => '%' . addcslashes('underscore_escape_can_be_matched', '%_') . '%', + 'expectedRows' => [ + 0 => [ + 'uid' => 4, + 'aCsvField' => 'not_underscore_value', + ], + 1 => [ + 'uid' => 5, + 'aCsvField' => 'Some value with a % in it', + ], + ], + 'excludePlatforms' => [], + ]; + + yield 'escaped value with wildcard search word matches properly' => [ + // addcslashes() is used in escapeLikeWildcards() + 'searchWord' => '%' . addcslashes('a % in', '%_') . '%', + 'expectedRows' => [ + 0 => [ + 'uid' => 3, + 'aCsvField' => 'underscore_escape_can_be_matched,second_value', + ], + 1 => [ + 'uid' => 4, + 'aCsvField' => 'not_underscore_value', + ], + ], + 'excludePlatforms' => [], + ]; + } + + /** + * @test + * @dataProvider notLikeWithWildcardValueCanBeMatchedDataProvider + */ + public function notLikeWithWildcardValueCanBeMatched(string $searchWord, array $expectedRows, array $excludePlatforms): void + { + $this->importCSVDataSet(__DIR__ . '/../../Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv'); + $queryBuilder = (new ConnectionPool())->getQueryBuilderForTable('tx_expressionbuildertest'); + if ($excludePlatforms !== []) { + $platform = $queryBuilder->getConnection()->getDatabasePlatform(); + foreach ($excludePlatforms as $excludePlatform) { + if ($platform instanceof $excludePlatform) { + self::markTestSkipped('Excluded platform ' . $excludePlatform); + } + } + } + $rows = $queryBuilder + ->select('uid', 'aCsvField') + ->from('tx_expressionbuildertest') + ->where( + // narrow down result set + $queryBuilder->expr()->eq('aField', $queryBuilder->createNamedParameter('likeescape')), + // this is what we are testing + $queryBuilder->expr()->notLike('aCsvField', $queryBuilder->createNamedParameter($searchWord)) + ) + ->executeQuery() + ->fetchAllAssociative(); + if (is_array($rows)) { + $rows = $this->ensureRowsUidAsInteger($rows); + } + self::assertSame($expectedRows, $rows); + } + /** * @test * @dataProvider notLikeReturnsExpectedDataSetsDataProvider @@ -835,7 +963,7 @@ public function notLikeReturnsExpectedDataSets(string $searchWord, array $expect $platform = $queryBuilder->getConnection()->getDatabasePlatform(); foreach ($excludePlatforms as $excludePlatform) { if ($platform instanceof $excludePlatform) { - self::markTestSkipped('Exculded platform ' . $excludePlatform); + self::markTestSkipped('Excluded platform ' . $excludePlatform); } } } diff --git a/typo3/sysext/core/Tests/Unit/Database/Query/Expression/ExpressionBuilderTest.php b/typo3/sysext/core/Tests/Unit/Database/Query/Expression/ExpressionBuilderTest.php index 2fd15fda654d..ca24445c3522 100644 --- a/typo3/sysext/core/Tests/Unit/Database/Query/Expression/ExpressionBuilderTest.php +++ b/typo3/sysext/core/Tests/Unit/Database/Query/Expression/ExpressionBuilderTest.php @@ -169,9 +169,11 @@ public function likeQuotesIdentifier(): void $databasePlatform = $this->prophesize(MockPlatform::class); $databasePlatform->getName()->willReturn('mysql'); $this->connectionProphecy->getDatabasePlatform()->willReturn($databasePlatform->reveal()); + $this->connectionProphecy->quote(Argument::type('string'))->will(function ($args) { + return '"' . $args[0] . '"'; + }); $result = $this->subject->like('aField', "'aValue%'"); - $this->connectionProphecy->quoteIdentifier('aField')->shouldHaveBeenCalled(); - self::assertSame("aField LIKE 'aValue%'", $result); + self::assertSame("aField LIKE 'aValue%' ESCAPE \"\\\"", $result); } /** @@ -182,9 +184,11 @@ public function notLikeQuotesIdentifier(): void $databasePlatform = $this->prophesize(MockPlatform::class); $databasePlatform->getName()->willReturn('mysql'); $this->connectionProphecy->getDatabasePlatform()->willReturn($databasePlatform->reveal()); + $this->connectionProphecy->quote(Argument::type('string'))->will(function ($args) { + return '"' . $args[0] . '"'; + }); $result = $this->subject->notLike('aField', "'aValue%'"); - $this->connectionProphecy->quoteIdentifier('aField')->shouldHaveBeenCalled(); - self::assertSame("aField NOT LIKE 'aValue%'", $result); + self::assertSame("aField NOT LIKE 'aValue%' ESCAPE \"\\\"", $result); } /** @@ -399,20 +403,28 @@ public function inSetForMssql(): void $databasePlatform = $this->prophesize(MockPlatform::class); $databasePlatform->getName()->willReturn('mssql'); $databasePlatform->getStringLiteralQuoteCharacter()->willReturn('\''); - - $this->connectionProphecy->quote('1', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'1'"); - $this->connectionProphecy->quote('1,%', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'1,%'"); - $this->connectionProphecy->quote('%,1', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'%,1'"); - $this->connectionProphecy->quote('%,1,%', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'%,1,%'"); $this->connectionProphecy->quoteIdentifier(Argument::cetera())->will(static function ($args) { return '[' . $args[0] . ']'; }); + $this->connectionProphecy->quote(Argument::cetera())->will(static function ($args) { + $value = (string)($args[0] ?? ''); + $map = [ + '1' => "'1'", + '1,%' => "'1,%'", + '%,1' => "'%,1'", + '%,1,%' => "'%,1,%'", + ]; + if ($map[$value] ?? false) { + return $map[$value]; + } + return "'" . $value . "'"; + }); $this->connectionProphecy->getDatabasePlatform()->willReturn($databasePlatform->reveal()); $result = $this->subject->inSet('aField', "'1'"); - self::assertSame("([aField] = '1') OR ([aField] LIKE '1,%') OR ([aField] LIKE '%,1') OR ([aField] LIKE '%,1,%')", $result); + self::assertSame("([aField] = '1') OR ([aField] LIKE '1,%' ESCAPE '\') OR ([aField] LIKE '%,1' ESCAPE '\') OR ([aField] LIKE '%,1,%' ESCAPE '\')", $result); } /** @@ -584,19 +596,28 @@ public function notInSetForMssql(): void $databasePlatform->getName()->willReturn('mssql'); $databasePlatform->getStringLiteralQuoteCharacter()->willReturn('\''); - $this->connectionProphecy->quote('1', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'1'"); - $this->connectionProphecy->quote('1,%', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'1,%'"); - $this->connectionProphecy->quote('%,1', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'%,1'"); - $this->connectionProphecy->quote('%,1,%', Connection::PARAM_STR)->shouldBeCalled()->willReturn("'%,1,%'"); $this->connectionProphecy->quoteIdentifier(Argument::cetera())->will(static function ($args) { return '[' . $args[0] . ']'; }); + $this->connectionProphecy->quote(Argument::cetera())->will(static function ($args) { + $value = (string)($args[0] ?? ''); + $map = [ + '1' => "'1'", + '1,%' => "'1,%'", + '%,1' => "'%,1'", + '%,1,%' => "'%,1,%'", + ]; + if ($map[$value] ?? false) { + return $map[$value]; + } + return "'" . $value . "'"; + }); $this->connectionProphecy->getDatabasePlatform()->willReturn($databasePlatform->reveal()); $result = $this->subject->inSet('aField', "'1'"); - self::assertSame("([aField] = '1') OR ([aField] LIKE '1,%') OR ([aField] LIKE '%,1') OR ([aField] LIKE '%,1,%')", $result); + self::assertSame("([aField] = '1') OR ([aField] LIKE '1,%' ESCAPE '\') OR ([aField] LIKE '%,1' ESCAPE '\') OR ([aField] LIKE '%,1,%' ESCAPE '\')", $result); } /** diff --git a/typo3/sysext/indexed_search/Tests/Functional/Utility/LikeWildcardTest.php b/typo3/sysext/indexed_search/Tests/Functional/Utility/LikeWildcardTest.php index a1d2b01b48d4..480de80e9f84 100644 --- a/typo3/sysext/indexed_search/Tests/Functional/Utility/LikeWildcardTest.php +++ b/typo3/sysext/indexed_search/Tests/Functional/Utility/LikeWildcardTest.php @@ -51,6 +51,9 @@ public function getLikeQueryPart(string $tableName, string $fieldName, string $l $expected = addcslashes($expected, '\\'); } $expected = $connection->quoteIdentifier($fieldName) . ' ' . $expected; + if (! $connection->getDatabasePlatform() instanceof PostgreSQLPlatform) { + $expected .= ' ESCAPE ' . $connection->quote('\\'); + } self::assertSame($expected, $subject->getLikeQueryPart($tableName, $fieldName, $likeValue)); }