#53986 closed defect (bug) (fixed)
Problematic error-handling in sanitize_option()
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | 4.2.3 |
Component: | Options, Meta APIs | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description (last modified by )
Given a very unfortunate series of events, it's possible for a site to brick itself from a call to update_option()
on even a FE request that should have just resulted in no update (option value was not changing). Introduced in #32350.
What happens
- Error is thrown here due to a query failure: https://github.com/WordPress/WordPress/blob/270f2011f8ec7265c3f4ddce39c77ef5b496ed1c/wp-includes/wp-db.php#L2776
- Error is eventually returned here for say
category_base
: https://github.com/WordPress/WordPress/blob/b60a512de42c0a2ce2fa258e9be70bedb29b337c/wp-includes/formatting.php#L4853-L4855 - But this fails to catch the error: https://github.com/WordPress/WordPress/blob/b60a512de42c0a2ce2fa258e9be70bedb29b337c/wp-includes/formatting.php#L4890
Why you ask?
$value = new WP_Error( 'wpdb_get_table_charset_failure' ); $error = $value->get_error_message(); => string(0) ""
Replicating
Replicating the true process of how things go badly is not very easy, as you need certain queries to pass along the way and very specific ones to fail. But an easier replication can be done like so:
wp shell $option_value = new WP_Error( 'wpdb_get_table_charset_failure' ); update_option( 'category_base', $option_value );
Note that this will immediately brick the site. To cleanup, will need to delete the options from the database manually:
delete from wp_options where option_name = 'rewrite_rules'; delete from wp_options where option_name = 'category_base';
And if using object cache, need to clear that out as well. Comment out this line https://github.com/WordPress/WordPress/blob/94c655c89286fdf43e5bab82d4108fb679e46a7a/wp-includes/default-filters.php#L580, and wp cache flush
.
Resolution
Patch incoming, but defaulting the $error to null
in sanitize_option() and checking against that specifically will help future-proof this. And then to be more descriptive in the error handling, can also add error descriptions to WP_Error
's where needed.
Change History (14)
This ticket was mentioned in PR #1613 on WordPress/wordpress-develop by WPprodigy.
4 years ago
#1
- Keywords has-patch added
#2
@
4 years ago
- Description modified (diff)
- Keywords needs-unit-tests added
- Milestone changed from Awaiting Review to 5.9
Hi there, thanks for the ticket and the PR!
This looks good to me at a glance. Would be great to also add a unit test for this.
#3
@
4 years ago
Updated the PR w/ a test. Could do a more specific test that covers a more realistic result, but would need to mock $wpdb->strip_invalid_text_for_column()
as it can be the one responsible for returning the WP_Error that was previously breaking things: https://github.com/WordPress/wordpress-develop/blob/e83a341cc082864edf69257fded43d70d8a27685/src/wp-includes/formatting.php#L4873
The test I added does technically cover the needed code path though, so think we'll all good.
This ticket was mentioned in Slack in #core by icaleb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by icaleb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
#9
@
3 years ago
- Keywords needs-refresh added
As per today's bugscrub, I just wanted to note that in the proposed PR, the "Could not retrieve table charset." and "Could not strip invalid text." strings need to be properly prepared for internationalization.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
#12
@
3 years ago
- Version changed from trunk to 4.2.3
Changing to 4.2.3 as it was introduced in #32350.
SergeyBiryukov commented on PR #1613:
3 years ago
#14
Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/52294.
Trac ticket: https://core.trac.wordpress.org/ticket/53986
To prevent potential false-negatives, set
$error
tonull
initially - so we can better tell if it is ever changed during the sanitization (to be able to better react if an empty string is added to it).Additionally, and mainly for the sake of the settings api at this point, add error messages to the
WP_Error
's that were previously causing the issues here.