Lead by example: pass unquoted placeholders to $wpdb->prepare()

[jrf]

In the aftermath of last weeks security release which broke a lot of SQL queries in plugins, a new sniff has been added to WPCS to check for incorrect placeholders.

The sniff also verifies that placeholders - as per the documentation - are left unquoted.

Having run this sniff over Core, it turns out that there are some fixes to be made.

See the below report.

I suggest tagging this as an easy-pick for new contributors.

FILE: \src\wp-admin\includes\class-wp-importer.php
----------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------
 32 | ERROR | Placeholders should be unquoted in the query string in $wpdb->prepare(). Found:
    |       | '%s'.
 68 | ERROR | Placeholders should be unquoted in the query string in $wpdb->prepare(). Found:
    |       | '%s'.
----------------------------------------------------------------------------------------


FILE: \src\wp-admin\includes\nav-menu.php
----------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------------
 999 | ERROR | Placeholders should be unquoted in the query string in $wpdb->prepare().
     |       | Found: '%d'.
----------------------------------------------------------------------------------------


FILE: \src\wp-includes\functions.php
----------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------
 4807 | ERROR | Placeholders should be unquoted in the query string in $wpdb->prepare().
      |       | Found: '%d'.
 4824 | ERROR | Placeholders should be unquoted in the query string in $wpdb->prepare().
      |       | Found: '%d'.
----------------------------------------------------------------------------------------


FILE: \src\wp-includes\taxonomy.php
----------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------
 3788 | ERROR | Placeholders should be unquoted in the query string in $wpdb->prepare().
      |       | Found: '%s'.
----------------------------------------------------------------------------------------

Refs:

• ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1160
• ​https://developer.wordpress.org/reference/classes/wpdb/prepare/
• ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#formatting-sql-statements

[johnjamesjacoby]

I suggest tagging this as an easy-pick for new contributors.

I love this suggestion, but think the recent scrutiny around this API means any patch is better than waiting.

Attached patch also includes a PHPDoc fix to the wpdb::esc_like() method.

[jrf]

@johnjamesjacoby Thanks for adding the patch and good call on the doc fix. I think the descriptive documentation could be improved a little as well to state that % wildcards to be used with LIKE should not be passed to the function as that is unclear unless you study the code sample.

The wpdb->prepare() documentation should probably also be more explicit about how % wildcards in LIKE statements should be passed. /cc @DrewAPicture

Also: did you mean to commit this extra file to the patch: wp-content/db-plugins/ludicrousdb ?

[johnjamesjacoby]

did you mean to commit this extra file to the patch

No; sorry. I'll update the original patch to remove that bit.

I agree it's a good idea to be more explicit and obvious about how prepare() expects to be used. Good call!

[jrf]

@johnjamesjacoby Thanks, but am I correct in that your updated patch no longer contains the doc fix to the esc_like() example code ?

[GaryJ]

Replying to jrf:

@johnjamesjacoby Thanks, but am I correct in that your updated patch no longer contains the doc fix to the esc_like() example code ?

It's the last hunk in the patch (wp-includes/wp-db.php) - note the asterisk at the start of the line. May be easier to see on ​GitHub.

[johnjamesjacoby]

am I correct in that your updated patch no longer contains the doc fix

It looks like it's there for me. Maybe caching related, because I just replaced the existing patch vs. adding a new one.

[pento]

In 41628:

Database: Don't quote placeholders in queries going through $wpdb->prepare()

To bring Core into line with the changes to $wpdb->prepare() in WordPress 4.8.2, query placeholders shouldn't be quoted.

Props jrf, johnjamesjacoby.
Fixes #41983.

[jrf]

@pento Thanks for the commit.

Should I open a new ticket for the still open question regarding documentation related improvements ? Or should this ticket stay open for that ?

I think the descriptive documentation could be improved a little as well to state that % wildcards to be used with LIKE should not be passed to the function as that is unclear unless you study the code sample.

The wpdb->prepare() documentation should probably also be more explicit about how % wildcards in LIKE statements should be passed.

[pento]

@jrf: Ah, good point. I'll improve the docs there.

[pento]

In 41632:

Docs: Update the documentation for wpdb::prepare()

The inline documentation for wpdb::prepare() was kind of confusing, and didn't describe some of the behaviour correctly.

Fixes #41983.

[jrf]

@pento Thanks, that does make it clearer.

One further improvement suggestion regarding the wording:

-/- Percentage wildcards (for example, to use in LIKE syntax) must be passed in the string argument, it cannot be inserted in the query string.

+/+ Percentage wildcards (for example, to use in LIKE syntax) must be passed via a substitution argument containing the complete LIKE string, these cannot be inserted directly in the query string. Also see {@see esc_like()}.

/cc @GaryJ Is that phrasing clear enough or have you got a better suggestion ?

[pento]

In 41660:

Docs: Clarify the docs for wpdb::prepare().

Make the usage of the % wildcard in queries clearer.

Props jrf.
Fixes #41983.

[jrf]

Thanks @pento!