Skip to:
Content

Opened 17 months ago

Closed 17 months ago

Last modified 17 months ago

#3168 closed defect (fixed)

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

Reported by: jrf Owned by: johnjamesjacoby
Milestone: 2.6 Priority: normal
Severity: normal Version: trunk
Component: API - Importers Keywords: has-patch commit
Cc: johnjamesjacoby, jrf

Description

As per the method documentation:

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

Attachments (1)

trac-3168-fix-convertor-db-query-unescaped-literal.patch (1.2 KB) - added by jrf 17 months ago.

Download all attachments as: .zip

Change History (10)

#1 @johnjamesjacoby
17 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 2.6
  • Owner set to johnjamesjacoby

#2 @johnjamesjacoby
17 months ago

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

In 6715:

Tools: Remove quotes from prepared query statements.

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

Props jrf. Fixes #3168.

#3 @jrf
17 months ago

@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/

#4 @johnjamesjacoby
17 months ago

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.

#5 @jrf
17 months ago

@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.

#6 @johnjamesjacoby
17 months ago

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

#7 @netweb
17 months ago

wp:changeset:41660 was committed today:

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

    a b  
    11641164        if ( func_num_args() === 1 && function_exists( '_deprecated_function' ) )
    11651165            _deprecated_function( __METHOD__, '3.6.0', 'wpdb::prepare() or esc_sql()' );
    11661166        if ( is_array( $data ) ) {
    11671167            foreach ( $data as $k => $v ) {
    11681168                if ( is_array( $v ) )
    11691169                    $data[$k] = $this->escape( $v, 'recursive' );
    11701170                else
    11711171                    $data[$k] = $this->_weak_escape( $v, 'internal' );
    11721172            }
    11731173        } else {
    11741174            $data = $this->_weak_escape( $data, 'internal' );
    11751175        }
    11761176
    11771177        return $data;
    11781178    }
    11791179
    11801180    /**
    11811181     * Escapes content by reference for insertion into the database, for security
    11821182     *
    11831183     * @uses wpdb::_real_escape()
    11841184     *
    11851185     * @since 2.3.0
    11861186     *
    11871187     * @param string $string to escape
    11881188     */
    11891189    public function escape_by_ref( &$string ) {
    11901190        if ( ! is_float( $string ) )
    11911191            $string = $this->_real_escape( $string );
    11921192    }
    11931193
    11941194    /**
    11951195     * Prepares a SQL query for safe execution. Uses sprintf()-like syntax.
    11961196     *
    11971197     * The following placeholders can be used in the query string:
    11981198     *   %d (integer)
    11991199     *   %f (float)
    12001200     *   %s (string)
    12011201     *
    12021202     * All placeholders MUST be left unquoted in the query string. A corresponding argument MUST be passed for each placeholder.
    12031203     *
    1204      * Literal percentage signs (%) in the query string must be written as %%. Percentage wildcards (for example, to use in LIKE syntax)
    1205      * must be passed in the string argument, it cannot be inserted in the query string.
     1204     * Literal percentage signs (%) in the query string must be written as %%. Percentage wildcards (for example,
     1205     * to use in LIKE syntax) must be passed via a substitution argument containing the complete LIKE string, these
     1206     * cannot be inserted directly in the query string. Also see {@see esc_like()}.
    12061207     *
    12071208     * This method DOES NOT support sign, padding, alignment, width or precision specifiers.
    12081209     * This method DOES NOT support argument numbering or swapping.
    12091210     *
    12101211     * Arguments may be passed as individual arguments to the method, or as a single array containing all arguments. A combination
    12111212     * of the two is not supported.
    12121213     *
    12131214     * Examples:
    12141215     *     $wpdb->prepare( "SELECT * FROM `table` WHERE `column` = %s AND `field` = %d OR `other_field` LIKE %s", array( 'foo', 1337, '%bar' ) );
    12151216     *     $wpdb->prepare( "SELECT DATE_FORMAT(`field`, '%%c') FROM `table` WHERE `column` = %s", 'foo' );
    12161217     *
    12171218     * @link https://secure.php.net/sprintf Description of syntax.
    12181219     * @since 2.3.0
    12191220     *
    12201221     * @param string      $query    Query statement with sprintf()-like placeholders
    12211222     * @param array|mixed $args     The array of variables to substitute into the query's placeholders if being called with an array of arguments,
    12221223     *                              or the first variable to substitute into the query's placeholders if being called with individual arguments.
    12231224     * @param mixed       $args,... further variables to substitute into the query's placeholders if being called wih individual arguments.
    12241225     * @return string|void Sanitized query string, if there is a query to prepare.
    12251226     */
    12261227    public function prepare( $query, $args ) {
    12271228        if ( is_null( $query ) )
    12281229            return;
    12291230
    12301231        // This is not meant to be foolproof -- but it will catch obviously incorrect usage.
    12311232        if ( strpos( $query, '%' ) === false ) {
    12321233            _doing_it_wrong( 'wpdb::prepare', sprintf( __( 'The query argument of %s must have a placeholder.' ), 'wpdb::prepare()' ), '3.9.0' );
    12331234        }
    12341235
    12351236        $args = func_get_args();
    12361237        array_shift( $args );
    12371238
    12381239        // If args were passed as an array (as in vsprintf), move them up
    12391240        if ( is_array( $args[0] ) && count( $args ) == 1 ) {
    12401241            $args = $args[0];
    12411242        }
    12421243
    12431244        foreach ( $args as $arg ) {
    12441245            if ( ! is_scalar( $arg ) && ! is_null( $arg ) ) {
    12451246                _doing_it_wrong( 'wpdb::prepare', sprintf( __( 'Unsupported value type (%s).' ), gettype( $arg ) ), '4.8.2' );

#8 @johnjamesjacoby
17 months ago

Yep! This looks all good. Thanks everyone. 👍

#9 @jrf
17 months ago

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

Note: See TracTickets for help on using tickets.