Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#53986 closed defect (bug) (fixed)

Problematic error-handling in sanitize_option()

Reported by: icaleb's profile iCaleb Owned by: sergeybiryukov's profile SergeyBiryukov
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 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

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.


3 years ago
#1

  • 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
3 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 @iCaleb
3 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.

#4 @SergeyBiryukov
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

This ticket was mentioned in Slack in #core by icaleb. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by icaleb. View the logs.


2 years ago

#7 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#9 @audrasjb
2 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.

#10 @audrasjb
2 years ago

  • Keywords needs-refresh removed

I updated the pull request accordingly.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#12 @hellofromTonya
2 years ago

  • Version changed from trunk to 4.2.3

Changing to 4.2.3 as it was introduced in #32350.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#13 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.