#52506 closed defect (bug) (fixed)
Add escaping method for table names in SQL queries
Reported by: | tellyworth | Owned by: | davidbaumwald |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Database | Keywords: | has-unit-tests early needs-docs has-patch needs-testing needs-dev-note |
Focuses: | performance | Cc: |
Description
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.
Change History (84)
#2
@
3 years ago
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.
#3
@
3 years ago
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".
This ticket was mentioned in PR #2127 on WordPress/wordpress-develop by craigfrancis.
3 years ago
#4
- Keywords has-patch has-unit-tests added
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.
#5
@
3 years ago
As requested by Tonya (@hellofromtonya)...
I've split my patch so we can discuss escaping Identifiers (table/field names). The other part is covered by Ticket 54042 to support 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).
This ticket was mentioned in PR #2192 on WordPress/wordpress-develop by craigfrancis.
3 years ago
#6
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.
craigfrancis commented on PR #2127:
3 years ago
#7
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.
#8
@
3 years ago
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).
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
3 years ago
#11
@
3 years ago
- Milestone changed from Awaiting Review to 6.0
As per today's dev chat, let's move this ticket to milestone 6.0 and try to commit it early
if possible.
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by sergey. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
#20
follow-up:
↓ 21
@
3 years ago
- Milestone changed from 6.0 to Future Release
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.
#21
in reply to:
↑ 20
@
3 years ago
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?
craigfrancis commented on PR #2192:
3 years ago
#22
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).
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
3 years ago
#25
@
3 years ago
- Keywords commit added; dev-feedback removed
- Milestone changed from Future Release to 6.1
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.
#26
@
3 years ago
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.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
2 years ago
#28
@
2 years ago
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!
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
#33
@
2 years ago
- Keywords needs-dev-note added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening to flag for a Dev note. As if he's hasn't done enough, @craigfrancis has kindly drafted one.
dream-encode commented on PR #2192:
2 years ago
#34
thanks for all the hard work everyone. This was merged into core in https://core.trac.wordpress.org/changeset/53575.
#35
@
2 years ago
- Keywords needs-docs added
- Resolution set to fixed
- Status changed from reopened to closed
@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.
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
2 years ago
#38
@
2 years ago
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
This ticket was mentioned in PR #3087 on WordPress/wordpress-develop by craigfrancis.
2 years ago
#39
Some feedback from Juliette (@jrfnl):
Trac ticket: https://core.trac.wordpress.org/ticket/52506
---
- 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). - 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? @access private
, you're right, it shouldn't be used on a method.- 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? - 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).
2 years ago
#40
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.
2 years ago
#41
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"_ ?
2 years ago
#42
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 ?
#43
@
2 years ago
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).
craigfrancis commented on PR #3087:
2 years ago
#44
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 :-)
craigfrancis commented on PR #3087:
2 years ago
#45
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?
#48
@
2 years ago
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).
This ticket was mentioned in Slack in #core by desrosj. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by jrf. View the logs.
2 years ago
This ticket was mentioned in PR #3541 on WordPress/wordpress-develop by @hellofromTonya.
2 years ago
#51
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
@Bernhard Reiter commented on PR #3541:
2 years ago
#52
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 commented on PR #3541:
2 years ago
#54
Committed via https://core.trac.wordpress.org/changeset/54733.
#56
@
2 years ago
- Keywords needs-patch added; has-patch commit has-dev-note removed
- Milestone changed from 6.1 to 6.2
- Resolution fixed deleted
- Status changed from closed to reopened
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.
This ticket was mentioned in PR #3724 on WordPress/wordpress-develop by craigfrancis.
2 years ago
#57
- Keywords has-patch added; needs-patch removed
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
craigfrancis commented on PR #3087:
2 years ago
#58
I've created PR 3724 to support the annoying "%%%s"
... and incorporated these changes with it.
#59
@
2 years ago
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.
@mukesh27 commented on PR #3724:
2 years ago
#60
@craigfrancis Is there any impact on performance for normal queries after these changes?
craigfrancis commented on PR #3724:
2 years ago
#61
@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.
craigfrancis commented on PR #3724:
2 years ago
#62
Thanks for the review @costdev, and yes, the more eyes on this the better.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
23 months ago
#64
@
23 months ago
- Keywords needs-testing needs-dev-note added
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.
#65
@
23 months ago
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.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
23 months ago
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
23 months ago
#68
@
23 months ago
Test Report
Patch tested: https://github.com/WordPress/wordpress-develop/pull/3724
Steps to Test
Test 1 - %i
placeholder functionality
- Check out the PR or apply the patch for this ticket, PR 3724.
- In your test site's
wp-content
directory, create amu-plugins
folder if it does not already exist. - In this new folder, create a new PHP file based on this test plugin gist.
- In a browser, open your test site's homepage (the test does not run in WP admin).
- The test plugin should render content similar to the image provided below.
- 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
- 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). - In a browser, open your test site's homepage (this test also does not run in WP admin).
- The test plugin should render a SQL query similar to the one provided below.
- 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 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
This ticket was mentioned in Slack in #core by craigfrancis. View the logs.
23 months ago
@davidbaumwald commented on PR #3724:
23 months ago
#71
Merged into core in https://core.trac.wordpress.org/changeset/55151.
#72
@
23 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
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.
This ticket was mentioned in Slack in #core by otto42. View the logs.
23 months ago
#75
@
23 months ago
- Resolution set to fixed
- Status changed from reopened to closed
Thanks @SergeyBiryukov!
#77
follow-up:
↓ 78
@
23 months ago
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.
#78
in reply to:
↑ 77
;
follow-up:
↓ 79
@
23 months ago
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 theinclude
for thewp-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
.
#79
in reply to:
↑ 78
@
22 months ago
Replying to SergeyBiryukov:
Replying to jrf:
HyperDB loads the
wpdb
class outside of WordPress core, socompat.php
was not loaded at all in this case.
Ah, that was the missing piece of the puzzle. 👍
#80
@
22 months ago
@craigfrancis, is this dev note still valid, or does it needs modifications?
#81
follow-up:
↓ 82
@
22 months ago
@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.
#82
in reply to:
↑ 81
;
follow-up:
↓ 83
@
22 months ago
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.
#83
in reply to:
↑ 82
@
22 months ago
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.
+1 to this
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'] ) );
+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 …"
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.