Add escaping method for table names in SQL queries

[tellyworth]

WordPress does not currently provide an explicit method for escaping SQL table names. This leads to potential security vulnerabilities, and makes reviewing code for security unnecessarily difficult.

Core tables are of course implicitly escaped when referenced in queries like SELECT * FROM $wpdb->posts. That’s fine.

When plugins create or reference custom table names, there are no API methods and no guidance as to how they should safely escape those names. Since $wpdb->prepare() surrounds %s references with ' quotes in a way that is incompatible with MySQL table name syntax, it becomes necessary to use bare PHP variables in the first parameter of prepare(), which is inherently risky. Plugins use a variety of ways of mitigating that risk, including none at all. These are examples paraphrased from real code in popular plugins:

$wpdb->query( $wpdb->prepare( "SELECT * FROM my_table_name WHERE …" ) ); // Literal string

$wpdb->query( $wpdb->prepare( "SELECT * FROM $my_table_name WHERE …" ) ); // Variable, which may or may not be assigned a literal or escaped in some way

$wpdb->query( $wpdb->prepare( "SELECT * FROM $this->table_name WHERE …" ) ); // Object property

$wpdb->query( $wpdb->prepare( "SELECT * FROM {$wpdb->prefix}my_table_name WHERE …" ) ); // Variable prefix with literal string

$wpdb->query( $wpdb->prepare( "SELECT * FROM {$this->get_table_name()} WHERE …" ) ); // Interpolated method call

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . sanitize_key( $table_name ) . " WHERE …" ) ); // Attempted escaping

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . preg_replace( '/\W/', '', $table_name ) . " WHERE …" ) ); // Hand-rolled escaping

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . self::TABLE_NAME . " WHERE …" ) ); // Class constant

Some variations use backticks around table names. And of course there are many other examples using similar queries without prepare() statements. Note that no guidance is given to plugin developers, so every one has had to separately decide how to handle this.

Other than the literal string examples, none of these examples are verifiably secure. They might be fine, but it is impossible to be sure without reviewing the code path leading up to that point. Even in the case of a constant, it is possible that the constant might in some circumstances be initialized with data from an insecure source. In cases that make use of OO, it is possible that the table name is initialized in a parent class in a different file or module. It might also be overridden in child classes.

All of this makes it difficult for a human code reviewer to be certain that a given query is secure, even though it uses prepare() statements. Static code analysis is similarly difficult. Developers who make use of wpcs and similar tools inevitably need to exclude their queries from sniffer rules because they will otherwise cause false positive errors.

The solution would be for WordPress to provide an explicit method for safely and defensively escaping table names, as close as possible to the query itself.

I can think of several options:

An esc_sql_table_name() function, to be used like this:

$wpdb->query( $wpdb->prepare( "SELECT * FROM " . esc_sql_table_name( $my_table_name ) . " WHERE …" ) );

A special formatting character supported by prepare(), something like:

$wpdb->query( $wpdb->prepare( "SELECT * FROM %z WHERE …", $my_table_name ) );

A modification to the %s character format that does not add ' quotes when surrounded with backticks:

$wpdb->query( $wpdb->prepare( "SELECT * FROM `%s` WHERE …", $my_table_name ) );

A way to safely add sanitized table names to the $wpdb object:

$wpdb->add_table_name( 'my_table_name' );
$wpdb->query( $wpdb->prepare( "SELECT * FROM $wpdb->my_table_name WHERE …" ) );

This last option does look attractive, but unfortunately doesn’t help with the use case where the table name is necessarily variable such as in a loop, which is somewhat common in things like backup plugins:

foreach ( get_list_of_table_names() as $table_name ) {
	$wpdb->query( $wpdb->prepare( "SELECT COUNT(*) FROM $table_name WHERE …" ) );
}

I would therefore tend to favour both adding an esc_sql_table_name() function, and also supporting one of the special prepare() formatting options (which would of course make use of esc_sql_table_name() behind the scenes).

Note that there is some obvious overlap with the very similar problem of escaping column names. I think that warrants a separate ticket, but the solution is probably similar as for table names.

[iandunn]

+1 to this

---

Developers who make use of wpcs and similar tools inevitably need to exclude their queries from sniffer rules because they will otherwise cause false positive errors.

Even that is problematic, since it opens the door for future mistakes. Take this for instance:

$reimbursements_index = Reimbursements_Dashboard\get_index_table_name();

// phpcs:ignore WordPressDotOrg.sniffs.DirectDB.UnescapedDBParameter -- this is safe
$paid_reimbursements = $wpdb->get_results( "
        SELECT blog_id, request_id, date_paid
        FROM `$reimbursements_index`
        WHERE status = 'wcb-paid'
" );

That's fine, but another developer could come along 6 months later and change it to this:

$reimbursements_index = Reimbursements_Dashboard\get_index_table_name();

// phpcs:ignore WordPressDotOrg.sniffs.DirectDB.UnescapedDBParameter -- this is safe
$paid_reimbursements = $wpdb->get_results( "
        SELECT blog_id, request_id, date_paid
        FROM `$reimbursements_index`
        WHERE
                status = 'wcb-paid' AND
                name = " . $_GET['name']
) );

---

adding an esc_sql_table_name() function, and also supporting one of the special prepare() formatting options

+1

To be descriptive, and avoid potential conflicts with future printf() formats, maybe something like:

$wpdb->query( $wpdb->prepare( "SELECT * FROM %table WHERE …", 'foo' ) ); becomes
"SELECT * FROM `foo` WHERE …"

---

A modification to the %s character format that does not add ' quotes when surrounded with backticks

My instinct is to be a bit leery of that, since it'd be a bit more complicated to implement, which could introduce the potential for esoteric bypasses. I could be convinced that I'm being too paranoid, though.

[craigfrancis]

Unfortunately I didn't find this ticket when creating #54042; where I'm also hoping to update $wpdb->prepare() to support backtick quoted Identifiers (e.g. table/field names).

From the suggestions so far, I'd prefer a placeholder, so the $query argument to $wpdb->prepare() can use the literal-string type that's now available in ​Psalm 4.8.0 and ​PHPStan 0.12.97 (and will hopefully be added to PHP in the future), this would allow us to avoid unsafe-variable concatenation and escaping mistakes.

In my patch, I've used %i for "Identifier"; and because ​printf() Types has historically used %d and %i for Integers, whereas PHP only uses %d, implying the PHP developers are unlikely to use %i for anything else.

My main problem at the moment is formatting, e.g. $wpdb->prepare('id = %5s', $_GET['id']) in the current implementation results in a unquoted (unsafe) string that's padded to 5 characters; whereas my patch ensures the value is quoted, but includes the 2 quote characters in the length.

[craigfrancis]

I've ​updated my patch so that it passes the existing unit tests.

Some basic checks suggest it might run a bit faster than the original - a 10k loop, on a query with 1 argument went from ~0.029 to ~0.023, and 3 arguments went from ~0.045s to ~0.041s (I think that's due to removing a RegEx).

I am still concerned that "%s / %5s" would quote the first string but not the second, but doing so does preserve backwards compatibility - "frequently used in the middle of longer strings, or as table name placeholders".

Update $wpdb->prepare() so it can support identifiers (e.g. table/field names).

Trac ticket: https://core.trac.wordpress.org/ticket/52506

This uses most of ​Pull Request #1668, which includes support for IN(), but is done separately so the individual parts can be discussed.

[craigfrancis]

As requested by Tonya (@hellofromtonya)...

I've split ​my patch so we can discuss Identifiers (table/field names) separately to the IN() operator.

There should be no conflicts with printf(), as ​printf() Types have historically used both %d and %i for signed integers (it's scanf() that treats them differently). ​PHP printf() only uses %d, with %i resulting in the fatal error Unknown format specifier "i", so we can use %i for Identifiers.

I've done my own performance tests, and generally find it about the same, and often a bit faster (the main performance improvement is with integers). This is due using less RegEx. But I'll see if I can get someone else (independent) to confirm.

Security testing, I come from a security background, so I won't lie and simply say this is perfectly secure (you cannot prove security, you can only prove insecurity), and because of the existing "%s / %5s" oddity (for legacy reasons the second value is not quoted, but I have added the $wbdp->unsafe_unquoted_parameters property for security conscious people to switch this off). For Identifiers, they are escaped according to the ​MySQL documentation.

The addition of $wbdp->version = 2 is not needed, but it might help address a concern raised by @johnbillion, so plugins can easily "determine whether it's able to use the new formats" (i.e. someone could have a wp-content/db.php that provides their own prepare() method).

I've added some basic unit tests (suggestions welcome for new ones); they check it accepts table/field names normally, backticks are doubled up (as required by MySQL), and the value is always/consistently quoted (this won't have that legacy quoting issue, and we shouldn't be doing weird things like removing/replacing quotes from the format string).

Update $wpdb->prepare() so it can support identifiers (e.g. table/field names).

Trac ticket: https://core.trac.wordpress.org/ticket/52506

This uses most of ​Pull Request #2191, which includes support for IN(), but is done separately so the individual parts can be discussed.

Replaced by Pull Request ​#2192.

I deleted my repo after I made a mess with the master/trunk change... and I've also changed the patch to use preg_split() rather than using the slightly slower preg_match_all() with PREG_OFFSET_CAPTURE.

[craigfrancis]

I've created a basic ​performance testing page, to give an indication on how well it works with some basic examples, running them 10,000 times to compare the current and my patched version of prepare().

You can also ​download /wp-content/db.php so you can test it locally, or see my ​index.php to see how I've done it (yep, it's basic, it's running on a small virtual machine, with other processes running, the CPU has TurboBoost enabled, and it will be affected by temperature throttling, etc).

[audrasjb]

Let's manage this change early in the release cycle :)

[audrasjb]

As per today's dev chat, let's move this ticket to milestone 6.0 and try to commit it early if possible.

[peterwilsoncc]

After discussing this with a number of contributors, it was decided to hold off for 6.0 while a few more things are worked out. Thanks for your tireless work on this, @craigfrancis, and I appreciate your understanding when we chatted a few hours ago.

[craigfrancis]

Replying to peterwilsoncc:

it was decided to hold off for 6.0 while a few more things are worked out.

Yep, np, I appreciate the need to be careful here. Out of interest though, what things need to be worked out?

I'm not sure what the convention is for WP, but as it's been a while, I'm going to mark some of these conversations as resolved (i.e. where I've replied, and don't think there is any outstanding issues).

[costdev]

This ticket was discussed in the bug scrub.

All PR conversations have been resolved and this now appears ready for committers to take a look.

[davidbaumwald]

Sorry I wasn't able to make the meeting, but I've been following this ticket, and I'd like to see this go in soon. I'm happy to commit it, but I'd like to have @tellyworth one final look at ​PR #2192 as it stands now.

[chaion07]

Thanks @tellyworth for reporting this. We reviewed this ticket during a recent bug-scrub session. Waiting on your feedback on the PR 2192 as suggested by David. Cheers!

[davidbaumwald]

In 53575:

Database: Add %i placeholder support to $wpdb->prepare to escape table and column names.

WordPress does not currently provide an explicit method for escaping SQL table and column names. This leads to potential security vulnerabilities, and makes reviewing code for security unnecessarily difficult. Also, static analysis tools also flag the queries as having unescaped SQL input.

Tables and column names in queries are usually in-the-raw, since using the existing %s will straight quote the value, making the query invalid.

This change introduces a new %i placeholder in $wpdb->prepare to properly quote table and column names using backticks.

Props tellyworth, iandunn, craigfrancis, peterwilsoncc, johnbillion, apokalyptik.
Fixes #52506.

[davidbaumwald]

Reopening to flag for a Dev note. As if he's hasn't done enough, @craigfrancis has kindly drafted one.

thanks for all the hard work everyone. This was merged into core in https://core.trac.wordpress.org/changeset/53575.

[peterwilsoncc]

@davidbaumwald I'm reclosing this as a dev-note is usually published against a closed ticket. An open ticket may lead to confusion as to whether or not the code is committed.

I've also added needs-docs as the extra information section of ​wpdb will need an update to include these placeholders.

Thanks so much for committing this.

[SergeyBiryukov]

In 53584:

Docs: Adjust some DocBlocks in wpdb per the documentation standards.

Includes:

• Wrapping long single-line comments to multi-line for better readability.
• Formatting code blocks to display correctly on the Code Reference.
• Other minor edits for consistency.

This applies to:

• wpdb::$allow_unsafe_unquoted_parameters
• wpdb::escape_identifier()
• wpdb::_escape_identifier_value()
• wpdb::prepare()
• wpdb::has_cap()

Follow-up to [53575].

See #52506, #55646.

[iandunn]

Kudos for all the great work here!

Since WPDB vulnerabilities are especially painful to fix, it may be good to ask for some extra attention on this during the beta period. e.g., ​emailing our HackerOne participants with a link to this ticket, and a reminder that ​bounties are doubled if they report a bug before it launches to users.

cc @hellofromtonya, @ehtis, @peterwilsoncc

Some feedback from Juliette (@jrfnl):

Trac ticket: https://core.trac.wordpress.org/ticket/52506

---

1. Using %1s, you're right, it's not common, so I've replaced those examples (except the "risky" and "safer approaches" examples, to show both styles).
2. Dockblock for _escape_identifier_value(), I copied the "To quote the identifier itself" line directly from the ​MySQL manual, I'm happy to change the word "quote" with "escape", I'm just not sure if a direct quote is better?
3. @access private, you're right, it shouldn't be used on a method.
4. With the doing it wrong line, I'd like to keep the (%s) in there (to show what's wrong), but I also made a mistake in saying "String and Identifier" escaping, because Integers and Floats also exist - does the suggested change help?
5. I've added 'wpdb->allow_unsafe_unquoted_parameters private' to the list of protected_members (so it cannot be easily/accidentally set to false; at least for now).

Dockblock for _escape_identifier_value(), I copied the "To quote the identifier itself" line directly from the ​MySQL manual, I'm happy to change the word "quote" with "escape", I'm just not sure if a direct quote is better?

To me (but that may just be me), it confused me when I first read it in this context (not when I read it in the MySQL manual).

The reason for the confusion is that the prepare() function is all about wrapping the values which replace the placeholders in the correct type of quotes (single quotes, double quotes or backticks), so the term _quoting_ in this context - to me - refers to the surrounding quotes, while this comment is about _how to use the character which is used for the wrapping quotes within the value itself_.

In PHP terminology, this is commonly called _escaping_ and that term is also used in the actual method name _escape_identifier_value().

In that respect, the escape_identifier() method should maybe be called [backtick_]quote_identifier() ?

Also see ​https://www.php.net/manual/en/language.types.string.php#language.types.string.syntax.single and ​https://www.php.net/manual/en/regexp.reference.escape.php for examples of the "escape" terminology being used in PHP.

For transparency, these are the comments I previously shared with @craigfrancis via Slack:

• Example code in various docblocks often use %1s, which is not commonly used. Might be clearer to use %1$s in those examples.
• In the docblock for _escape_identifier_value(): _"To quote the identifier itself, then you need to double the character, e.g. ab`"_ => this sentence is unclear for the average reader. I think replacing "quote" with "escape" would make it clearer.
• @access private should be removed. This tag should only be used for global functions which can't be assigned visibility, not for OO methods which do have visibility. (and to be honest, I'd prefer to get rid of it completely as if something should be private, the code should be written to make it so and should not be a global function)
• Doing it wrong: _"Arguments (%s) cannot be used for both String and Identifier escaping."_ => I saw the discussion about making the message clearer. What about _"The same argument cannot be prepared to be used as both an identifier name as well as a string value"_ ?

With the doing it wrong line, I'd like to keep the (%s) in there (to show what's wrong), but I also made a mistake in saying "String and Identifier" escaping, because Integers and Floats also exist - does the suggested change help?

Current, updated, phrasing as per this PR: _"The arguments (%s) cannot be prepared as both an Identifier, and as a value."_

If I read the code correctly, the %s will be replaced by one or more comma-separated numbers.
Those numbers will correspond to the 0-index based number of the replacement argument(s) - in contrast to the 1-index based numbers used in the placeholder arguments.

In the tests I could only find one example of this: WHERE id = %d AND %i LIKE %2$s LIMIT 1, which - if I read the code correctly - would result in the following _doing it wrong_ message: _The arguments (1) cannot be prepared as both an Identifier, and as a value._

If I'm honest, I'd find that message very confusing as a dev-user anyway for a variety of reasons:

• arguments plural when there is only a single problem situation.
• Is the 1 1-based or 0-based ?
• Does it refer to the placeholders or the replacement arguments ?

Once you look at the actual query code and - in this case - see there is only a single %i, you can extrapolate what the message means to say from there, but it takes work and won't get any easier if there are multiple %i placeholders, with or without numbers:
{{{sql
$query = 'SELECT * FROM %1$i WHERE %1$i.%2$i = %1$s AND %3$i = %2$d' Nonsense query, just to show what I mean.
}}}

This would result in the message:
The arguments (0, 1) cannot be prepared as both an Identifier, and as a value.

... which would confuse me no end.

I know this would probably require some thinking over on how this could be implemented, but the message might become clearer if the formatting placeholders which are involved in the conflict were shown in the error.
Maybe something like:
Arguments cannot be prepared as both an Identifier, and as a value. Found the following conflicts: %1$i and %1$s, %2$i and %2$d

What do you think @craigfrancis ?

[jrf]

FYI: @craigfrancis and me are discussing some additional tweaks to the patch in ​GitHub PR #3087 (leaving this comment as a ping for ticket watchers as Trac does not mail out notifications for GH comments).

Thanks @jrfnl, I've updated the patch to include your suggested error message. For performance reasons, it re-parses $split_query to get the formatting placeholders. This approach will also hopefully prevent any security concerns, because it's part of the if that always returns NULL. I've also updated the tests to check the contents of the error messages.

And thanks for the suggestion to use ReflectionProperty() for the allow_unsafe_unquoted_parameters test, which is needed now this property cannot be changed via __set().

PS: Sorry for the delay, I kinda broke my elbow (ice skating), so doing some of this one handed :-)

Ref the naming of escape_identifier() vs quote_identifier().

I'm happy with either, and I think you're right on the dictionary definition. I don't think I gave it any thought, other than trying to match the other functions/methods that use "escape" in their name (doing a quick search though the PHP files, I'm simply reminded of the slightly flawed "magic quotes")... is there a way to find out what would be preferred?

[milana_cap]

Dev note ​https://make.wordpress.org/core/2022/10/08/escaping-table-and-field-names-with-wpdbprepare-in-wordpress-6-1/

[desrosj]

There's been an issue raised that seems potentially related to the changes on this ticket. I've gone and opened #56933 to explore that.

It's not yet clear if the changes related to this enhancement or the ones associated with #56802 are to blame. If you were a participant on this ticket, please see how you can help push #56933 along and rule out the changesets related to this enhancement (especially with 6.1 due out in a ~24 hours).

Reverts changeset 52506 and adds a test for Trac #56933.

Applies https://core.trac.wordpress.org/attachment/ticket/56933/56933.diff for the revert. Props @SergeyBiryukov.

Trac ticket: https://core.trac.wordpress.org/ticket/52506
Trac ticket: https://core.trac.wordpress.org/ticket/56933

Confirming that test_prepare_with_placeholders_and_array_args fails on the 6.1 branch with the data provider amended as per @SergeyBiryukov's 7205118:

{{{diff
diff --git a/tests/phpunit/tests/db.php b/tests/phpunit/tests/db.php
index b39e31eeb8..6df8afd658 100644
--- a/tests/phpunit/tests/db.php
+++ b/tests/phpunit/tests/db.php
@@ -1717,6 +1717,17 @@ 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' ),

}}}

Gives

1) Tests_DB::test_prepare_with_placeholders_and_array_args with data set #27 ('%%%s%%', 'hello', false, '{7f3cebaf0d13f8356a1e2dae161f...61e8a}')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'{7f3cebaf0d13f8356a1e2dae161f6ceffa27c75b16440fee3b9f62b573861e8a}hello{7f3cebaf0d13f8356a1e2dae161f6ceffa27c75b16440fee3b9f62b573861e8a}'
+'{7f3cebaf0d13f8356a1e2dae161f6ceffa27c75b16440fee3b9f62b573861e8a}'hello'{7f3cebaf0d13f8356a1e2dae161f6ceffa27c75b16440fee3b9f62b573861e8a}'

/var/www/tests/phpunit/tests/db.php:1549
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97

[hellofromTonya]

In 54733:

Database: Revert [53575].

When using '%%%s%%' pattern with $wpdb->prepare(), it works on 6.0.3 but does not on 6.1-RC. Why? The inserted value is wrapped in quotes on 6.1-RC5 whereas it is not on <= 6.0.3.

With 6.1 final release tomorrow, more time is needed to further investigate and test. Reverting this changeset to restore the previous behavior.

This commit also adds a dataset for testing the '%%%s%%' pattern.

Props SergeyBiryukov, hellofromTonya, bernhard-reiter, desrosj, davidbaumwald, jorbin.

Fixes #56933.
See #52506.

Committed via https://core.trac.wordpress.org/changeset/54733.

[SergeyBiryukov]

In 54734:

Database: Revert [53575].

When using '%%%s%%' pattern with $wpdb->prepare(), it works on 6.0.3 but does not on 6.1-RC. Why? The inserted value is wrapped in quotes on 6.1-RC5 whereas it is not on <= 6.0.3.

With 6.1 final release tomorrow, more time is needed to further investigate and test. Reverting this changeset to restore the previous behavior.

This commit also adds a dataset for testing the '%%%s%%' pattern.

Props SergeyBiryukov, hellofromTonya, bernhard-reiter, desrosj, davidbaumwald, jorbin.
Reviewed by hellofromTonya, SergeyBiryukov.
Merges [54733] to the 6.1 branch.
Fixes #56933.
See #52506.

[SergeyBiryukov]

As noted above, the changes were reverted for 6.1 due to an issue raised in #56933.

Reopening to revisit this during the next release cycle.

Update $wpdb->prepare() so it can support identifiers (e.g. table/field names).

Trac ticket: https://core.trac.wordpress.org/ticket/52506

This replaces ​PR 2192, and incorporates the changes suggested by Juliette (@jrfnl) in ​Draft PR 3087

I've created ​PR 3724 to support the annoying "%%%s"... and incorporated these changes with it.

[craigfrancis]

Does anyone have any time to check over ​PR 3724?

The main change (compared to the ​reverted patch) is that I've added the following so %%%s does not quote %s:

substr( $split_query[ $key - 1 ], -1 ) !== '%'

According to the documentation a literal % in the query string is written as %% (and that works), but there is a hidden "feature" where the now escaped % will also affect the following %s (so the value is escaped, but not quoted).

While I think this is unexpected and wrong, some extensions use this to incorrectly use wildcards in their SQL LIKE (​details). Similar to how numbered or formatted string placeholders do not quote values, we cannot break BC at the moment, so there's a longer term plan to disable these unsafe features via allow_unsafe_unquoted_parameters.

I've also included some tweaks from Juliette, ref ​PR 3087.

@craigfrancis Is there any impact on performance for normal queries after these changes?

@mukeshpanchal27, while these changes must technically affect performance (it's doing an extra operation), I cannot measure any difference with some basic testing on my computer.

I will note that my testing is a desktop computer, where I have closed down as many programs as possible (still running the GUI), disabled the processors TurboBoost, and tried to alternate the tests to hopefully keep cache/temperature affects to a minimum... where I'm using hrtime(true) before and after I've called prepare() and prepare_original() 100,000 times with some basic queries... so there will be better setups to more accurately test, but overall I'm fairly confident these changes have not introduced a performance issue.

Thanks for the review @costdev, and yes, the more eyes on this the better.

[audrasjb]

As per today's bug scrub: let's leave this ticket in milestone 6.2 until next week, and punt it if the patch is not ready or if it doesn’t get reviewed in time.

[craigfrancis]

Thanks @audrasjb, I’d still like to get this in 6.2, it’s basically the same as last time, just with the small change as noted in comment 59 (ref ​line 1599), which matches what the original RegEx did.

[ironprogrammer]

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/3724

Steps to Test

Test 1 - %i placeholder functionality

1. Check out the PR or apply the patch for this ticket, ​PR 3724.
2. In your test site's wp-content directory, create a mu-plugins folder if it does not already exist.
3. In this new folder, create a new PHP file based on ​this test plugin gist.
4. In a browser, open your test site's homepage (the test does not run in WP admin).
5. The test plugin should render content similar to the image provided below.
6. When testing is complete, delete or move the test plugin file out of the mu-plugins folder.

Test 2 - regression for this ticket's revert that was addressed in #56933

1. After removing the test plugin above, in the mu-plugins folder, create a new PHP file based on ​this other test plugin from #56933 (props hellofromTonya).
2. In a browser, open your test site's homepage (this test also does not run in WP admin).
3. The test plugin should render a SQL query similar to the one provided below.
4. When testing is complete, delete or move the test plugin out of the mu-plugins folder.

Environment

• Hardware: MacBook Pro Apple M1 Pro
• OS: macOS 12.6.2
• Browser: Safari 16.2
• Database: MySQL 8.0.27
• Server: nginx/1.23.3
• PHP: 7.4.33
• WordPress: 6.2-alpha-54642-src

Actual Results

• ✅ Using %i as a placeholder in $wpdb->prepare() statements escapes table and field names with backticks.
• ✅ Performance using %i to handle escaping compared with current methods like `%1s` appears consistent with previous findings.
• ✅ Regression test shows the PR retains the fix from https://core.trac.wordpress.org/ticket/56933.

Supplemental Artifacts

Test 1 example plugin output

Test 2 example plugin output: the search term "hello" in the LIKE clause is not quoted (i.e. }hello{ and not }'hello'{)

(select wp_posts.ID, post_title, wp_mgmlp_folders.folder_id, pm.meta_value as attached_file, 'a' as item_type 
from wp_posts
LEFT JOIN wp_mgmlp_folders ON(wp_posts.ID = wp_mgmlp_folders.post_id)
LEFT JOIN wp_postmeta AS pm ON (pm.post_id = wp_posts.ID)
LEFT JOIN wp_users AS us ON (wp_posts.post_author = us.ID) 
where post_type = 'mgmlp_media_folder' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '{34eb7a3049b8cba3a0981fed3048fc504dff44a188548d73f755f178c269a3f3}hello{34eb7a3049b8cba3a0981fed3048fc504dff44a188548d73f755f178c269a3f3}')
union all
(select wp_posts.ID, post_title, wp_mgmlp_folders.folder_id, pm.meta_value as attached_file, 'b' as item_type
from wp_posts 
LEFT JOIN wp_mgmlp_folders ON( wp_posts.ID = wp_mgmlp_folders.post_id) 
LEFT JOIN wp_postmeta AS pm ON (pm.post_id = wp_posts.ID) 
LEFT JOIN wp_users AS us ON (wp_posts.post_author = us.ID) 
where post_type = 'attachment' and pm.meta_key = '_wp_attached_file' and SUBSTRING_INDEX(pm.meta_value, '/', -1) like '{34eb7a3049b8cba3a0981fed3048fc504dff44a188548d73f755f178c269a3f3}hello{34eb7a3049b8cba3a0981fed3048fc504dff44a188548d73f755f178c269a3f3}') order by attached_file

[davidbaumwald]

In 55151:

Database: Add %i placeholder support to $wpdb->prepare to escape table and column names, take 2.

[53575] during the 6.1 cycle was reverted in [54734] to address issues around multiple % placeholders not being properly quoted as reported in #56933. Since then, this issue has been resolved and the underlying code improved significantly. Additionally, the unit tests have been expanded and the inline docs have been improved as well.

This change reintroduces %i placeholder support in $wpdb->prepare() to give extenders the ability to safely escape table and column names in database queries.

Follow-up to [53575] and [54734].

Props craigfrancis, jrf, xknown, costdev, ironprogrammer, SergeyBiryukov.
Fixes #52506.

Merged into core in https://core.trac.wordpress.org/changeset/55151.

[Otto42]

Fatal error: Uncaught Error: Call to undefined function str_ends_with() in .../wp-includes/class-wpdb.php on line 1623

This broke the grav-redirect.php functionality on wordpress.org, which is why all the gravatars on this trac page are not showing up.

If WPDB is going to rely on a polyfill function, it probably should be included somewhere in the file.

[SergeyBiryukov]

In 55157:

Database: Replace str_ends_with() usage in wpdb::prepare().

This avoids a fatal error if the file is included directly outside of WordPress core, e.g. by HyperDB.

While WordPress core does include a polyfill function, it is not directly loaded in the wpdb class.

This commit replaces the str_ends_with() calls with substr_compare() for now.

Follow-up to [55151].

Props Otto42.
See #52506.

[Otto42]

Thanks @SergeyBiryukov!

[SergeyBiryukov]

In 55158:

Database: Replace substr_compare() usage with substr() in wpdb::prepare().

This amends the previous commit to avoid a warning on PHP < 7.2.18 if haystack is an empty string:

Warning: substr_compare(): The start position cannot exceed initial string length

Follow-up to [55151], [55157].

See #52506.

[jrf]

Just a question - I can see the use of str_ends_with() has been replaced now, but couldn't this have been solved by moving the include for the wp-includes/compat.php file up ? I though that was loaded pretty early anyway, so I was surprised to see the error being reported.

Note: I'm on the road, so haven't had a chance to properly look at the loading bootstrap. It's just a question which popped up in my mind when I saw the error and the chosen fix. Not meant as a criticism in any way.

[SergeyBiryukov]

Replying to jrf:

Just a question - I can see the use of str_ends_with() has been replaced now, but couldn't this have been solved by moving the include for the wp-includes/compat.php file up ? I though that was loaded pretty early anyway, so I was surprised to see the error being reported.

Yeah, including compat.php from class-wpdb.php would be another option and can still be done if preferable.

I went with a fix that does not add a new dependency to the wpdb class, as that might need more discussion. HyperDB loads the wpdb class outside of WordPress core, so compat.php was not loaded at all in this case. In the standard bootstrap process, compat.php is already loaded earlier than class-wpdb.php.

[jrf]

Replying to SergeyBiryukov:

Replying to jrf:

HyperDB loads the wpdb class outside of WordPress core, so compat.php was not loaded at all in this case.

Ah, that was the missing piece of the puzzle. 👍

[milana_cap]

@craigfrancis, is ​this dev note still valid, or does it needs modifications?

[craigfrancis]

@milana_cap, yes. thanks for checking, it's still valid, but I'm not sure if we need a new dev note... technically everything remains the same, the only difference is that it's in 6.2, so I'm not sure what should be done in this case.

[milana_cap]

Replying to craigfrancis:

@milana_cap, yes. thanks for checking, it's still valid, but I'm not sure if we need a new dev note... technically everything remains the same, the only difference is that it's in 6.2, so I'm not sure what should be done in this case.

Thanks for the prompt reply.

We can change the version number at the beginning and slightly change the content of the info block to reflect this change was postponed. I want to keep the info for historical reasons.

[craigfrancis]

Replying to milana_cap:

We can change the version number at the beginning and slightly change the content of the info block to reflect this change was postponed. I want to keep the info for historical reasons.

Great; are you in the best position to do that, as you know what can/cannot be changed?... if it's a time thing, I think I still have access to make changes, and could do it later this week.

[milana_cap]

That's no problem @craigfrancis, I will do it. Thank you so much.