Fix unescaped literal in $wpdb->prepare() call

[jrf]

As per the ​method documentation:

Literals (%) as parts of the query must be properly written as %%.

[johnjamesjacoby]

In 6715:

Tools: Remove quotes from prepared query statements.

Also use the same esc_like() result in 2 matched queries.

Props jrf. Fixes #3168.

[jrf]

@johnjamesjacoby I've looked at the fix now committed, AFAICS the original issue I reported is not addressed. At the same time, looking at the code of the wpdb methods involved, this may well be a shortcoming in the WP Core documentation instead of something that needs to be fixed here.

The wpdb->prepare() documentation states that a literal % need to be escaped to %%, however, it doesn't mention how to handle the %'s in LIKE statements and a quick test shows that the query here should work correctly the way it is now, which goes against the expectations created by the WP documentation (and I doubt it would work the same if the % was used at the start of an arbitrary phrase passed to LIKE).

Refs:
​https://developer.wordpress.org/reference/classes/wpdb/esc_like/
​https://developer.wordpress.org/reference/classes/wpdb/prepare/

[johnjamesjacoby]

I had a really nice, in-depth reply crafted for you here, and Trac ate it and I couldn't recover it. :/

Long story short, I'm going to do a deeper dive into whether %% is actually correct or not, before considering this resolved.

WordPress core literally does not use the %% literal handler anywhere, even for LIKE queries. It might just be a carryover from wp_sprintf() which also is only circumstantially used.

[jrf]

@johnjamesjacoby Sorry you lost the in-depth reply. I really appreciate that you're willing to deep dive into this.
Based on the hackedy test I ran your current code is correct (one % in this query), but the inconsistency with the documentation is curious and grating and may well be the cause of more issues in the wider WP plugin landscape, so it would be great to get more clarity about this.

I came across this query through a new sniff I created for WPCS for which I did a test run on bbPress - see ​https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1160 .
I largely based the sniff code on the documentation (inspired by the issues with the unsupported placeholders since the release of 4.8.2).
I will adjust the sniff behaviour based on your findings as I wouldn't like to be the cause of more confusion about this :-), so will also try and hold off the release of the next version of WPCS until there is clarity.

[johnjamesjacoby]

Sounds good. And thank you for your diligence! Always nice to have a partner in crime. :)

[netweb]

​wp:changeset:41660 was committed today:

trunk/src/wp-includes/wp-db.php

@@ -1164 +1164 @@
         if ( func_num_args() === 1 && function_exists( '_deprecated_function' ) )
             _deprecated_function( __METHOD__, '3.6.0', 'wpdb::prepare() or esc_sql()' );
         if ( is_array( $data ) ) {
             foreach ( $data as $k => $v ) {
                 if ( is_array( $v ) )
                     $data[$k] = $this->escape( $v, 'recursive' );
                 else
                     $data[$k] = $this->_weak_escape( $v, 'internal' );
             }
         } else {
             $data = $this->_weak_escape( $data, 'internal' );
         }
 
         return $data;
     }
 
     /**
      * Escapes content by reference for insertion into the database, for security
      *
      * @uses wpdb::_real_escape()
      *
      * @since 2.3.0
      *
      * @param string $string to escape
      */
     public function escape_by_ref( &$string ) {
         if ( ! is_float( $string ) )
             $string = $this->_real_escape( $string );
     }
 
     /**
      * Prepares a SQL query for safe execution. Uses sprintf()-like syntax.
      *
      * The following placeholders can be used in the query string:
      *   %d (integer)
      *   %f (float)
      *   %s (string)
      *
      * All placeholders MUST be left unquoted in the query string. A corresponding argument MUST be passed for each placeholder.
      *
-     * Literal percentage signs (%) in the query string must be written as %%. Percentage wildcards (for example, to use in LIKE syntax)
-     * must be passed in the string argument, it cannot be inserted in the query string.
+     * Literal percentage signs (%) in the query string must be written as %%. 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()}.
      *
      * This method DOES NOT support sign, padding, alignment, width or precision specifiers.
      * This method DOES NOT support argument numbering or swapping.
      *
      * Arguments may be passed as individual arguments to the method, or as a single array containing all arguments. A combination 
      * of the two is not supported.
      *
      * Examples:
      *     $wpdb->prepare( "SELECT * FROM `table` WHERE `column` = %s AND `field` = %d OR `other_field` LIKE %s", array( 'foo', 1337, '%bar' ) );
      *     $wpdb->prepare( "SELECT DATE_FORMAT(`field`, '%%c') FROM `table` WHERE `column` = %s", 'foo' );
      *
      * @link https://secure.php.net/sprintf Description of syntax.
      * @since 2.3.0
      *
      * @param string      $query    Query statement with sprintf()-like placeholders
      * @param array|mixed $args     The array of variables to substitute into the query's placeholders if being called with an array of arguments,
      *                              or the first variable to substitute into the query's placeholders if being called with individual arguments.
      * @param mixed       $args,... further variables to substitute into the query's placeholders if being called wih individual arguments.
      * @return string|void Sanitized query string, if there is a query to prepare.
      */
     public function prepare( $query, $args ) {
         if ( is_null( $query ) )
             return;
 
         // This is not meant to be foolproof -- but it will catch obviously incorrect usage.
         if ( strpos( $query, '%' ) === false ) {
             _doing_it_wrong( 'wpdb::prepare', sprintf( __( 'The query argument of %s must have a placeholder.' ), 'wpdb::prepare()' ), '3.9.0' );
         }
 
         $args = func_get_args();
         array_shift( $args );
 
         // If args were passed as an array (as in vsprintf), move them up
         if ( is_array( $args[0] ) && count( $args ) == 1 ) {
             $args = $args[0];
         }
 
         foreach ( $args as $arg ) {
             if ( ! is_scalar( $arg ) && ! is_null( $arg ) ) {
                 _doing_it_wrong( 'wpdb::prepare', sprintf( __( 'Unsupported value type (%s).' ), gettype( $arg ) ), '4.8.2' );

[johnjamesjacoby]

Yep! This looks all good. Thanks everyone. 👍

[jrf]

Yes! As far as I'm concerned we're all good now with regards to this. Will update the sniff later today.