diff --git a/src/wp-includes/class-wpdb.php b/src/wp-includes/class-wpdb.php index 88317f535e..5260130ffd 100644 --- a/src/wp-includes/class-wpdb.php +++ b/src/wp-includes/class-wpdb.php @@ -654,41 +654,6 @@ class wpdb { 'ANSI', ); - /** - * Backward compatibility, where wpdb::prepare() has not quoted formatted/argnum placeholders. - * - * Historically this could be used for table/field names, or for some string formatting, e.g. - * - * $wpdb->prepare( 'WHERE `%1s` = "%1s something %1s" OR %1$s = "%-10s"', 'field_1', 'a', 'b', 'c' ); - * - * But it's risky, e.g. forgetting to add quotes, resulting in SQL Injection vulnerabilities: - * - * $wpdb->prepare( 'WHERE (id = %1s) OR (id = %2$s)', $_GET['id'], $_GET['id'] ); // ?id=id - * - * This feature is preserved while plugin authors update their code to use safer approaches: - * - * $wpdb->prepare( 'WHERE %1s = %s', $_GET['key'], $_GET['value'] ); - * $wpdb->prepare( 'WHERE %i = %s', $_GET['key'], $_GET['value'] ); - * - * While changing to false will be fine for queries not using formatted/argnum placeholders, - * any remaining cases are most likely going to result in SQL errors (good, in a way): - * - * $wpdb->prepare( 'WHERE %1s = "%-10s"', 'my_field', 'my_value' ); - * true = WHERE my_field = "my_value " - * false = WHERE 'my_field' = "'my_value '" - * - * But there may be some queries that result in an SQL Injection vulnerability: - * - * $wpdb->prepare( 'WHERE id = %1s', $_GET['id'] ); // ?id=id - * - * So there may need to be a `_doing_it_wrong()` phase, after we know everyone can use - * identifier placeholders (%i), but before this feature is disabled or removed. - * - * @since 6.1.0 - * @var bool - */ - private $allow_unsafe_unquoted_parameters = true; - /** * Whether to use mysqli over mysql. Default false. * @@ -1397,37 +1362,6 @@ class wpdb { } } - /** - * Escapes an identifier for a MySQL database, e.g. table/field names. - * - * @since 6.1.0 - * - * @param string $identifier Identifier to escape. - * @return string Escaped identifier. - */ - public function escape_identifier( $identifier ) { - return '`' . $this->_escape_identifier_value( $identifier ) . '`'; - } - - /** - * Escapes an identifier value without adding the surrounding quotes. - * - * - Permitted characters in quoted identifiers include the full Unicode - * Basic Multilingual Plane (BMP), except U+0000. - * - To quote the identifier itself, you need to double the character, e.g. `a``b`. - * - * @since 6.1.0 - * @access private - * - * @link https://dev.mysql.com/doc/refman/8.0/en/identifiers.html - * - * @param string $identifier Identifier to escape. - * @return string Escaped identifier. - */ - private function _escape_identifier_value( $identifier ) { - return str_replace( '`', '``', $identifier ); - } - /** * Prepares a SQL query for safe execution. * @@ -1436,7 +1370,6 @@ class wpdb { * - %d (integer) * - %f (float) * - %s (string) - * - %i (identifier, e.g. table/field names) * * All placeholders MUST be left unquoted in the query string. A corresponding argument * MUST be passed for each placeholder. @@ -1469,10 +1402,6 @@ class wpdb { * @since 5.3.0 Formalized the existing and already documented `...$args` parameter * by updating the function signature. The second parameter was changed * from `$args` to `...$args`. - * @since 6.1.0 Added `%i` for identifiers, e.g. table or field names. - * Check support via `wpdb::has_cap( 'identifier_placeholders' )`. - * This preserves compatibility with sprintf(), as the C version uses - * `%d` and `$i` as a signed integer, whereas PHP only supports `%d`. * * @link https://www.php.net/sprintf Description of syntax. * @@ -1504,6 +1433,28 @@ class wpdb { ); } + // If args were passed as an array (as in vsprintf), move them up. + $passed_as_array = false; + if ( isset( $args[0] ) && is_array( $args[0] ) && 1 === count( $args ) ) { + $passed_as_array = true; + $args = $args[0]; + } + + foreach ( $args as $arg ) { + if ( ! is_scalar( $arg ) && ! is_null( $arg ) ) { + wp_load_translations_early(); + _doing_it_wrong( + 'wpdb::prepare', + sprintf( + /* translators: %s: Value type. */ + __( 'Unsupported value type (%s).' ), + gettype( $arg ) + ), + '4.8.2' + ); + } + } + /* * Specify the formatting allowed in a placeholder. The following are allowed: * @@ -1524,106 +1475,20 @@ class wpdb { */ $query = str_replace( "'%s'", '%s', $query ); // Strip any existing single quotes. $query = str_replace( '"%s"', '%s', $query ); // Strip any existing double quotes. + $query = preg_replace( '/(?allow_unsafe_unquoted_parameters || '' === $format ) { - $placeholder = "'%" . $format . "s'"; - } - } - - // Glue (-2), any leading characters (-1), then the new $placeholder. - $new_query .= $split_query[ $key - 2 ] . $split_query[ $key - 1 ] . $placeholder; - - $key += 3; - $arg_id++; - } - - // Replace $query; and add remaining $query characters, or index 0 if there were no placeholders. - $query = $new_query . $split_query[ $key - 2 ]; - - $dual_use = array_intersect( $arg_identifiers, $arg_strings ); - - if ( count( $dual_use ) ) { - wp_load_translations_early(); - _doing_it_wrong( - 'wpdb::prepare', - sprintf( - /* translators: %s: A comma-separated list of arguments found to be a problem. */ - __( 'Arguments (%s) cannot be used for both String and Identifier escaping.' ), - implode( ', ', $dual_use ) - ), - '6.1.0' - ); - - return; - } + // Count the number of valid placeholders in the query. + $placeholders = preg_match_all( "/(^|[^%]|(%%)+)%($allowed_format)?[sdF]/", $query, $matches ); $args_count = count( $args ); - if ( $args_count !== $placeholder_count ) { - if ( 1 === $placeholder_count && $passed_as_array ) { - /* - * If the passed query only expected one argument, - * but the wrong number of arguments was sent as an array, bail. - */ + if ( $args_count !== $placeholders ) { + if ( 1 === $placeholders && $passed_as_array ) { + // If the passed query only expected one argument, but the wrong number of arguments were sent as an array, bail. wp_load_translations_early(); _doing_it_wrong( 'wpdb::prepare', @@ -1644,7 +1509,7 @@ class wpdb { sprintf( /* translators: 1: Number of placeholders, 2: Number of arguments passed. */ __( 'The query does not contain the correct number of placeholders (%1$d) for the number of arguments passed (%2$d).' ), - $placeholder_count, + $placeholders, $args_count ), '4.8.3' @@ -1654,17 +1519,8 @@ class wpdb { * If we don't have enough arguments to match the placeholders, * return an empty string to avoid a fatal error on PHP 8. */ - if ( $args_count < $placeholder_count ) { - $max_numbered_placeholder = 0; - - for ( $i = 2, $l = $split_query_count; $i < $l; $i += 3 ) { - // Assume a leading number is for a numbered placeholder, e.g. '%3$s'. - $argnum = intval( substr( $split_query[ $i ], 1 ) ); - - if ( $max_numbered_placeholder < $argnum ) { - $max_numbered_placeholder = $argnum; - } - } + if ( $args_count < $placeholders ) { + $max_numbered_placeholder = ! empty( $matches[3] ) ? max( array_map( 'intval', $matches[3] ) ) : 0; if ( ! $max_numbered_placeholder || $args_count < $max_numbered_placeholder ) { return ''; @@ -1673,35 +1529,8 @@ class wpdb { } } - $args_escaped = array(); - - foreach ( $args as $i => $value ) { - if ( in_array( $i, $arg_identifiers, true ) ) { - $args_escaped[] = $this->_escape_identifier_value( $value ); - } elseif ( is_int( $value ) || is_float( $value ) ) { - $args_escaped[] = $value; - } else { - if ( ! is_scalar( $value ) && ! is_null( $value ) ) { - wp_load_translations_early(); - _doing_it_wrong( - 'wpdb::prepare', - sprintf( - /* translators: %s: Value type. */ - __( 'Unsupported value type (%s).' ), - gettype( $value ) - ), - '4.8.2' - ); - - // Preserving old behavior, where values are escaped as strings. - $value = ''; - } - - $args_escaped[] = $this->_real_escape( $value ); - } - } - - $query = vsprintf( $query, $args_escaped ); + array_walk( $args, array( $this, 'escape_by_ref' ) ); + $query = vsprintf( $query, $args ); return $this->add_placeholder_escape( $query ); } @@ -3950,13 +3779,11 @@ class wpdb { * @since 2.7.0 * @since 4.1.0 Added support for the 'utf8mb4' feature. * @since 4.6.0 Added support for the 'utf8mb4_520' feature. - * @since 6.1.0 Added support for the 'identifier_placeholders' feature. * * @see wpdb::db_version() * * @param string $db_cap The feature to check for. Accepts 'collation', 'group_concat', - * 'subqueries', 'set_charset', 'utf8mb4', 'utf8mb4_520', - * or 'identifier_placeholders'. + * 'subqueries', 'set_charset', 'utf8mb4', or 'utf8mb4_520'. * @return bool True when the database feature is supported, false otherwise. */ public function has_cap( $db_cap ) { @@ -4001,12 +3828,6 @@ class wpdb { } case 'utf8mb4_520': // @since 4.6.0 return version_compare( $db_version, '5.6', '>=' ); - case 'identifier_placeholders': // @since 6.1.0 - /* - * As of WordPress 6.1, wpdb::prepare() supports identifiers via '%i', - * e.g. table/field names. - */ - return true; } return false; diff --git a/tests/phpunit/tests/db.php b/tests/phpunit/tests/db.php index b39e31eeb8..4e4b1b8c02 100644 --- a/tests/phpunit/tests/db.php +++ b/tests/phpunit/tests/db.php @@ -494,11 +494,9 @@ class Tests_DB extends WP_UnitTestCase { $this->assertTrue( $wpdb->has_cap( 'collation' ) ); $this->assertTrue( $wpdb->has_cap( 'group_concat' ) ); $this->assertTrue( $wpdb->has_cap( 'subqueries' ) ); - $this->assertTrue( $wpdb->has_cap( 'identifier_placeholders' ) ); $this->assertTrue( $wpdb->has_cap( 'COLLATION' ) ); $this->assertTrue( $wpdb->has_cap( 'GROUP_CONCAT' ) ); $this->assertTrue( $wpdb->has_cap( 'SUBQUERIES' ) ); - $this->assertTrue( $wpdb->has_cap( 'IDENTIFIER_PLACEHOLDERS' ) ); $this->assertSame( version_compare( $wpdb->db_version(), '5.0.7', '>=' ), $wpdb->has_cap( 'set_charset' ) @@ -1717,135 +1715,26 @@ class Tests_DB extends WP_UnitTestCase { false, "'{$placeholder_escape}'{$placeholder_escape}s 'hello'", ), + /* + * @ticket 56933. + * When preparing a '%%%s%%', test that the inserted value + * is not wrapped in single quotes between the 2 hex values. + */ + array( + '%%%s%%', + 'hello', + false, + "{$placeholder_escape}hello{$placeholder_escape}", + ), array( "'%-'#5s' '%'#-+-5s'", array( 'hello', 'foo' ), false, "'hello' 'foo##'", ), - array( - 'SELECT * FROM %i WHERE %i = %d;', - array( 'my_table', 'my_field', 321 ), - false, - 'SELECT * FROM `my_table` WHERE `my_field` = 321;', - ), - array( - 'WHERE %i = %d;', - array( 'evil_`_field', 321 ), - false, - 'WHERE `evil_``_field` = 321;', // To quote the identifier itself, then you need to double the character, e.g. `a``b`. - ), - array( - 'WHERE %i = %d;', - array( 'evil_````````_field', 321 ), - false, - 'WHERE `evil_````````````````_field` = 321;', - ), - array( - 'WHERE %i = %d;', - array( '``evil_field``', 321 ), - false, - 'WHERE `````evil_field````` = 321;', - ), - array( - 'WHERE %i = %d;', - array( 'evil\'field', 321 ), - false, - 'WHERE `evil\'field` = 321;', - ), - array( - 'WHERE %i = %d;', - array( 'evil_\``_field', 321 ), - false, - 'WHERE `evil_\````_field` = 321;', - ), - array( - 'WHERE %i = %d;', - array( 'evil_%s_field', 321 ), - false, - "WHERE `evil_{$placeholder_escape}s_field` = 321;", - ), - array( - 'WHERE %i = %d;', - array( 'value`', 321 ), - false, - 'WHERE `value``` = 321;', - ), - array( - 'WHERE `%i = %d;', - array( ' AND evil_value', 321 ), - false, - 'WHERE `` AND evil_value` = 321;', // Won't run (SQL parse error: "Unclosed quote"). - ), - array( - 'WHERE %i` = %d;', - array( 'evil_value -- ', 321 ), - false, - 'WHERE `evil_value -- `` = 321;', // Won't run (SQL parse error: "Unclosed quote"). - ), - array( - 'WHERE `%i`` = %d;', - array( ' AND true -- ', 321 ), - false, - 'WHERE `` AND true -- ``` = 321;', // Won't run (Unknown column ''). - ), - array( - 'WHERE ``%i` = %d;', - array( ' AND true -- ', 321 ), - false, - 'WHERE ``` AND true -- `` = 321;', // Won't run (SQL parse error: "Unclosed quote"). - ), - array( - 'WHERE %2$i = %1$d;', - array( '1', 'two' ), - false, - 'WHERE `two` = 1;', - ), - array( - 'WHERE \'%i\' = 1 AND "%i" = 2 AND `%i` = 3 AND ``%i`` = 4 AND %15i = 5', - array( 'my_field1', 'my_field2', 'my_field3', 'my_field4', 'my_field5' ), - false, - 'WHERE \'`my_field1`\' = 1 AND "`my_field2`" = 2 AND ``my_field3`` = 3 AND ```my_field4``` = 4 AND ` my_field5` = 5', // Does not remove any existing quotes, always adds it's own (safer). - ), - array( - 'WHERE id = %d AND %i LIKE %2$s LIMIT 1', - array( 123, 'field -- ', false ), - true, // Incorrect usage. - null, // Should be rejected, otherwise the `%1$s` could use Identifier escaping, e.g. 'WHERE `field -- ` LIKE field -- LIMIT 1' (thanks @vortfu). - ), - array( - 'WHERE %i LIKE %s LIMIT 1', - array( "field' -- ", "field' -- " ), - false, - "WHERE `field' -- ` LIKE 'field\' -- ' LIMIT 1", // In contrast to the above, Identifier vs String escaping is used. - ), ); } - public function test_allow_unsafe_unquoted_parameters() { - global $wpdb; - - $sql = 'WHERE (%i = %s) OR (%10i = %10s) OR (%5$i = %6$s)'; - $values = array( 'field_a', 'string_a', 'field_b', 'string_b', 'field_c', 'string_c' ); - - $default = $wpdb->allow_unsafe_unquoted_parameters; - - $wpdb->allow_unsafe_unquoted_parameters = true; - - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared - $part = $wpdb->prepare( $sql, $values ); - $this->assertSame( 'WHERE (`field_a` = \'string_a\') OR (` field_b` = string_b) OR (`field_c` = string_c)', $part ); // Unsafe, unquoted parameters. - - $wpdb->allow_unsafe_unquoted_parameters = false; - - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared - $part = $wpdb->prepare( $sql, $values ); - $this->assertSame( 'WHERE (`field_a` = \'string_a\') OR (` field_b` = \' string_b\') OR (`field_c` = \'string_c\')', $part ); - - $wpdb->allow_unsafe_unquoted_parameters = $default; - - } - /** * @dataProvider data_escape_and_prepare */