Make WordPress Core

Opened 8 weeks ago

Last modified 8 weeks ago

#53986 new defect (bug)

Problematic error-handling in sanitize_option()

Reported by: iCaleb Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Options, Meta APIs Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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

  1. Error is thrown here due to a query failure: https://github.com/WordPress/WordPress/blob/270f2011f8ec7265c3f4ddce39c77ef5b496ed1c/wp-includes/wp-db.php#L2776
  2. Error is eventually returned here for say category_base: https://github.com/WordPress/WordPress/blob/b60a512de42c0a2ce2fa258e9be70bedb29b337c/wp-includes/formatting.php#L4853-L4855
  3. 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 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.


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 (4)

This ticket was mentioned in PR #1613 on WordPress/wordpress-develop by WPprodigy.

8 weeks ago

  • Keywords has-patch added

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

To prevent potential false-negatives, set $error to null 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.

#2 @SergeyBiryukov
8 weeks 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 @iCaleb
8 weeks 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.

#4 @SergeyBiryukov
8 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed
Note: See TracTickets for help on using tickets.