Problematic error-handling in sanitize_option()

[iCaleb]

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

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.

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.

[SergeyBiryukov]

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.

[iCaleb]

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.

[audrasjb]

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.

[audrasjb]

I updated the pull request accordingly.

[hellofromTonya]

Changing to 4.2.3 as it was introduced in #32350.

[SergeyBiryukov]

In 52294:

Options, Meta APIs: Improve error handling in sanitize_option().

To prevent potential false negatives, set $error to null initially, so we can better tell if it was ever changed during the sanitization and 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 some WP_Error objects returned from wpdb methods that were previously causing the issues here.

Follow-up to [32791].

Props iCaleb, audrasjb, hellofromTonya, SergeyBiryukov.
Fixes #53986.

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/52294.