WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 weeks ago

#34392 reopened defect (bug)

calling update_option("siteurl", $newval) can overwrite it with an instance of WP_Error.

Reported by: pbogdan Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.3.1
Component: General Keywords: bulk-reopened
Focuses: Cc:

Description

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

Attachments (1)

34392.diff (639 bytes) - added by MikeHansenMe 3 years ago.

Download all attachments as: .zip

Change History (5)

@MikeHansenMe
3 years ago

#1 @MikeHansenMe
3 years ago

  • Keywords has-patch added

#2 @MikeHansenMe
3 years ago

  • Keywords has-patch removed

Actually that won't work either because the $value still gets assigned the error and gets set in the option.

#3 @iseulde
4 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

This ticket has not seen any activity in over *two* years, so I'm closing it as "wontfix".

The ticket may lack decisiveness, may have become irrelevant, or may not have gathered enough interest.

If you think this ticket does deserve some attention again, feel free to reopen.

For bugs, it would be great if you could provide updated steps to reproduce against the latest version of WordPress (5.0.2 at the time of writing). Remember images or a video can be superior to explain a problem. At the very least, quickly test again to make sure the bug still exists.

If it’s an enhancement or feature, some extra motivation may help.

Thank you for your contributions to WordPress! <3

#4 @JeffPaul
8 weeks ago

  • Keywords bulk-reopened added
  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

A decision was made to reopen tickets that were closed in the bulk edit that this ticket was affected by. This ticket is being placed back into the Awaiting Review milestone so it can be individually evaluated and verified to determine if it is still relevant/valid or reproducible.

Note: See TracTickets for help on using tickets.