[wp-trac] [WordPress Trac] #34392: calling update_option("siteurl", $newval) can overwrite it with an instance of WP_Error.

[WordPress Trac]

#34392: calling update_option("siteurl", $newval) can overwrite it with an instance
of WP_Error.
--------------------------+-----------------------------
 Reporter:  pbogdan       |      Owner:
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  General       |    Version:  4.3.1
 Severity:  normal        |   Keywords:
  Focuses:                |
--------------------------+-----------------------------
 Hello,

 under admittedly somewhat obscure circumstances a call to
 update_option("siteurl", ...) can overwrite its value with an instance of
 WP_Error class.

 When being updated `siteurl` is subject to some additional checks in
 `sanitize_option()` function, and the bug is due its handling of
 WP_Error's.

 In particular the code at
 https://github.com/WordPress/WordPress/blob/4.3.1/wp-
 includes/formatting.php#L3609-L3612:

 - overwrites the `$value` variable with result of the call to
   `$wpdb->strip_invalid_text_for_column()` -
 - will only create the `$error` variable if the WP_Error received from the
 call
   to `$wpdb->strip_invalid_text_for_column()` contains the optional error
   message

 If `$wpdb->strip_invalid_text_for_column()` returns an instance of
 WP_Error
 without an error message we end up with `$value` containing a WP_Error
 instance
 but empty `$error` variable.

 Then the check at
 https://github.com/WordPress/WordPress/blob/4.3.1/wp-
 includes/formatting.php#L3720
 fails due to empty `$error`, and the WP_Error `$value` propagates upwards
 as the
 return result of `sanitize_option()`, to be written into the database. As
 most
 places trust get_option("siteurl") to return a string rather than an
 object this
 results in fatal errors as WP_Error can't be converted to a string.

 In the specific case I observed, the WP_Error instance without an error
 message
 was originating from `wpdb::get_table_charset()` function -
 https://github.com/WordPress/WordPress/blob/4.3.1/wp-includes/wp-
 db.php#L2306-L2308. From
 what I gather `wpdb::strip_invalid_text_for_column()` needs to know the
 database
 charset for the database column to determine valid input, this will
 eventually
 call out to `wpdb::get_table_charset()` and if the query in that function
 fails
 for some reason the resulting WP_Error propagates all the way up to
 `sanitize_option()`.

 As this was tracked down retrospectively I don't know what exact
 conditions
 allowed the query in `wpdb::get_table_charset()` to fail while allowing
 any
 subsequent queries to succeed, however it can be reproduced with a test
 case at
 https://gist.github.com/pbogdan/9827a8f2358cf9e8ca71

 I have not reviewed the logic for other options handled by
 `sanitize_option()`
 but it's possible some of them could be affected if they use the same
 logic for
 dealing with WP_Error's - which relies on the WP_Error having the optional
 error
 message.

 I might be bit slow to respond but let me know if there is any additional
 information I can provide.


 Thanks,
 Piotr

--
Ticket URL: <https://core.trac.wordpress.org/ticket/34392>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform