Cast (int) $cron->time - time() to prevent String - int error

[toddlahman]

On line 2802 and 2828 of wp-admin/includes/class-wp-site-health.php, $cron->time - time() is not automatically cast by PHP version 8.1.2 from the String $cron->time to an integer to perform the arithmetic. Forcing the cast with (int) $cron->time - time() corrects the critical error, detailed below, that prevented the site health dashboard from loading.

2023-04-28T16:27:37+00:00 CRITICAL Uncaught TypeError: Unsupported operand types: string - int in /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php:2802
Stack trace:
#0 /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php(1768): WP_Site_Health->has_missed_cron()
#1 /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php(194): WP_Site_Health->get_test_scheduled_events()
#2 /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php(139): WP_Site_Health->perform_test()
#3 /var/www/toddlahman.com/wp-includes/class-wp-hook.php(308): WP_Site_Health->enqueue_scripts()
#4 /var/www/toddlahman.com/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#5 /var/www/toddlahman.com/wp-includes/plugin.php(517): WP_Hook->do_action()
#6 /var/www/toddlahman.com/wp-admin/admin-header.php(118): do_action()
#7 /var/www/toddlahman.com/wp-admin/site-health.php(96): require_once('...')
#8 {main}
  thrown in /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php on line 2802

[toddlahman]

File with corrections.

On line 2802 and 2828 of wp-admin/includes/class-wp-site-health.php, $cron->time - time() is not automatically cast by PHP version 8.1.2 from the String $cron->time to an integer to perform the arithmetic. Forcing the cast with (int) $cron->time - time() corrects the critical error, detailed below, that prevented the site health dashboard from loading.

2023-04-28T16:27:37+00:00 CRITICAL Uncaught TypeError: Unsupported operand types: string - int in /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php:2802
Stack trace:
#0 /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php(1768): WP_Site_Health->has_missed_cron()
#1 /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php(194): WP_Site_Health->get_test_scheduled_events()
#2 /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php(139): WP_Site_Health->perform_test()
#3 /var/www/toddlahman.com/wp-includes/class-wp-hook.php(308): WP_Site_Health->enqueue_scripts()
#4 /var/www/toddlahman.com/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters()
#5 /var/www/toddlahman.com/wp-includes/plugin.php(517): WP_Hook->do_action()
#6 /var/www/toddlahman.com/wp-admin/admin-header.php(118): do_action()
#7 /var/www/toddlahman.com/wp-admin/site-health.php(96): require_once('...')
#8 {main}

thrown in /var/www/toddlahman.com/wp-admin/includes/class-wp-site-health.php on line 2802

Trac ticket: #58221

[toddlahman]

​https://github.com/WordPress/wordpress-develop/pull/4398

[jrf]

@toddlahman Thanks for reporting this. What I'd be interested in, is to see a var_dump() of the return value of _get_cron_array() and the value of $this->crons from an install where this error is happening.

The $this->crons[*]['time'] value should only ever be an integer and is set from the $timestamp which is used as a key in the array returned from _get_cron_array().

So, before thinking in solutions (or working around/hiding the error), I'd like to figure out how this error could happen in the first place.

[toddlahman]

Upon further investigation I found it was a plugin, or some other code, that directly altered the array stored in cron in the options table. I rarely use any plugins from anywhere that aren't written for WooCommerce or by seasoned developers. After modifications to the WP Control plugin I was able to visualize the issue ​https://www.dropbox.com/s/9j5d82crqiw6kum/Screen%20Shot%202023-04-29%20at%204.51.38%20PM.png?dl=0. However the cron var_dump using

_get_cron_array()

[ ​https://www.dropbox.com/s/ns8tcrhlkzvbm5k/cron_dump.txt?dl=0] revealed a bigger issue. On line 599 the top level array key is not an integer, but a string 'wp_batch_split_terms'. The cron API code does prevent anything other than an integer from being used as the epoch/unix timestamp, however by appending the cron array in the options table, a plugin, or some other code, was able to bypass the code safeguards. I was able to remove the entries with the following code:

		$cron = get_option( 'cron' );
		update_option( 'old_cron', $cron );
		unset( $cron['wp_batch_split_terms'] );
		update_option( 'cron', $cron );

I could not use a plugin to delete these entries because they typically rely on the cron API, as they should.

For the end user who never sees the cron entries, this is a critical issue, since their site will not behave as expected, such as the health report not being visible due to a fatal error. There is nothing in the code that validates the timestamp as an integer when the cron array is retrieved from the options table, and no other values from the cron array are validated for their data type. This led to fatal errors on my site. PHP itself is not type safe, and in this instance casting a String to an integer may lead to a zero value, which isn't much help. Cron is used routinely. I have set

define( 'DISABLE_WP_CRON', true );

, but most sites do not. Relying on code to prevent incorrect data types from being used is not sufficient because the data can be directly manipulated in the options database table. It would be better to create a custom cron database table that can enforce a BIGINT timestamp column, and other appropriate data types, to insure this type of fatal error cannot happen in the future. Migrating the current cron array of data would be beneficial to those who have incorrect data for cron entries currently that would not be allowed in the new cron table due to the column data type enforcement.

To fix this issue, my suggestion is to create a custom cron database table. This would have the side effect of making sites perform faster, in addition to insuring the data had the correct data types.

If no one is available to make this addition, I could submit this as a core contribution in the fall.

[jrf]

@toddlahman Thank you for getting back with your analysis and I'm glad to see it confirmed that this was not a WP Core issue as such.

I totally agree that the lack of input validation (in this case, lack of validation of input received from the database) is an issue. However, that is an issue throughout WP Core everywhere and probably needs a cohesive application wide approach and solution.

[SergeyBiryukov]

Thanks for the follow-up! This is likely related to [33646] & [33727] / #33423.

Due to accidentally reversed parameters passed to wp_schedule_single_event() in [33615], it was actually possible for the cron array to have a wp_batch_split_terms array key instead of an integer value. The issue was introduced in WordPress 4.3 and fixed in 4.3.1 via an upgrade routine which unschedules the invalid cron entries, though apparently they can still be found on some sites.

Validation of the $timestamp parameter was added a bit later in [33936] / #33475 for WordPress 4.4.