Make WordPress Core

Opened 18 months ago

Last modified 17 months ago

#57149 new defect (bug)

get_table_from_query() doesn't properly handle a prepared escape_like() table name

Reported by: prettyboymp's profile prettyboymp Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Database Keywords:
Focuses: multisite Cc:

Description

A common way to check if a table exists is to run a query like the following, the same way that maybe_create_table() currently works:

<?php
$wpdb->prepare( 'SHOW TABLES LIKE %s', $wpdb->esc_like( $table_name ) );

This results in the prepared query for a table name such as wp_my_custom_table to end up being:

SHOW TABLES LIKE 'wp\\_my\\_custom\\_table'

While the above isn't necessarily a proper query, it is still valid.

The problem is that $wpdb->get_table_from_query() ends up returning the table name with extra slashes, e.g. wp\_my\_custom\_table. This specifically breaks HyperDB which is attempting to use that table name to determine the dataset to use.

Attachments (2)

57149.diff (664 bytes) - added by prettyboymp 18 months ago.
57149.2.diff (671 bytes) - added by prettyboymp 18 months ago.
Updating to code standards

Download all attachments as: .zip

Change History (5)

@prettyboymp
18 months ago

@prettyboymp
18 months ago

Updating to code standards

#1 follow-up: @johnjamesjacoby
18 months ago

Hey @prettyboymp 👋 nice find 🔍 nice patch 🙌

I am able to reproduce this defect in:

  • Vanilla WordPress wpdb
    • maybe_create_table() (not the one in install-helper.php)
    • network_domain_check()
    • display_setup_form()
    • wp-admin/maint/repair.php
  • HyperDB
  • LudicrousDB
  • BerlinDB

Your suggested code change does successfully resolve the problem as you've reported it 💎


Additional research & testing has revealed some potential tangential defects 😬

  1. \ is a legal character in MySQL table names:
    CREATE TABLE `wp\_users` LIKE `wp_users`;
    CREATE TABLE `wp\\_users` LIKE `wp\_users`;
    CREATE TABLE `wp\\\_users` LIKE `wp\\_users`;
    CREATE TABLE `wp\\\\_users` LIKE `wp\\\_users`;
    CREATE TABLE `wp\_\_\_users` LIKE `wp\\\\_users`;
    CREATE TABLE `wp\_\_\_\_users` LIKE `wp\\\\_users`;
    SHOW TABLES LIKE 'wp%users';
    
  2. WordPress does not list NO_BACKSLASH_ESCAPES as an incompatible mode in wpdb#3286
  3. WordPress does not use the ESCAPE clause to enforce \ as the escape character
  4. Documentation for wpdb::prepare() includes the following:
    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 wpdb::esc_like().
    
    Interpretation:
    1. Emphasis: Percentage wildcards (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.
    2. Missing: The docs do not instruct to use a similar approach for Underscore wildcards, or really what approach to use for them at all.

Conclusions:

  1. Table names & Columns in SHOW ... LIKE %s queries...
    • most likely should not use $wpdb->esc_like()
    • most likely should not use $wpdb->prepare()
    • ...because it is not absolutely certain that "_" is intended to be wild or literal
  2. Otherwise, they are mangled with unnecessary slashes...
    • ...causing subsequent $wpdb->get_var() queries to fail...
    • ...because a table name like wp_users gets:
      • esc_like'ed to wp\_users
      • prepare'ed to wp\\_users
    • even though:
      • wp_users is adequately "wild"
      • wp\_users is not syntactically what was being queried
      • wp\\_users is not syntactically what was being queried
    • and _ does not need escaping the way % does
      • I.E. \_\_\_ needs to mean wp\_\_\_users and cannot mean wp___users
  3. Instead, most likely, these queries should be concatenated, unescaped and unprepared, and manually slashed to accommodate the desired matching. In this way, the core str_replace( '\\_', '_', $maybe[2] ) is surprisingly accurate:
    $like = 'wp\\\_\\\_\\\_\\\_users';
    $sql = 'SHOW TABLES LIKE ' . $like;
    $query = $wpdb->get_var( $sql );
    
    ...or...
    $like = $wpdb->esc_like( 'wp\_\_\_\_users' );
    $sql = "SHOW TABLES LIKE '{$like}'";
    $query = $wpdb->get_var( $sql );
    
  4. Perhaps, we are all doing it wrong everywhere, and a deeper conclusion is required?
    • Additionally plausible is that I've missed a mundane detail and all of this is wrong 🌚
    • For all of the obvious reasons, I do not think it is a great idea to remove escaping or preparation without sufficient unit tests.
    • Maybe WordPress officially just-says-no to database tables with slashes in them? Or, some other decision?
Version 3, edited 18 months ago by johnjamesjacoby (previous) (next) (diff)

#2 @johnjamesjacoby
18 months ago

Update: revised my above comment slightly after some rumination 🤔

#3 in reply to: ↑ 1 @prettyboymp
17 months ago

Replying to johnjamesjacoby:

Conclusions:

  1. Table names & Columns in SHOW ... LIKE %s queries...
    • most likely should not use $wpdb->esc_like()
    • most likely should not use $wpdb->prepare()
    • ...because it is not absolutely certain that "_" is intended to be wild or literal

Based on the documentation for $wpdb->esc_like(), we should be able to assume that "_" is intended to be literal. Any wildcards should be added to the like string after $wpdb->esc_like() is applied to it.

/**
...
 * Example Prepared Statement:
 *
 *     $wild = '%';
 *     $find = 'only 43% of planets';
 *     $like = $wild . $wpdb->esc_like( $find ) . $wild;
 *     $sql  = $wpdb->prepare( "SELECT * FROM $wpdb->posts WHERE post_content LIKE '%s'", $like );
...
 */

After some more thought and research, I also now believe I was wrong in my initial assessment. The following is the proper way the query should be escaped.

SHOW TABLES LIKE 'wp\\_my\\_custom\\_table'

Last edited 17 months ago by prettyboymp (previous) (diff)
Note: See TracTickets for help on using tickets.