[wp-trac] [WordPress Trac] #53986: Problematic error-handling in sanitize_option()

[WordPress Trac]

#53986: Problematic error-handling in sanitize_option()
----------------------------------------+---------------------
 Reporter:  iCaleb                      |       Owner:  (none)
     Type:  defect (bug)                |      Status:  new
 Priority:  normal                      |   Milestone:  5.9
Component:  Options, Meta APIs          |     Version:  trunk
 Severity:  normal                      |  Resolution:
 Keywords:  has-patch needs-unit-tests  |     Focuses:
----------------------------------------+---------------------
Changes (by SergeyBiryukov):

 * keywords:  has-patch => has-patch needs-unit-tests
 * milestone:  Awaiting Review => 5.9


Old description:

> 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 https://core.trac.wordpress.org/ticket/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.

New description:

 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.

--

Comment:

 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/53986#comment:2>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform