Skip to content

Commit

Permalink
[BUGFIX] Add ESCAPE keyword for like() and `notLike() expressions
Browse files Browse the repository at this point in the history 
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 <stefan@buerk.tech>
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
  • Loading branch information
@sbuerk
sbuerk committed Jun 26, 2023
1 parent d75deb0 commit cd000ec
Show file tree
Hide file tree
Showing 5 changed files with 176 additions and 19 deletions.
6 changes: 4 additions & 2 deletions typo3/sysext/core/Classes/Database/Query/Expression/ExpressionBuilder.php
View file
Expand Up @@ -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('\\'));
}

/**
Expand All @@ -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('\\'));
}

/**
Expand Down
3 changes: 3 additions & 0 deletions ...t/core/Tests/Functional/Database/Fixtures/DataSet/TestExpressionBuilderLikeAndNotLike.csv
View file
Expand Up @@ -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"
132 changes: 130 additions & 2 deletions typo3/sysext/core/Tests/Functional/Database/Query/Expression/ExpressionBuilderTest.php
View file
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}
}
Expand Down
51 changes: 36 additions & 15 deletions typo3/sysext/core/Tests/Unit/Database/Query/Expression/ExpressionBuilderTest.php
View file
Expand Up @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand Down
3 changes: 3 additions & 0 deletions typo3/sysext/indexed_search/Tests/Functional/Utility/LikeWildcardTest.php
View file
Expand Up @@ -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));
}

Expand Down

0 comments on commit cd000ec

Please sign in to comment.