Escape LIKE metacharacters by greg0ire · Pull Request #3013 · doctrine/dbal

greg0ire · 2018-02-05T20:30:52Z

There seems to be a lack of tools to fight against wildcard attacks, or to simply provide a way to let your users search for strings that include metacharacters

todo

add a functional test to make sure valid SQL expressions are built by means of the new API.
allow specifying an escape character in the expression builder like and notLike methods.
implement getWildcardCharacters for RDBMSs that have more than the default wildcard characters.

Taluu · 2018-02-05T20:58:34Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+            $replacePairs[$metaCharacter] = $escapeChar.$metaChar;
+        }
+
+        return strtr($untrusted, $replacePairs);


$untrustedString ?

Taluu · 2018-02-05T20:59:24Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+    }
+
+    abstract protected function getLikeEscapeChar();
+    abstract protected function getLikeMetaCharacters();


: string and : iterable ?

greg0ire · 2018-02-05T21:01:10Z

Please tell me if you're fine with the feature, the naming, and I should add implementations, docs and tests.

morozov · 2018-02-05T21:31:51Z

@greg0ire I have a feeling that in most supported DB platforms, escaping wildcard characters works almost the same.

Instead of introducing new abstract methods which is a breaking change, can you introduce default implementations for them?

morozov · 2018-02-05T21:33:28Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+     */
+    public function escapeStringForLike(string $untrustedString): string
+    {
+        $replacePairs = [];


The escape character itself should also be always escaped. Does this approach account for getLikeMetaCharacters() always returning getLikeEscapeChar() or escapeStringForLike() should handle that itself?

I think the best is to let escapeStringForLike handle it, and I could rename "meta" to "wildcard", what do you think?

Agree on both.

morozov · 2018-02-05T21:36:27Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+     * Escapes metacharacters in a string intended to be used with a LIKE
+     * operator.
+     */
+    public function escapeStringForLike(string $untrustedString): string


This is not about trust. The input is an arbitrary string with no meaning while the output is a LIKE expression.

It could indeed come from a trusted source and still need to be escaped because you genuinely want to search for that 👍

greg0ire · 2018-02-05T21:45:14Z

"Funny" thing: In MS SQL, the escape character has to be picked by the user and has no default: https://docs.microsoft.com/en-us/sql/t-sql/language-elements/like-transact-sql
I think this means any char would do.

morozov · 2018-02-05T21:48:01Z

Funny thing: In MS SQL, the escape character has to be picked by the user and has no default: https://docs.microsoft.com/en-us/sql/t-sql/language-elements/like-transact-sql

In my personal experience, it defaults to ! same as in other databases.

-- Syntax for SQL Server and Azure SQL Database

match_expression [ NOT ] LIKE pattern [ ESCAPE escape_character ]

Why is then the ESCAPE part optional?

greg0ire · 2018-02-05T21:48:47Z

Why is then the ESCAPE part optional?

Probably because you don't always need to escape characters

greg0ire · 2018-02-05T21:49:49Z

For PostgreSQL and MySQL, you can specify an escape char too, but it defaults to backslash

morozov · 2018-02-05T21:52:56Z

Also IIRC, in IBM DB2 there's also a different default escape character. Probably, we need to gather escaping examples for all DBs and then see if the ESCAPE part can be avoided by using the default escape character for the current platform. Otherwise, it may turn out that just escaping the string may be insufficient and the API will need to look somehow differently.

greg0ire · 2018-02-05T21:53:06Z

MySQL sounds like a lot of fun:

 Because MySQL uses C escape syntax in strings (for example, \n to represent a newline character), you must double any \ that you use in LIKE strings. For example, to search for \n, specify it as \\n. To search for \, specify it as \\\\; this is because the backslashes are stripped once by the parser and again when the pattern match is made, leaving a single backslash to be matched against.

Exception: At the end of the pattern string, backslash can be specified as \\. At the end of the string, backslash stands for itself because there is nothing following to escape. Suppose that a table contains the following values:

greg0ire · 2018-02-05T22:07:14Z

Also IIRC, in IBM DB2 there's also a different default escape character. Probably, we need to gather escaping examples for all DBs and then see if the ESCAPE part can be avoided by using the default escape character for the current platform. Otherwise, it may turn out that just escaping the string may be insufficient and the API will need to look somehow differently.

Well the only one which seems problematic to me is MS SQL, because it seems you have to append ESCAPE somechar and escape with that char to get it to work. A solution could be to pick some random char and always use ESCAPE no matter what but that would imply all systems have that escape char specification capability.

greg0ire · 2018-02-05T22:19:10Z

Ok so let's start the gathering

Platform	Escape sequence
PostgreSQL	\
MySQL	\
SQLite	\
MS SQL	none
DB2	none
Oracle	none
SQL Anywhere	none

greg0ire · 2018-02-05T23:13:40Z

the API will need to look somehow differently.

That's right, I think an optional $escapeChar argument could be added to escapeStringForLike, and let the user be responsible for specifying it again when provided whatever they want to the like() method of the expression builder.

morozov · 2018-02-06T00:14:58Z

That's right, I think an optional $escapeChar argument could be added to escapeStringForLike

It should be either mandatory or not configurable at all and accessible in a read-only mode (e.g. const ESCAPE_CHAR = '!').

If mandatory, the caller will always manually pass the character of choice to the escaping function and add it to ESCAPE. If read-only, the caller will always take its value and also add it to ESCAPE.

Otherwise, the caller doesn't know if it's needed for a given string and platform and what to pass to ESCAPE and if it's needed at all.

greg0ire · 2018-02-06T17:17:40Z

I think the mandatory parameter might be best: it offers the most control, and it acts as a better reminder since people not knowing what to specify will read the phpdoc, where I can document that they need to also add ESCAPE

greg0ire · 2018-02-06T19:23:24Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+    final public function escapeStringForLike(string $inputString, string $escapeChar) : string
+    {
+        /* Some RDBMS' can deal with multibyte characters, but let us assume
+         * that kind of will not actually be needed */


Tell me if I should allow them instead, or even remove that check and let the RDBMS deal with that.

I'd say the DB should decide what's valid and what's not. Even if it's known that it should be one character for all supported platforms, there may exist other platform-specific limitations which we'll not be able to cover. Let's not even try.

morozov · 2018-02-06T21:08:11Z

I'd also add a functional test to make sure valid SQL expressions are built by means of the new API.

morozov · 2018-02-06T21:09:11Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php

+    public function testItEscapesStringsForLikeProperly() : void
+    {
+        self::assertSame(
+            '25\% off \\\\o/',


Could you add the underscore here as well?

morozov · 2018-02-06T21:09:56Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php

        );
    }
+
+    public function testItEscapesStringsForLikeProperly() : void


The "properly" part seems redundant. It's implied that the test will only pass if the API is implemented properly.

morozov · 2018-02-06T21:13:06Z

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

+     * operator.
+     *
+     * @param string $inputString a literal, unquoted string
+     * @param string $escapeChar  should be exactly one character long and


If it's not enforced by the API, I'd omit the "should be exactly one character long" part. It's still a DB abstraction layer where all DB limitations are implied but don't necessarily have to be called out. The rest is valid.

morozov · 2018-02-06T21:35:45Z

Not sure if it's outside of the scope but maybe the ~~QueryBuilder~~ExpressionBuilder should be able to build the [NOT] LIKE ? ESCAPE ? expression to encapsulate the SQL syntax. In that case, the client-side code might look like:

$pattern = $platform->escapeStringForLike($input, '!') . '%';
$builder->like($pattern, '!');

The builder, in turn, may or may not append the ESCAPE part the expression depending on whether the escape character or wildcard characters are found in the pattern (for sake of compactness).

Looks a bit complicated but I can't see a simpler approach.

greg0ire · 2018-02-17T11:39:09Z

@Majkl578 @morozov I switched to preg + implode, please review again, and agree on something :P

Majkl578 · 2018-02-17T19:57:47Z

I don't really care which way it is, but array doesn't give any guarantees about chars or even string[] rather than arbitrary mixed[]. I can technically do:

protected function getLikeWildcardCharacters() : array
{
    return [$this];
}

And there will be lots of dragons. 🐲

That's why I proposed using simple string instead of array.

morozov · 2018-02-17T21:01:40Z

Agree, let’s revert to the string. It makes more sense with the regexp approach.

greg0ire · 2018-02-17T21:05:44Z

Agreement! Great, let's do this!

Ocramius · 2018-02-17T21:08:13Z

tests/Doctrine/Tests/DBAL/Query/Expression/ExpressionBuilderTest.php

+    public function testNotLikeWithEscape()
+    {
+        self::assertEquals(
+            "p.description NOT LIKE '20💩%' ESCAPE '💩'",


Classy 💩

Ocramius

LGTM 👍

Ocramius · 2018-02-17T21:12:21Z

Agree, let’s revert to the string. It makes more sense with the regexp approach.

Not needed: string[] is a sufficient documentation for subclasses/implementors. In addition to that, there is no guarantee that the wildcard is a single char (although I do really hope that's the case everywhere :-P)

Ocramius · 2018-02-17T21:15:02Z

Oh well, I see it's done anyway. Waiting for CI, then 🚢

A string is made of characters, an array might contain anything.

greg0ire · 2018-02-17T21:59:59Z

Ready to 🚢

Ocramius · 2018-02-17T22:09:55Z

👍

…mySelectSQL() For testing purposes and not only, it may be needed to evaluate an arbitrary SQL expression (see doctrine#3013, doctrine#3108). Instead of doing `str_replace()` on an expected string, one could specify the expression explicitly.

greg0ire changed the title ~~Escape LIKE metacharacters~~ WIP: Escape LIKE metacharacters Feb 5, 2018

Taluu reviewed Feb 5, 2018

View reviewed changes

greg0ire force-pushed the escape_like_literals branch from dd1b35a to 8ed7349 Compare February 5, 2018 21:00

morozov reviewed Feb 5, 2018

View reviewed changes

greg0ire force-pushed the escape_like_literals branch from 8ed7349 to ad8435c Compare February 5, 2018 22:12

greg0ire force-pushed the escape_like_literals branch from ad8435c to e3f6433 Compare February 5, 2018 23:12

greg0ire force-pushed the escape_like_literals branch from 539896f to c9cf880 Compare February 6, 2018 19:20

greg0ire commented Feb 6, 2018

View reviewed changes

greg0ire force-pushed the escape_like_literals branch 3 times, most recently from d6a3807 to 47b0a67 Compare February 6, 2018 19:52

morozov reviewed Feb 6, 2018

View reviewed changes

greg0ire force-pushed the escape_like_literals branch from 47b0a67 to 85815f6 Compare February 6, 2018 21:27

greg0ire force-pushed the escape_like_literals branch from f8bba7f to a9e5e5a Compare February 15, 2018 23:19

Ocramius reviewed Feb 17, 2018

View reviewed changes

Ocramius approved these changes Feb 17, 2018

View reviewed changes

Ocramius self-assigned this Feb 17, 2018

Ocramius added Improvement Platforms QueryBuilder Quoting labels Feb 17, 2018

Ocramius added this to the 2.7.0 milestone Feb 17, 2018

greg0ire force-pushed the escape_like_literals branch from 493310f to ddda207 Compare February 17, 2018 21:13

greg0ire force-pushed the escape_like_literals branch from ddda207 to fddd873 Compare February 17, 2018 21:23

Use strings to represent wildcard characters

f397fe0

A string is made of characters, an array might contain anything.

greg0ire force-pushed the escape_like_literals branch from fddd873 to f397fe0 Compare February 17, 2018 21:39

Ocramius merged commit ea7657f into doctrine:master Feb 17, 2018

greg0ire deleted the escape_like_literals branch February 17, 2018 23:43

This was referenced Apr 14, 2018

Allow to specify arbitrary SQL expression in AbstractPlatform::getDummySelectSQL() #3109

Merged

Fix like escape for sqlite #3104

Closed

greg0ire mentioned this pull request May 8, 2020

Add Array filter to search value in @ORM\Column(type="array") sonata-project/SonataDoctrineORMAdminBundle#1042

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2022

Uh oh!

Conversation

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

todo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taluu Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Feb 5, 2018

Uh oh!

morozov commented Feb 5, 2018

Uh oh!

morozov Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Feb 5, 2018

Uh oh!

greg0ire commented Feb 5, 2018

Uh oh!

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Feb 5, 2018

Uh oh!

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Feb 17, 2018

Uh oh!

Majkl578 commented Feb 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented Feb 17, 2018

Uh oh!

greg0ire commented Feb 17, 2018

greg0ire commented Feb 5, 2018 •

edited

Loading

Taluu Feb 5, 2018 •

edited

Loading

morozov Feb 5, 2018 •

edited

Loading

greg0ire commented Feb 5, 2018 •

edited

Loading

greg0ire commented Feb 5, 2018 •

edited

Loading

greg0ire commented Feb 5, 2018 •

edited

Loading

greg0ire commented Feb 5, 2018 •

edited

Loading

greg0ire commented Feb 5, 2018 •

edited

Loading

greg0ire commented Feb 5, 2018 •

edited

Loading

morozov commented Feb 6, 2018 •

edited

Loading

morozov commented Feb 6, 2018 •

edited

Loading

Majkl578 commented Feb 17, 2018 •

edited

Loading