WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 weeks ago

#10041 closed defect (bug) (fixed)

like_escape() should escape backslashes too

Reported by: miau_jp Owned by: wonderboymusic
Milestone: 4.0 Priority: high
Severity: normal Version: 2.8
Component: Formatting Keywords: 4.0-early has-patch
Focuses: Cc:

Description

The like_escape() function doesn't escape backslashes.

source:trunk/wp-includes/formatting.php@11518#L2260

	return str_replace(array("%", "_"), array("\\%", "\\_"), $text);

should be ...

	return str_replace(array("\\", "%", "_"), array("\\\\", "\\%", "\\_"), $text);

or simply ...

	return addcslashes($text, '%_');

Considering multi-byte characters ...

	if (function_exists('mb_ereg_replace')) {
		$text = mb_ereg_replace('\\\\', '\\\\', $text);
		$text = mb_ereg_replace('%', '\\%', $text);
		$text = mb_ereg_replace('_', '\\_', $text);
		return $text;
	} else {
		return addcslashes($text, '%_');
	}

Attachments (19)

10041.diff (425 bytes) - added by Denis-de-Bernardy 5 years ago.
10041.2.diff (436 bytes) - added by Denis-de-Bernardy 4 years ago.
10041.3.diff (1.2 KB) - added by Denis-de-Bernardy 4 years ago.
10041.4.diff (2.2 KB) - added by Denis-de-Bernardy 4 years ago.
10041.5.diff (1.5 KB) - added by ampt 3 years ago.
Updated patch 10041.4.diff to apply cleanly against trunk, the changes to /wp-admin/ms-sites.php no longer apply. The unit tests for like_escape pass.
10041.6.diff (1.6 KB) - added by wonderboymusic 17 months ago.
miqro-10041.patch (28.1 KB) - added by miqrogroove 3 months ago.
miqro-10041.2.patch (28.1 KB) - added by miqrogroove 3 months ago.
Fixed a missing arg in schema.php.
miqro-10041.3.patch (29.6 KB) - added by miqrogroove 2 months ago.
Move new function into wpdb.
miqro-10041.4.patch (29.6 KB) - added by miqrogroove 2 months ago.
Fixed missing wildcards from original patch of query.php
miqro-10041.5.patch (29.7 KB) - added by miqrogroove 8 weeks ago.
Refresh
miqro-10041.6.patch (30.0 KB) - added by miqrogroove 8 weeks ago.
Also test chars not special in MySQL.
10041.7.diff (29.7 KB) - added by wonderboymusic 5 weeks ago.
miqro-10041-part1.patch (6.0 KB) - added by miqrogroove 5 weeks ago.
miqro-10041-part2.patch (23.9 KB) - added by miqrogroove 5 weeks ago.
miqro-10041-part3.patch (473 bytes) - added by miqrogroove 5 weeks ago.
Unit test for deprecated code.
10041-comment.php.patch (642 bytes) - added by miqrogroove 5 weeks ago.
Fixes a mistake and improves style.
10041-user.php.patch (867 bytes) - added by miqrogroove 5 weeks ago.
Style tweak.
10041.8.diff (2.2 KB) - added by johnjamesjacoby 4 weeks ago.
Remove deprecated notice. Introduce esc_like_sql() wrapper.

Download all attachments as: .zip

Change History (90)

comment:1 Denis-de-Bernardy5 years ago

  • Keywords needs-patch early added
  • Milestone changed from Unassigned to 2.9
  • Version set to 2.8

Confirming...

mysql> select '\\' like '\\';
+----------------+
| '\\' like '\\' |
+----------------+
|              1 | 
+----------------+
1 row in set (0.00 sec)

mysql> select '\\' like '\\\\';
+------------------+
| '\\' like '\\\\' |
+------------------+
|                1 | 
+------------------+
1 row in set (0.00 sec)

mysql> select '\\' like '\\%';
+-----------------+
| '\\' like '\\%' |
+-----------------+
|               0 | 
+-----------------+
1 row in set (0.00 sec)

mysql> select '\\' like '\\\\%';
+-------------------+
| '\\' like '\\\\%' |
+-------------------+
|                 1 | 
+-------------------+
1 row in set (0.00 sec)

comment:2 Denis-de-Bernardy5 years ago

  • Owner set to Denis-de-Bernardy
  • Status changed from new to accepted

comment:3 Denis-de-Bernardy5 years ago

this is probably good enough as a fix:

return addcslashes($text, '%_');

It's like... WP isn't mb_string safe anyway.

comment:4 Denis-de-Bernardy5 years ago

  • Keywords has-patch added; needs-patch removed

Denis-de-Bernardy5 years ago

comment:5 Denis-de-Bernardy5 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from accepted to assigned

comment:6 westi5 years ago

  • Keywords has-unit-tests added
  • Milestone changed from 2.9 to 3.0

I've added these test cases to wordpress-tests.

We can fix this early in the 3.0 dev cycle.

comment:7 westi5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12460]) Improve like_escape to also escape '\'. Fixes #10041 props miau_jp and Denis-de-Bernardy.

comment:8 westi4 years ago

(In [12957]) Revert [12460] for now while we consider a better fix. See #10041

comment:9 westi4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:10 miqrogroove4 years ago

#12123 closed as dupe. See also [12961].

In the course of discussing the input paths in IRC, we came to some conclusions:

  1. Super-global variables should be addslashes() instead of wpdb->escape(). That was changed in [12961].
  1. I discovered the list of like-special chars is not limited to \ % _ Apparently single and double quotes are also special at the LIKE layer, so searching literally for hello\\'world becomes an interesting test case. This contradicts the MySQL manual, but appears to be accurate. I also confirmed \x, \0, \r, and \n are not special at the LIKE layer.
  1. We did not yet decide on a new pattern for calling like_escape(). Any time a super-global variable is passed in, it must be stripslashes() first. We could make that a requirement, or we could create a second function like_escape_global() that includes the call to stripslashes(). In any case, escape() or prepare() must still be used after like_escape(), unless there are more changes.
  1. Most instances of LIKE do not currently use like_escape(). A partial list was included in #12060.
Last edited 17 months ago by miqrogroove (previous) (diff)

comment:11 miqrogroove4 years ago

I posted these two examples at the mysql.com string comparison page. There's no need to support both of them, but it is an interesting exercise.

SELECT 'hello\\\\\'world' LIKE 'hello\\\\\\\\\\\'world'

or

SELECT 'hello\\\\\'world' LIKE 'hello|\\|\\|\'world' ESCAPE '|'

Last edited 17 months ago by miqrogroove (previous) (diff)

comment:12 miqrogroove4 years ago

I guess we need to ask ourselves, what's the difference between these two queries?

SELECT 'hello\'world' LIKE 'hello\'world';

SELECT 'hello\'world' LIKE 'hello\\\'world';

If you say there's no difference, then we can safely ignore quotes in like_escape(). If you are confused and think this makes no sense, then the extra slashes help explain what's going on.

Last edited 17 months ago by miqrogroove (previous) (diff)

comment:13 Denis-de-Bernardy4 years ago

@robert: the issue isn't quotes, which are properly handled. the issue is backslashes and percent chars.

comment:14 miqrogroove4 years ago

Yes of course. The problem is that quotes are unavoidable when discussing slashes. Earlier, I was trying to explain to Mark the difference between \%
% \' and
\' in LIKE values. Unfortunately, those last 2 examples turned out to be identical, screwing up my point about the need to double-escape slashes and percent chars.

In any case, the focus should be on how to implement stripslashes() and escape/prepare so that the like_escape() function doesn't create vulnerabilities.

Version 0, edited 4 years ago by miqrogroove (next)

comment:15 miqrogroove4 years ago

Here's one way to simplify the issue:

The 3 precedents for using like_escape() in WordPress all have this pattern:

$like = like_escape($var);
$sql = "SELECT whatever LIKE '%$like%';

So, one way to make that secure is...

//$stringin expected slashed
function like_escape($stringin) {
  global $wpdb;
  $stringout = stripslashes($stringin);         // Get raw value
  $stringout = addcslashes($stringout, '\\%_'); // Make value like-safe
  $stringout = $wpdb->_real_escape($stringout); // Make value sql-safe
  return $stringout;
}

Then in places like canonical.php, the call to prepare() would have to be removed when implementing like_escape().

comment:16 miqrogroove4 years ago

  • Keywords has-patch removed
  • Priority changed from low to high
  • Severity changed from minor to normal

Westi, if I were to write a full diff based on the strategy in my previous reply, would you commit it?

comment:17 follow-up: Denis-de-Bernardy4 years ago

tiny note: I think like_escape() should expect *unslashed* data.

see also #12402

comment:18 in reply to: ↑ 17 miqrogroove4 years ago

Replying to Denis-de-Bernardy:

tiny note: I think like_escape() should expect *unslashed* data.

In this situation, the choice to go with slashed or unslashed will have no security impact. like_escape() has an algorithmic requirement for unslashed data, therefore it is more conservative to explicitly stripslashes() within the function. Since it is also the case that all super global values are slashed by WP at load, it makes no sense to require most like_escape() calls to be preceeded by a call to stripslashes(). I think the strategy I proposed above holds to that argument.

Denis-de-Bernardy4 years ago

comment:19 Denis-de-Bernardy4 years ago

  • Keywords has-patch added

Denis-de-Bernardy4 years ago

Denis-de-Bernardy4 years ago

comment:20 nacin4 years ago

  • Milestone changed from 3.0 to 3.1

Early 3.1 per the dev chat today.

comment:21 nacin4 years ago

  • Keywords 3.2-early added; early removed
  • Milestone changed from Awaiting Triage to Future Release

No traction, feedback, or testing. Future for now.

ampt3 years ago

Updated patch 10041.4.diff to apply cleanly against trunk, the changes to /wp-admin/ms-sites.php no longer apply. The unit tests for like_escape pass.

wonderboymusic17 months ago

comment:22 wonderboymusic17 months ago

  • Milestone changed from Future Release to 3.6

Updated the patch so it applies against trunk without dying - this can be perused as part of the overall slashing inspection

comment:23 miqrogroove17 months ago

10041.6.diff would octuple-slash the search value in taxonomy.php. Please correct that.

comment:24 ryan15 months ago

  • Milestone changed from 3.6 to Future Release

comment:25 miqrogroove3 months ago

Is the general direction in 10041.6.diff agreeable to everyone?

The input is a raw string, no slashes added or removed.

The output is a raw LIKE phrase, which is not SQL safe. The phrase can then be used as a string in prepare() or real_escape() as part of a query.

I can pick this up and get it ready for the 4.0 milestone, but this ticket needs a direction.

comment:26 miqrogroove3 months ago

Important: Convolution of slashes in MySQL:

SELECT 'a'
a

SELECT '\a'
a

SELECT '\\a'
\a

SELECT '\\\a'
\a

SELECT '\\\\a'
\\a


SELECT '%'
%

SELECT '\%'
\%

SELECT '\\%'
\%

SELECT '\\\%'
\\%

SELECT '\\\\%'
\\%

The main thing to notice here is the potentially confusing result of a LIKE '\\\%' statement. You might expect this string literal to represent \% which is a literal % at the LIKE layer. That is not the case. At the DML layer, this string literal represents \\% which is a literal slash followed by a wildcard at the LIKE layer.

Just always keep this in mind when testing things for this ticket. It's very different from string literals in PHP or even C++.

comment:27 follow-up: miqrogroove3 months ago

  • Keywords dev-feedback 4.0-early added; has-patch has-unit-tests 3.2-early removed

I'm working on a proper patch that will take about 700 lines to get this fixed and add the needed unit tests.

Rather than posting a work in progress, I'm waiting for some guidance from the lead developers. They are super busy with the 3.9 milestone right now, so it will be some weeks before there is more progress.

I'm thinking like_escape() needs to be deprecated at this point. The problem with tweaking the existing function is that we can't very well patch all of the plugins that are using it. I think this is why slashes were never escaped during all this time. Changing the existing function would affect how and where it can be used, and everywhere else it would be broken.

comment:28 in reply to: ↑ 27 ; follow-up: Denis-de-Bernardy3 months ago

Replying to miqrogroove:

I'm thinking like_escape() needs to be deprecated at this point.

If you do, can I suggest naming the new function as quote_like() or quote_sql_like() or quote_like_sql() rather than esc_like() or esc_sql_like() or esc_like_sql(), to make it clearer that it's not actually escaping anything, but merely quoting LIKE special chars.

Last edited 3 months ago by Denis-de-Bernardy (previous) (diff)

comment:29 follow-up: Denis-de-Bernardy3 months ago

Also, and btw, addclashes() doesn't seem to double-escape occurrences of escaped quotes, so this works fine:

    var_dump(
        mysqli_escape_string($wpdb->dbh, "Hello'\"\\ World"),
        mysqli_escape_string($wpdb->dbh, addcslashes(addslashes("Hello'\"\\ World"), "_%"))
    );

Yields:

string 'Hello\'\"\\ World' (length=17)
string 'Hello\\\'\\\"\\\\ World' (length=23)

And then:

MariaDB [(none)]> select 'Hello\'\"\\ World', 'Hello\'\"\\ World' like 'Hello\\\'\\\"\\\\ World';
+----------------+----------------------------------------------------+
| Hello'"\ World | 'Hello\'\"\\ World' like 'Hello\\\'\\\"\\\\ World' |
+----------------+----------------------------------------------------+
| Hello'"\ World |                                                  1 |
+----------------+----------------------------------------------------+
1 row in set (0.00 sec)

Which probably means that this three-liner would be an adequate replacement that passes all of the tests:

function quote_sql_like($str) {
    return addcslashes(addslashes($str), '%_');
}
Last edited 3 months ago by Denis-de-Bernardy (previous) (diff)

miqrogroove3 months ago

comment:30 in reply to: ↑ 29 miqrogroove3 months ago

Patch attached.

Denis, thanks for the suggestions. I decided to start with esc_like() for consistency, and IMO the name is not going to help or hurt developers. I put some nice big doc blocks in there. If that isn't clear enough, then we would need something more drastic than a new name.

Also, I agree with your earlier comment that quotes are not special at the like layer. We don't need addslashes(). It makes an already complicated situation that much worse.

miqrogroove3 months ago

Fixed a missing arg in schema.php.

comment:31 miqrogroove3 months ago

  • Keywords has-patch added; dev-feedback removed

comment:32 ircbot2 months ago

This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.

comment:33 miqrogroove2 months ago

Status update: I'm hoping to get this patch on the 4.0 milestone ASAP. It needs to be early.

miqro-10041.2.patch has these main features:

in wp-includes/formatting.php correct the escaping function, rename it to esc_like(), and add documentation of appropriate usage.

in wp-includes/deprecated.php make a new home for the old like_escape().

add tests/formatting/EscLike.php with real $wpdb tests instead of ambiguous I/O strings.

delete tests/formatting/LikeEscape.php

All other files involved have calls to $wpdb that were not correct before the patch.

comment:34 ircbot2 months ago

This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.

miqrogroove2 months ago

Move new function into wpdb.

comment:35 miqrogroove2 months ago

Based on initial feedback from nacin, I've attached miqro-10041.3.patch.

  • Moved the new function to wpdb::esc_like()
  • Added 2 more calls to prepare() in wp-includes/query.php for consistency.
  • Refreshed all calls to the new function.
  • Refreshed the new unit tests.

miqrogroove2 months ago

Fixed missing wildcards from original patch of query.php

comment:36 in reply to: ↑ 28 ; follow-up: miqrogroove2 months ago

Replying to Denis-de-Bernardy:

make it clearer that it's not actually escaping anything, but merely quoting LIKE special chars.

I considered this again. Remember, escape is the verb used in SQL, i.e.

LIKE '25\\% of \'people\'.'

is the default equivalent of

LIKE '25\\% of \'people\'.' ESCAPE '\\'

but we could specify

LIKE '25@% of \'people\'.' ESCAPE '@'

so yes, we are actually escaping things in this context.

comment:37 in reply to: ↑ 36 Denis-de-Bernardy2 months ago

Replying to miqrogroove:

Replying to Denis-de-Bernardy:

make it clearer that it's not actually escaping anything, but merely quoting LIKE special chars.

I considered this again. Remember, escape is the verb used in SQL, i.e.

(...)
so yes, we are actually escaping things in this context.

Of course... but the intent of my remark was to avoid the potential for confusion in a not-so-competent plugin dev's mind. If it's called esc_like(), it looks like esc_attr() or esc_sql() and the implicit message is "secure". If we call it quote_like(), it quotes -- without it sending any kind of implicit message that may give a false sense of security.

comment:38 miqrogroove8 weeks ago

Starting to see multiple conflicted files in trunk. Nacin, is there any chance of getting this committed?

miqrogroove8 weeks ago

Refresh

comment:39 Denis-de-Bernardy8 weeks ago

Can I suggest an extra two tests with quotes?

'howdy\''
'howdy\"'

comment:40 Denis-de-Bernardy8 weeks ago

And the same tests with quotes for the data_like_query() test cases.

comment:41 miqrogroove8 weeks ago

I'll add tests on the next refresh, sure. I was thinking it would make sense to also add like-special chars from other SQL flavors so that the unit tests can give accurate results on exotic servers. In those situations the MySQL tests might pass, but an asterisk or a [ could break our flavor of escaping.

comment:42 Denis-de-Bernardy8 weeks ago

+1 to suggested alternative implementation in the latest patch. Quick and dirty tests:

function esc_like_orig($str) {
    return str_replace(array("%", "_"), array("\\%", "\\_"), $str);
}

function esc_like_bad($str) {
    return addcslashes($str, '_%');
}

function esc_like1($str) {
    return addcslashes($str, '_%\\');
}

function esc_like2($str) {
    return addcslashes(addslashes($str), '%_');
}

foreach (array(
    'foo%bar',
    'foo_bar',
    'foo\\bar',
    'foo\'bar',
    'foo"bar',
    'foo\'"\\b\\%_a_%\\\'"r',
) as $val) {
    echo "<h1>$val</h1>";
    var_dump(
        esc_like_orig($val),
        $wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like_orig($val))),
        $wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like_orig($val))."'"),
        esc_like_bad($val),
        $wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like_bad($val))),
        $wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like_bad($val))."'"),
        esc_like1($val),
        $wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like1($val))),
        $wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like1($val))."'"),
        esc_like2($val),
        $wpdb->get_var($wpdb->prepare("SELECT %s LIKE %s", $val, esc_like2($val))),
        $wpdb->get_var("SELECT '".esc_sql($val)."' LIKE '".esc_sql(esc_like2($val))."'")
    );
}

In the above, esc_like1() (suggested in the patch) and esc_like2() (suggested earlier) cover the bases as needed, in contrast to the current implementation (esc_like_orig()) and esc_like_bad() which was incorrectly suggested in the original report.

I'd add the keyword "commit", but I don't seem to be able to.

comment:43 Denis-de-Bernardy8 weeks ago

I still think it should be renamed 'quote_like()` to avoid any potential for plugin or theme devs to assume that it's secure in any way, that said.

comment:44 Denis-de-Bernardy8 weeks ago

One last note, in the interest of highlighting differences between MySQL and Postgres, as the latter is oftentimes much stricter than the former.

Postgres will only resolve the LIKE with backslashes escaped one more time as below — which none of the suggested functions compute:

select E'foo\'"\\b\\%_a_%\\\'"r' like E'foo\'"\\\\b\\\\\%\_a\_\%\\\\\'"r';
select 'foo''"\\b\\%_a_%\\''"r' like 'foo''"\\\\b\\\\\%\_a\_\%\\\\''"r';

MySQL, incidentally, is also happy with the above:

select 'foo\'"\\b\\%_a_%\\\'"r' like 'foo\'"\\\\b\\\\\%\_a\_\%\\\\\'"r';
select 'foo''"\\b\\%_a_%\\''"r' like 'foo''"\\\\b\\\\\%\_a\_\%\\\\''"r';

comment:45 Denis-de-Bernardy8 weeks ago

Oops, nevermind, I forgot to escape it in my Postgres tests. This implementation yields the correct output after getting SQL escaped:

return addcslashes($str, '_%\\');

miqrogroove8 weeks ago

Also test chars not special in MySQL.

comment:46 miqrogroove7 weeks ago

Anyone know if addcslashes was binary safe in PHP 5.2.4?

According to sandbox.onlinephpfunctions.com that function works in 5.2.16 but could give unexpected results in 5.1.6. The versions in between are not available there.

comment:47 miqrogroove7 weeks ago

Found it in the 5.2.2 changelog. Looks like we can use it.

comment:48 ircbot7 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.

wonderboymusic5 weeks ago

comment:49 follow-ups: wonderboymusic5 weeks ago

  • Milestone changed from Future Release to 4.0

10041.7.diff gets rid of some fuzz. I think this patch might need to be broken up into smaller pieces. There is a lot of stuff to test here. If done across multiple commits, each thing it touches can be looked at properly.

comment:50 in reply to: ↑ 49 miqrogroove5 weeks ago

Replying to wonderboymusic:

10041.7.diff gets rid of some fuzz.

That isn't fuzz, sir. You've removed the escaping of several strings that are not properly escaped.

comment:51 in reply to: ↑ 49 miqrogroove5 weeks ago

Replying to wonderboymusic:

I think this patch might need to be broken up into smaller pieces. There is a lot of stuff to test here. If done across multiple commits, each thing it touches can be looked at properly.

Just to make it more digestible, I will put the function changes in one patch, and all of the function calls in a separate patch. The function call patch will be large and touch many files, but each file could be committed separately without breaking anything.

comment:52 wonderboymusic5 weeks ago

In 28711:

LIKE escape sanity:

  • Deprecate like_escape()
  • Add a method to $wpdb, ->esc_like(), and add unit tests

$wpdb::esc_like() is not used yet. As such, many unit tests will throw Unexpected deprecated notice for like_escape. Subsequent commits will alleviate this.

Props miqrogroove.
See #10041.

comment:53 wonderboymusic5 weeks ago

In 28712:

Replace all uses of like_escape() with $wpdb->esc_like().

Props miqrogroove.
See #10041.

miqrogroove5 weeks ago

Unit test for deprecated code.

comment:54 wonderboymusic5 weeks ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from reopened to closed

In 28714:

Fix a unit test for the now deprecated function like_escape().

Props miqrogroove.
Fixes #10041.

comment:55 miqrogroove5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm re-checking everything before I get yelled at. :) comment.php needs some attention. I'll patch it and any other oversights.

miqrogroove5 weeks ago

Fixes a mistake and improves style.

comment:56 wonderboymusic5 weeks ago

In 28720:

In WP_Comment_Query::get_search_sql(), don't double-like-escape.

Props miqrogroove.
See #10041.

comment:57 wonderboymusic5 weeks ago

That commit message is inaccurate - it adds wildcards. I spaced.

comment:58 miqrogroove5 weeks ago

It's okay :)

In terms of style, I could make the same change in user.php. Pull the function calls out of the loop, because the arguments to prepare() are not changing. No bugs there though.

Change it or close it?

comment:59 wonderboymusic5 weeks ago

Patch it if it's better

miqrogroove5 weeks ago

Style tweak.

comment:60 wonderboymusic5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 28722:

Set a variable for like-escaped string before looping in WP_User_Query::get_search_sql().

Props miqrogroove.
Fixes #10041.

comment:61 follow-up: johnjamesjacoby4 weeks ago

A note from Boone about deprecating like_escape() for a class method that's newly introduced in 4.0, and repercussions it will have for plugin authors:

Given that the function is deprecated *and* its replacement is introduced in 4.0, plugin authors can't just swap it out and still support earlier versions of WP. So either we:

  • do nothing and just live with the deprecated notices, at least until we drop support for WP < 4.0
  • create a wrapper function that does the necessary check and find/replace deprecated calls

As much as I find the latter option unpleasant, I think it's probably the right way to go.

In the past, we would go a few releases before adding deprecated notices to functions similar to this one (see: r13096 and #11388) – worth doing the same here?

Consider also that using like_escape() did not previously require a touch to the $wpdb global that's now required using the new $wpdb->esc_like() method. Is there any objection to a general esc_like_sql() function that does the global touch for us, and making like_escape() a wrapper for it?

comment:62 wonderboymusic4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Good questions - deprecation notice is a little brutal, if one of you wants to patch up a proposed solution/change/etc, would be amaze

johnjamesjacoby4 weeks ago

Remove deprecated notice. Introduce esc_like_sql() wrapper.

comment:63 in reply to: ↑ 61 ; follow-up: miqrogroove4 weeks ago

Replying to johnjamesjacoby:

Consider also that using like_escape() did not previously require a touch to the $wpdb global that's now required using the new $wpdb->esc_like() method. Is there any objection to a general esc_like_sql() function that does the global touch for us, and making like_escape() a wrapper for it?

Using esc_like() without a database is a rare situation that would not be encountered by novice programmers. There's nothing wrong with adding another function, but it's unnecessary.

Regarding deprecation: Seems like a lot of fuss over a one-line function. It won't be used in core anymore, and however that's signaled is great.

comment:64 miqrogroove4 weeks ago

The problem with 10041.8.diff is that we can't call esc_like() from like_escape(). They have different and incompatible output.

comment:65 in reply to: ↑ 63 ; follow-up: johnjamesjacoby4 weeks ago

Replying to miqrogroove:

Using esc_like() without a database is a rare situation that would not be encountered by novice programmers. There's nothing wrong with adding another function, but it's unnecessary.

Not "without a database" – without defining the $wpdb global in the current PHP scope.

Regarding deprecation: Seems like a lot of fuss over a one-line function. It won't be used in core anymore, and however that's signaled is great.

Deprecated notices are good things for developer optics, but they're bad for plugins that wish to support older WordPress versions in newer plugin versions. We end up writing our own versions of these functions just to handle the function_exists() checks.

The problem with 10041.8.diff is that we can't call esc_like() from like_escape(). They have different and incompatible output.

There are a few places in r28712 where they are swapped out directly. If they are indeed two functions doing two separate things, deprecating like_escape() without an equal alternative isn't deprecation, it's elimination. If that's truly the intent, I'd suggest a post to make.wordpress.org/core explaining what the original problem is and what steps plugin authors need to take to secure their code and comply with this new approach.

(Just read the comments backlog; sorry for not having done so further. Noticed my patch goes against the decisions made above, but hopefully highlights the confusion that plugin authors will run into without documentation and a migration plan.)

comment:66 in reply to: ↑ 65 ; follow-up: miqrogroove4 weeks ago

Replying to johnjamesjacoby:

There are a few places in r28712 where they are swapped out directly.

Indeed. And in a few places it wasn't possible. Even in core code.

The problem wasn't just with usage. The docs actually said like_escape() was SQL safe even though it was not. So we have to anticipate that message resulted in the function being used in many strange ways in plugins.

comment:67 in reply to: ↑ 66 johnjamesjacoby4 weeks ago

Replying to miqrogroove:

The problem wasn't just with usage. The docs actually said like_escape() was SQL safe even though it was not. So we have to anticipate that message resulted in the function being used in many strange ways in plugins.

You're probably right. We audited usages in BuddyPress last year, and wrote tests for them, which then all broke when like_escape() was deprecated. :)

comment:68 ircbot4 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:69 boonebgorges4 weeks ago

The problem wasn't just with usage. The docs actually said like_escape() was SQL safe even though it was not. So we have to anticipate that message resulted in the function being used in many strange ways in plugins.

I did a quick survey of plugins.svn.wordpress.org. It looks like maybe half the plugins using like_escape() are doing separate SQL sanitization (this includes BuddyPress, fwiw :) ), while the other half is not (or is doing it wrong). It's likely that the incorrect docs for like_escape() had something to do with this. Given the security implications, throwing a deprecated notice to get the attention of developers is probably prudent, and I think this ticket can probably be reclosed.

I strongly second jjj's suggestion above that we get a post somewhere (make.wordpress.org/plugins/) seems like an appropriate place) that explains the change and describes what they need to do to fix. If others agree that this is the right way to go, I can take the reins on that task.

comment:70 miqrogroove3 weeks ago

Here's a description of the issue for developers:

http://www.miqrogroove.com/blog/2014/like-escape-deprecated/

You are welcome to copy that for wordpress.org.

Note: See TracTickets for help on using tickets.