From f83f7a10238ce72e09194def1d196f665b8ca182 Mon Sep 17 00:00:00 2001 From: Vlad Olaru Date: Wed, 8 Jul 2026 11:34:03 +0300 Subject: [PATCH 01/16] fix: harden schedule deserialization against object injection When a scheduled action is read back, its schedule is stored as a native PHP-serialized blob and passed to unserialize() with no allowed_classes restriction. An attacker able to influence those stored bytes could make PHP instantiate an arbitrary class, running its __wakeup()/__destruct() magic methods (PHP object injection). The existing "is this an ActionScheduler_Schedule?" check in validate_schedule() runs only AFTER unserialize(), i.e. after the damage is done. Move that trust decision to before instantiation using a two-phase deserialization in the new ActionScheduler_ScheduleDeserializer: 1. Parse the blob with allowed_classes => false. This is inert: every object becomes a __PHP_Incomplete_Class placeholder, so no constructor, __wakeup(), __destruct() or autoloading runs. 2. Inspect the class names present. Only if the outermost class implements ActionScheduler_Schedule, and every nested class is on a small vetted allow-list (the bundled CronExpression family, filterable, plus any class implementing the schedule interface), re-run unserialize() restricted to exactly that verified set. The allow-list is derived, not declared: any loaded schedule class -- including third-party ones -- is trusted automatically, so extenders need to do nothing. This also means no backward-compatibility window: a schedule class that is not loaded already produced a broken action before this change, so rejecting it here regresses nothing that previously worked. Legacy blobs carrying the full CronExpression object graph continue to deserialize unchanged. Rejections reuse the existing corrupt-blob path (NullSchedule for canceled actions, ActionScheduler_InvalidActionException otherwise), so no new failure mode is introduced. An action_scheduler_unexpected_schedule_class hook fires for observability, and a shadow mode (action_scheduler_enforce_schedule_allowed_classes filter, default enforce) lets site owners report-only for a release before enforcing -- without touching any stored data. Both the default custom-table store (ActionScheduler_DBStore) and the legacy post store (ActionScheduler_wpPostStore, which reads the raw serialized meta to bypass the meta API's implicit unserialize) are covered. No storage-format change and no migration: existing rows are read exactly as before, only more safely. Refs #1318 Co-Authored-By: Claude Fable 5 --- .../ActionScheduler_ScheduleDeserializer.php | 271 ++++++++++++++++++ .../data-stores/ActionScheduler_DBStore.php | 2 +- .../ActionScheduler_wpPostStore.php | 37 ++- tests/bootstrap.php | 2 + tests/phpunit.xml.dist | 1 + .../ActionScheduler_Test_Custom_Schedule.php | 44 +++ .../ActionScheduler_Test_Evil_Gadget.php | 26 ++ ...tionScheduler_ScheduleUnserialize_Test.php | 243 ++++++++++++++++ 8 files changed, 624 insertions(+), 2 deletions(-) create mode 100644 classes/ActionScheduler_ScheduleDeserializer.php create mode 100644 tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule.php create mode 100644 tests/phpunit/helpers/ActionScheduler_Test_Evil_Gadget.php create mode 100644 tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php new file mode 100644 index 00000000..503b7f89 --- /dev/null +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -0,0 +1,271 @@ + false`, which instantiates nothing — every object + * in the payload becomes a harmless __PHP_Incomplete_Class placeholder. No constructor, + * __wakeup(), __destruct() or autoloading runs. + * 2. Inspect the class names actually present. Only if the outermost class implements + * ActionScheduler_Schedule, and every nested class is on a small vetted allow-list, do we + * re-run unserialize() a second time restricted to exactly that verified set of classes. + * + * The allow-list is derived, not hard-coded: any loaded schedule class (including third-party + * ones) is trusted automatically because it implements ActionScheduler_Schedule, so extenders need + * to do nothing. A blob referencing anything else is rejected the same way a corrupt blob already + * is today, or — in shadow mode — allowed through untouched while a warning is surfaced. + * + * @since 3.10.0 + */ +class ActionScheduler_ScheduleDeserializer { + + /** + * Deserialize a stored schedule blob without instantiating unexpected classes. + * + * @param string $data The raw serialized schedule blob as stored in the database. + * @return ActionScheduler_Schedule|object|false The schedule object, or false if the blob is + * unusable (corrupt, or rejected while enforcing). + * Callers already treat false like a corrupt blob. + */ + public static function unserialize( $data ) { + if ( ! is_string( $data ) || '' === $data ) { + return false; + } + + // Phase 1: parse the blob without instantiating any class from it. This is inert: objects + // become __PHP_Incomplete_Class placeholders, so no __wakeup()/__destruct()/autoload runs. + $inert = @unserialize( $data, array( 'allowed_classes' => false ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged + + // A schedule is always an object graph. Anything else is corrupt/unexpected data. + if ( ! is_object( $inert ) ) { + return false; + } + + $top_class = self::class_name_of( $inert ); + $nested_classes = array(); + self::collect_class_names( $inert, $nested_classes, true ); + + // Phase 2a: the outermost object must be a real, loaded schedule class. This is the same + // contract ActionScheduler_Store::validate_schedule() already enforces, only applied before + // instantiation instead of after. + if ( ! self::is_allowed_top_level_class( $top_class ) ) { + return self::handle_rejection( $data, $top_class, $top_class, $nested_classes ); + } + + // Phase 2b: every nested object must be on the vetted allow-list. + foreach ( $nested_classes as $nested_class ) { + if ( ! self::is_allowed_nested_class( $nested_class ) ) { + return self::handle_rejection( $data, $nested_class, $top_class, $nested_classes ); + } + } + + // All classes vetted: re-hydrate for real, restricting instantiation to exactly that set. + $allowed = array_values( array_unique( array_merge( array( $top_class ), $nested_classes ) ) ); + + return unserialize( $data, array( 'allowed_classes' => $allowed ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize + } + + /** + * Decide whether the outermost class in a schedule blob may be instantiated. + * + * Allowed iff the class is loaded and implements ActionScheduler_Schedule. A class that is not + * loaded cannot be validated (and, prior to this change, already produced a broken action), so + * rejecting it here does not regress any case that previously worked. + * + * @param string $class_name Class name discovered in the blob. + * @return bool + */ + protected static function is_allowed_top_level_class( $class_name ) { + if ( ! is_string( $class_name ) || '' === $class_name || ! class_exists( $class_name ) ) { + return false; + } + + $implements = class_implements( $class_name ); + + return is_array( $implements ) && isset( $implements['ActionScheduler_Schedule'] ); + } + + /** + * Decide whether a nested class (a property of the schedule) may be instantiated. + * + * The only classes Action Scheduler itself ever nests inside a schedule are the bundled + * CronExpression family. Composite schedules that nest another schedule are also fine. The + * default list is filterable so extenders can vet their own supporting classes. + * + * @param string $class_name Class name discovered nested in the blob. + * @return bool + */ + protected static function is_allowed_nested_class( $class_name ) { + if ( ! is_string( $class_name ) || '' === $class_name ) { + return false; + } + + if ( in_array( $class_name, self::get_allowed_nested_classes(), true ) ) { + return true; + } + + // A schedule nested inside another schedule (composite schedules) is safe by the same rule + // as the top-level check. + if ( class_exists( $class_name ) ) { + $implements = class_implements( $class_name ); + if ( is_array( $implements ) && isset( $implements['ActionScheduler_Schedule'] ) ) { + return true; + } + } + + return false; + } + + /** + * The supporting classes Action Scheduler is willing to instantiate when nested in a schedule. + * + * @return string[] + */ + public static function get_allowed_nested_classes() { + $default = array( + 'CronExpression', + 'CronExpression_FieldFactory', + 'CronExpression_AbstractField', + 'CronExpression_MinutesField', + 'CronExpression_HoursField', + 'CronExpression_DayOfMonthField', + 'CronExpression_MonthField', + 'CronExpression_DayOfWeekField', + 'CronExpression_YearField', + ); + + /** + * Filters the list of non-schedule classes that may be instantiated when nested inside a + * stored schedule blob during deserialization. + * + * Add the fully-qualified names of any supporting classes your custom schedule stores as + * properties. Schedule classes themselves (implementing ActionScheduler_Schedule) are + * always allowed and do not need to be listed here. + * + * @since 3.10.0 + * + * @param string[] $default List of allowed nested class names. + */ + return (array) apply_filters( 'action_scheduler_allowed_nested_schedule_classes', $default ); + } + + /** + * Whether an unexpected class causes the blob to be rejected (true) or merely reported while the + * legacy, unrestricted deserialization proceeds (false, "shadow mode"). + * + * Enforcement is on by default. Site owners can opt into shadow mode for a release to gather + * data before enforcing, without changing any stored data. + * + * @return bool + */ + public static function is_enforced() { + /** + * Filters whether Action Scheduler rejects schedule blobs referencing unexpected classes. + * + * Return false to run in "shadow mode": unexpected classes are reported via the + * `action_scheduler_unexpected_schedule_class` action but the blob is still deserialized + * exactly as it was before this hardening. Useful to confirm the allow-list is complete for + * your site before switching enforcement on. + * + * @since 3.10.0 + * + * @param bool $enforced Whether to reject blobs with unexpected classes. Default true. + */ + return (bool) apply_filters( 'action_scheduler_enforce_schedule_allowed_classes', true ); + } + + /** + * Handle a blob that references a class outside the vetted set. + * + * Always surfaces the event for observability. Under enforcement (the default) the blob is + * rejected by returning false, which callers already handle like a corrupt schedule. In shadow + * mode the pre-hardening behaviour is preserved so a mis-tuned allow-list cannot disrupt a site. + * + * @param string $data The raw serialized blob. + * @param string $offending_class The first class that failed validation. + * @param string $top_class The outermost class in the blob. + * @param string[] $nested_classes All nested class names discovered in the blob. + * @return object|false + */ + protected static function handle_rejection( $data, $offending_class, $top_class, array $nested_classes ) { + /** + * Fires when a stored schedule blob references a class Action Scheduler did not expect. + * + * @since 3.10.0 + * + * @param string $offending_class The first disallowed class encountered. + * @param string $top_class The outermost class in the blob. + * @param string[] $nested_classes All nested class names discovered in the blob. + * @param bool $enforced Whether the blob was rejected (true) or allowed through + * in shadow mode (false). + */ + do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $top_class, $nested_classes, self::is_enforced() ); + + if ( self::is_enforced() ) { + return false; + } + + // Shadow mode: behave exactly as Action Scheduler did before this change. + return @unserialize( $data ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged + } + + /** + * Get the class name of an object parsed in "inert" mode. + * + * Objects whose class was disallowed become __PHP_Incomplete_Class, which hides the original + * name behind get_class(); it lives in the __PHP_Incomplete_Class_Name pseudo-property instead. + * stdClass is never converted by PHP, so its real name is returned directly. + * + * @param object $maybe_incomplete Object parsed with allowed_classes => false. + * @return string The original class name, or '' if it cannot be determined. + */ + protected static function class_name_of( $maybe_incomplete ) { + if ( $maybe_incomplete instanceof __PHP_Incomplete_Class ) { + $vars = (array) $maybe_incomplete; + return isset( $vars['__PHP_Incomplete_Class_Name'] ) ? (string) $vars['__PHP_Incomplete_Class_Name'] : ''; + } + + return get_class( $maybe_incomplete ); + } + + /** + * Recursively collect the class names of every object nested inside a value. + * + * @param mixed $value The value to walk (object, array, or scalar). + * @param string[] $found Accumulator of discovered nested class names (by reference). + * @param bool $is_root Whether $value is the outermost object (excluded from $found). + * @return void + */ + protected static function collect_class_names( $value, array &$found, $is_root = false ) { + if ( is_object( $value ) ) { + $vars = (array) $value; + + if ( ! $is_root ) { + $found[] = self::class_name_of( $value ); + } + + foreach ( $vars as $key => $child ) { + if ( '__PHP_Incomplete_Class_Name' === $key ) { + continue; + } + self::collect_class_names( $child, $found, false ); + } + + return; + } + + if ( is_array( $value ) ) { + foreach ( $value as $child ) { + self::collect_class_names( $child, $found, false ); + } + } + } +} diff --git a/classes/data-stores/ActionScheduler_DBStore.php b/classes/data-stores/ActionScheduler_DBStore.php index 96da02af..41b9e50d 100644 --- a/classes/data-stores/ActionScheduler_DBStore.php +++ b/classes/data-stores/ActionScheduler_DBStore.php @@ -401,7 +401,7 @@ protected function make_action_from_db_record( $data ) { $this->validate_args( $args, $data->action_id ); } - $schedule = @unserialize( $data->schedule ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize WordPress.PHP.NoSilencingOperator + $schedule = ActionScheduler_ScheduleDeserializer::unserialize( $data->schedule ); if ( false === $schedule && $data->status === self::STATUS_CANCELED ) { // The handled corrupted action should appear in UI. $schedule = new ActionScheduler_NullSchedule(); diff --git a/classes/data-stores/ActionScheduler_wpPostStore.php b/classes/data-stores/ActionScheduler_wpPostStore.php index a72df963..300c5f8f 100644 --- a/classes/data-stores/ActionScheduler_wpPostStore.php +++ b/classes/data-stores/ActionScheduler_wpPostStore.php @@ -238,7 +238,7 @@ protected function make_action_from_post( $post ) { $args = json_decode( $post->post_content, true ); $this->validate_args( $args, $post->ID ); - $schedule = get_post_meta( $post->ID, self::SCHEDULE_META_KEY, true ); + $schedule = $this->get_schedule_from_post_meta( $post->ID ); $this->validate_schedule( $schedule, $post->ID ); $group = wp_get_object_terms( $post->ID, self::GROUP_TAXONOMY, array( 'fields' => 'names' ) ); @@ -247,6 +247,41 @@ protected function make_action_from_post( $post ) { return ActionScheduler::factory()->get_stored_action( $this->get_action_status_by_post_status( $post->post_status ), $hook, $args, $schedule, $group ); } + /** + * Read a scheduled action's schedule from post meta without instantiating unexpected classes. + * + * Using get_post_meta() would run the stored blob through maybe_unserialize(), instantiating + * whatever classes it names before we can vet them. Instead we read the raw serialized value and + * hand it to ActionScheduler_ScheduleDeserializer, which only instantiates a valid schedule (and its + * vetted supporting classes). @see https://github.com/woocommerce/action-scheduler/issues/1318 + * + * @param int $post_id Post ID of the scheduled action. + * @return ActionScheduler_Schedule|object|false The schedule, or false if unusable. + */ + protected function get_schedule_from_post_meta( $post_id ) { + global $wpdb; + + $raw = $wpdb->get_var( + $wpdb->prepare( + "SELECT meta_value FROM {$wpdb->postmeta} WHERE post_id = %d AND meta_key = %s ORDER BY meta_id ASC LIMIT 1", + $post_id, + self::SCHEDULE_META_KEY + ) + ); + + if ( null === $raw ) { + return false; + } + + // Schedule objects are stored serialized by the meta API. A non-serialized value cannot be a + // schedule object, so there is nothing to protect against — return it as-is for validation. + if ( ! is_serialized( $raw ) ) { + return $raw; + } + + return ActionScheduler_ScheduleDeserializer::unserialize( $raw ); + } + /** * Get action status by post status. * diff --git a/tests/bootstrap.php b/tests/bootstrap.php index ba6c31b7..b8daf758 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -32,6 +32,8 @@ } include_once 'phpunit/helpers/ActionScheduler_Callbacks.php'; +include_once 'phpunit/helpers/ActionScheduler_Test_Evil_Gadget.php'; +include_once 'phpunit/helpers/ActionScheduler_Test_Custom_Schedule.php'; include_once 'phpunit/ActionScheduler_Mocker.php'; include_once 'phpunit/ActionScheduler_Mock_Async_Request_QueueRunner.php'; include_once 'phpunit/jobstore/AbstractStoreTest.php'; diff --git a/tests/phpunit.xml.dist b/tests/phpunit.xml.dist index 65ee747b..721cbbf4 100644 --- a/tests/phpunit.xml.dist +++ b/tests/phpunit.xml.dist @@ -17,6 +17,7 @@ ./phpunit/jobstore/ActionScheduler_DBStoreMigrator_Test.php ./phpunit/jobstore/ActionScheduler_DBStore_Test.php + ./phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php ./phpunit/jobstore/ActionScheduler_HybridStore_Test.php ./phpunit/logging/ActionScheduler_DBLogger_Test.php diff --git a/tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule.php b/tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule.php new file mode 100644 index 00000000..3adb6ea8 --- /dev/null +++ b/tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule.php @@ -0,0 +1,44 @@ +timestamp = $timestamp; + } + + /** + * Get the next run date. + * + * @param null|DateTime $after Timestamp. + * @return DateTime|null + */ + public function next( ?DateTime $after = null ) { + return as_get_datetime_object( $this->timestamp ); + } + + /** + * Not recurring. + * + * @return bool + */ + public function is_recurring() { + return false; + } +} diff --git a/tests/phpunit/helpers/ActionScheduler_Test_Evil_Gadget.php b/tests/phpunit/helpers/ActionScheduler_Test_Evil_Gadget.php new file mode 100644 index 00000000..f9da0e4f --- /dev/null +++ b/tests/phpunit/helpers/ActionScheduler_Test_Evil_Gadget.php @@ -0,0 +1,26 @@ +query( "DELETE FROM {$wpdb->actionscheduler_actions}" ); + ActionScheduler_Test_Evil_Gadget::$fired = false; + parent::setUp(); + } + + /** + * Persist a real action, then overwrite its stored schedule blob with an arbitrary string, + * simulating an attacker (or corruption) tampering with the serialized column. + * + * @param string $schedule_blob Raw value to store in the schedule column. + * @return int Action ID. + */ + private function store_action_with_raw_schedule( $schedule_blob ) { + global $wpdb; + + $store = new ActionScheduler_DBStore(); + $action = new ActionScheduler_Action( + ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, + array(), + new ActionScheduler_SimpleSchedule( as_get_datetime_object() ) + ); + $action_id = $store->save_action( $action ); + + $wpdb->update( + $wpdb->actionscheduler_actions, + array( 'schedule' => $schedule_blob ), + array( 'action_id' => $action_id ), + array( '%s' ), + array( '%d' ) + ); + + return $action_id; + } + + /** + * THE VULNERABILITY. A tampered schedule blob referencing an arbitrary class must never + * cause that class to be instantiated during deserialization. + * + * On trunk this test is RED: unserialize() instantiates the gadget and runs its __wakeup(). + */ + public function test_arbitrary_class_is_not_instantiated_from_schedule_blob() { + $malicious = serialize( new ActionScheduler_Test_Evil_Gadget() ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $action_id = $this->store_action_with_raw_schedule( $malicious ); + + $store = new ActionScheduler_DBStore(); + + // Fetching may legitimately throw/return a null action for an unusable schedule; that is + // fine. What must never happen is the gadget's __wakeup() side effect firing. + try { + $store->fetch_action( $action_id ); + } catch ( Exception $e ) { + unset( $e ); // Rejecting the action is an acceptable outcome; we only assert on the side effect. + } + + $this->assertFalse( + ActionScheduler_Test_Evil_Gadget::$fired, + 'Deserializing a schedule blob instantiated an arbitrary class (object injection).' + ); + } + + /** + * A gadget nested INSIDE an otherwise-valid schedule wrapper must also be blocked. This guards + * against smuggling a gadget in via the recurrence/property of a legitimate schedule class. + */ + public function test_arbitrary_class_nested_in_valid_schedule_is_not_instantiated() { + $nul = chr( 0 ); + $prop_ts = $nul . '*' . $nul . 'scheduled_timestamp'; // Protected property marker. + $prop_rec = $nul . '*' . $nul . 'recurrence'; // Protected property marker. + $gadget = 'ActionScheduler_Test_Evil_Gadget'; + $container = 'ActionScheduler_IntervalSchedule'; + $blob = 'O:' . strlen( $container ) . ':"' . $container . '":2:{' + . 's:' . strlen( $prop_ts ) . ':"' . $prop_ts . '";i:' . ( time() + HOUR_IN_SECONDS ) . ';' + . 's:' . strlen( $prop_rec ) . ':"' . $prop_rec . '";' + . 'O:' . strlen( $gadget ) . ':"' . $gadget . '":0:{}' + . '}'; + $action_id = $this->store_action_with_raw_schedule( $blob ); + + $store = new ActionScheduler_DBStore(); + try { + $store->fetch_action( $action_id ); + } catch ( Exception $e ) { + unset( $e ); // Rejecting the action is an acceptable outcome; we only assert on the side effect. + } + + $this->assertFalse( + ActionScheduler_Test_Evil_Gadget::$fired, + 'A gadget nested inside a valid schedule wrapper was instantiated.' + ); + } + + /** + * The happy path must keep working: a simple schedule round-trips through the store unchanged. + */ + public function test_simple_schedule_round_trips() { + $time = as_get_datetime_object( '2026-03-04 05:06:07' ); + $store = new ActionScheduler_DBStore(); + $action = new ActionScheduler_Action( ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, array(), new ActionScheduler_SimpleSchedule( $time ) ); + $action_id = $store->save_action( $action ); + + $fetched = $store->fetch_action( $action_id ); + $schedule = $fetched->get_schedule(); + + $this->assertInstanceOf( 'ActionScheduler_SimpleSchedule', $schedule ); + $this->assertEquals( $time->getTimestamp(), $schedule->get_date()->getTimestamp() ); + } + + /** + * Cron schedules must round-trip, including their recurrence. + */ + public function test_cron_schedule_round_trips() { + $time = as_get_datetime_object( '2026-01-01 00:00:00' ); + $store = new ActionScheduler_DBStore(); + $action = new ActionScheduler_Action( ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, array(), new ActionScheduler_CronSchedule( $time, '0 0 * * *' ) ); + $action_id = $store->save_action( $action ); + + $fetched = $store->fetch_action( $action_id ); + $schedule = $fetched->get_schedule(); + + $this->assertInstanceOf( 'ActionScheduler_CronSchedule', $schedule ); + $this->assertEquals( '0 0 * * *', $schedule->get_recurrence() ); + } + + /** + * Existing rows serialized before this change still contain the full CronExpression object + * graph. Those must continue to deserialize into a working schedule. + */ + public function test_legacy_cron_blob_still_unserializes() { + $action_id = $this->store_action_with_raw_schedule( base64_decode( self::LEGACY_CRON_BLOB_B64 ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode + + $store = new ActionScheduler_DBStore(); + $fetched = $store->fetch_action( $action_id ); + $schedule = $fetched->get_schedule(); + + $this->assertInstanceOf( 'ActionScheduler_CronSchedule', $schedule ); + $this->assertEquals( '0 0 * * *', $schedule->get_recurrence() ); + } + + /** + * A third party schedule class (implementing ActionScheduler_Schedule, defined outside AS) + * must keep working without the vendor registering anything. + */ + public function test_third_party_schedule_class_round_trips() { + $timestamp = time() + DAY_IN_SECONDS; + $blob = serialize( new ActionScheduler_Test_Custom_Schedule( $timestamp ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $action_id = $this->store_action_with_raw_schedule( $blob ); + + $store = new ActionScheduler_DBStore(); + $fetched = $store->fetch_action( $action_id ); + $schedule = $fetched->get_schedule(); + + $this->assertInstanceOf( 'ActionScheduler_Test_Custom_Schedule', $schedule ); + $this->assertEquals( $timestamp, $schedule->next()->getTimestamp() ); + } + + /** + * The same protection must cover the legacy post-based store, whose schedule lives in post meta. + * + * On trunk this is RED: get_post_meta() runs the tampered blob through maybe_unserialize(). + */ + public function test_wp_post_store_does_not_instantiate_arbitrary_class() { + $store = new ActionScheduler_wpPostStore(); + $action = new ActionScheduler_Action( + ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, + array(), + new ActionScheduler_SimpleSchedule( as_get_datetime_object() ) + ); + $action_id = $store->save_action( $action ); + + // Tamper the stored schedule meta with a gadget blob, bypassing the meta API's serialization. + global $wpdb; + // phpcs:disable WordPress.DB.SlowDBQuery.slow_db_query_meta_value, WordPress.DB.SlowDBQuery.slow_db_query_meta_key, WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $wpdb->update( + $wpdb->postmeta, + array( 'meta_value' => serialize( new ActionScheduler_Test_Evil_Gadget() ) ), + array( + 'post_id' => $action_id, + 'meta_key' => ActionScheduler_wpPostStore::SCHEDULE_META_KEY, + ), + array( '%s' ), + array( '%d', '%s' ) + ); + // phpcs:enable WordPress.DB.SlowDBQuery.slow_db_query_meta_value, WordPress.DB.SlowDBQuery.slow_db_query_meta_key, WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + clean_post_cache( $action_id ); + wp_cache_delete( $action_id, 'post_meta' ); + + try { + $store->fetch_action( $action_id ); + } catch ( Exception $e ) { + unset( $e ); // Rejecting the action is an acceptable outcome; we only assert on the side effect. + } + + $this->assertFalse( + ActionScheduler_Test_Evil_Gadget::$fired, + 'The post-based store instantiated an arbitrary class from a schedule blob.' + ); + } + + /** + * The post-based store must still round-trip legitimate schedules after hardening. + */ + public function test_wp_post_store_round_trips_valid_schedule() { + $time = as_get_datetime_object( '2026-05-06 07:08:09' ); + $store = new ActionScheduler_wpPostStore(); + $action = new ActionScheduler_Action( ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, array(), new ActionScheduler_SimpleSchedule( $time ) ); + $action_id = $store->save_action( $action ); + + $fetched = $store->fetch_action( $action_id ); + $schedule = $fetched->get_schedule(); + + $this->assertInstanceOf( 'ActionScheduler_SimpleSchedule', $schedule ); + $this->assertEquals( $time->getTimestamp(), $schedule->get_date()->getTimestamp() ); + } +} From aeffbb0d73048935a422a71b7611c5139e42398e Mon Sep 17 00:00:00 2001 From: Vlad Olaru Date: Wed, 8 Jul 2026 15:19:07 +0300 Subject: [PATCH 02/16] fix: harden schedule deserializer per review feedback Two hardening fixes surfaced in code review of the two-phase deserializer: 1. Cyclic/deep object graphs (DoS). The class-name discovery walk recursed through the inert object graph with no cycle or depth guard. A tampered blob can use PHP's r:/R: references to encode a self-referential graph, sending the walk into unbounded recursion and exhausting the stack before the blob was ever rejected. We now track visited objects by spl_object_id (short-circuiting cycles and shared references) and cap recursion depth well above any real schedule (cron's CronExpression graph is the deepest at ~6 levels). A graph we cannot fully walk is treated as untrustworthy and rejected. 2. Log parity on re-hydration. The restricted phase-2 unserialize() was not silenced, whereas the stores historically used @unserialize() for schedule blobs. Restored the @ so corrupt data cannot start emitting warnings/notices into logs or output that it did not before this change. Adds a regression test covering the self-referential-blob case. Refs #1318 Co-Authored-By: Claude Fable 5 --- .../ActionScheduler_ScheduleDeserializer.php | 60 +++++++++++++++---- ...tionScheduler_ScheduleUnserialize_Test.php | 27 +++++++++ 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index 503b7f89..c4fc8be4 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -27,6 +27,16 @@ */ class ActionScheduler_ScheduleDeserializer { + /** + * Maximum object-graph depth we will walk while vetting a blob. + * + * Real schedules nest only a handful of levels deep (a cron schedule's CronExpression graph is + * the deepest at ~6). Anything beyond this bound is treated as untrustworthy rather than walked. + * + * @var int + */ + const MAX_GRAPH_DEPTH = 100; + /** * Deserialize a stored schedule blob without instantiating unexpected classes. * @@ -51,7 +61,13 @@ public static function unserialize( $data ) { $top_class = self::class_name_of( $inert ); $nested_classes = array(); - self::collect_class_names( $inert, $nested_classes, true ); + $seen = array(); + + // A tampered blob can encode a self-referential or pathologically deep object graph. If we + // cannot fully walk it within a sane bound, we refuse to vouch for it and reject. + if ( ! self::collect_class_names( $inert, $nested_classes, true, $seen, 0 ) ) { + return self::handle_rejection( $data, $top_class, $top_class, $nested_classes ); + } // Phase 2a: the outermost object must be a real, loaded schedule class. This is the same // contract ActionScheduler_Store::validate_schedule() already enforces, only applied before @@ -70,7 +86,9 @@ public static function unserialize( $data ) { // All classes vetted: re-hydrate for real, restricting instantiation to exactly that set. $allowed = array_values( array_unique( array_merge( array( $top_class ), $nested_classes ) ) ); - return unserialize( $data, array( 'allowed_classes' => $allowed ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize + // Silenced to match the stores' historical @unserialize() behaviour: a blob that parsed inert + // in phase 1 will parse here too, but we keep the same log-quiet contract regardless. + return @unserialize( $data, array( 'allowed_classes' => $allowed ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged } /** @@ -239,33 +257,53 @@ protected static function class_name_of( $maybe_incomplete ) { /** * Recursively collect the class names of every object nested inside a value. * - * @param mixed $value The value to walk (object, array, or scalar). - * @param string[] $found Accumulator of discovered nested class names (by reference). + * A tampered blob can decode (via `r:`/`R:` references) into a self-referential or extremely deep + * object graph. Walking that naively would recurse until the stack overflows, so we track objects + * already visited (to short-circuit cycles and shared references) and cap the recursion depth. + * + * @param mixed $value The value to walk (object, array, or scalar). + * @param string[] $found Accumulator of discovered nested class names (by reference). * @param bool $is_root Whether $value is the outermost object (excluded from $found). - * @return void + * @param array $seen Object ids already visited, keyed by spl_object_id (by reference). + * @param int $depth Current recursion depth. + * @return bool True if the value was fully walked; false if it was too deep to vet safely. */ - protected static function collect_class_names( $value, array &$found, $is_root = false ) { + protected static function collect_class_names( $value, array &$found, $is_root, array &$seen, $depth ) { + if ( $depth > self::MAX_GRAPH_DEPTH ) { + return false; + } + if ( is_object( $value ) ) { - $vars = (array) $value; + $object_id = spl_object_id( $value ); + if ( isset( $seen[ $object_id ] ) ) { + return true; // Already walked (cycle or shared reference); nothing new to collect. + } + $seen[ $object_id ] = true; if ( ! $is_root ) { $found[] = self::class_name_of( $value ); } - foreach ( $vars as $key => $child ) { + foreach ( (array) $value as $key => $child ) { if ( '__PHP_Incomplete_Class_Name' === $key ) { continue; } - self::collect_class_names( $child, $found, false ); + if ( ! self::collect_class_names( $child, $found, false, $seen, $depth + 1 ) ) { + return false; + } } - return; + return true; } if ( is_array( $value ) ) { foreach ( $value as $child ) { - self::collect_class_names( $child, $found, false ); + if ( ! self::collect_class_names( $child, $found, false, $seen, $depth + 1 ) ) { + return false; + } } } + + return true; } } diff --git a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php index e4f572a1..bafd2ad3 100644 --- a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php +++ b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php @@ -118,6 +118,33 @@ public function test_arbitrary_class_nested_in_valid_schedule_is_not_instantiate ); } + /** + * A self-referential blob must not send the class-name walk into infinite recursion. + * + * PHP's `r:` references let a tampered blob decode into a cyclic object graph. Before the cycle + * guard, walking it recursed until the stack overflowed (a DoS). The store must instead return. + */ + public function test_self_referential_blob_is_handled_without_infinite_recursion() { + $nul = chr( 0 ); + $prop_rec = $nul . '*' . $nul . 'recurrence'; // Protected property marker. + $container = 'ActionScheduler_IntervalSchedule'; + // The single property `recurrence` references value #1 — the object itself — forming a cycle. + $blob = 'O:' . strlen( $container ) . ':"' . $container . '":1:{' + . 's:' . strlen( $prop_rec ) . ':"' . $prop_rec . '";r:1;}'; + $action_id = $this->store_action_with_raw_schedule( $blob ); + + $store = new ActionScheduler_DBStore(); + $fetched = null; + try { + $fetched = $store->fetch_action( $action_id ); + } catch ( Exception $e ) { + unset( $e ); // Acceptable; the point is that we return rather than exhaust the stack. + } + + // Reaching this line at all proves the walk terminated. fetch_action always yields an object. + $this->assertNotNull( $fetched, 'A self-referential schedule blob did not resolve to an action.' ); + } + /** * The happy path must keep working: a simple schedule round-trips through the store unchanged. */ From d59f9ddb418cb19e22557f01429239dc6b4fe6a3 Mon Sep 17 00:00:00 2001 From: Vlad Olaru Date: Wed, 8 Jul 2026 16:58:50 +0300 Subject: [PATCH 03/16] fix: address second-round review feedback on deserializer Three small correctness/clarity fixes from code review: - Resolve is_enforced() once in handle_rejection() instead of calling it twice. A non-deterministic or expensive action_scheduler_enforce_schedule_allowed_classes filter could otherwise make the value reported to the observability action disagree with the branch actually taken. - Correct get_schedule_from_post_meta()'s documented return type to mixed: when the stored meta is not a serialized object it is returned as-is (a scalar) for validate_schedule() to reject, matching the prior get_post_meta() behaviour. - Annotate the intentional direct, uncached postmeta read with a DirectDatabaseQuery/NoCaching phpcs:ignore, matching this file's convention for its other direct queries and documenting why the meta cache is bypassed. Refs #1318 Co-Authored-By: Claude Fable 5 --- classes/ActionScheduler_ScheduleDeserializer.php | 8 ++++++-- classes/data-stores/ActionScheduler_wpPostStore.php | 6 +++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index c4fc8be4..907fcbdd 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -214,6 +214,10 @@ public static function is_enforced() { * @return object|false */ protected static function handle_rejection( $data, $offending_class, $top_class, array $nested_classes ) { + // Resolve enforcement once so the reported value and the branch taken cannot disagree, and the + // action_scheduler_enforce_schedule_allowed_classes filter runs a single time. + $enforced = self::is_enforced(); + /** * Fires when a stored schedule blob references a class Action Scheduler did not expect. * @@ -225,9 +229,9 @@ protected static function handle_rejection( $data, $offending_class, $top_class, * @param bool $enforced Whether the blob was rejected (true) or allowed through * in shadow mode (false). */ - do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $top_class, $nested_classes, self::is_enforced() ); + do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $top_class, $nested_classes, $enforced ); - if ( self::is_enforced() ) { + if ( $enforced ) { return false; } diff --git a/classes/data-stores/ActionScheduler_wpPostStore.php b/classes/data-stores/ActionScheduler_wpPostStore.php index 300c5f8f..8703713c 100644 --- a/classes/data-stores/ActionScheduler_wpPostStore.php +++ b/classes/data-stores/ActionScheduler_wpPostStore.php @@ -256,11 +256,15 @@ protected function make_action_from_post( $post ) { * vetted supporting classes). @see https://github.com/woocommerce/action-scheduler/issues/1318 * * @param int $post_id Post ID of the scheduled action. - * @return ActionScheduler_Schedule|object|false The schedule, or false if unusable. + * @return mixed The schedule object, false if unusable, or the raw meta value when it is not a + * serialized object (left for validate_schedule() to reject, as before). */ protected function get_schedule_from_post_meta( $post_id ) { global $wpdb; + // Direct, uncached query is intentional: we need the raw serialized value before the meta API + // runs it through maybe_unserialize(), so we bypass get_post_meta() and its cache here. + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching $raw = $wpdb->get_var( $wpdb->prepare( "SELECT meta_value FROM {$wpdb->postmeta} WHERE post_id = %d AND meta_key = %s ORDER BY meta_id ASC LIMIT 1", From 874e782755865622b6ad1017c5f1acd1a3fd8b48 Mon Sep 17 00:00:00 2001 From: Vlad Olaru Date: Wed, 8 Jul 2026 18:05:56 +0300 Subject: [PATCH 04/16] perf: resolve nested-class allow-list once per deserialization is_allowed_nested_class() called get_allowed_nested_classes() for every nested class, so the action_scheduler_allowed_nested_schedule_classes filter ran once per nested object on every schedule fetch -- around seven times for a cron schedule (CronExpression, its field factory and five field objects). The list is invariant across the loop. Resolve it once before the loop and pass it into is_allowed_nested_class(). The parameter is optional and defaults to resolving the list, so a standalone call is unaffected. Refs #1318 Co-Authored-By: Claude Fable 5 --- .../ActionScheduler_ScheduleDeserializer.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index 907fcbdd..42cf509e 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -76,9 +76,11 @@ public static function unserialize( $data ) { return self::handle_rejection( $data, $top_class, $top_class, $nested_classes ); } - // Phase 2b: every nested object must be on the vetted allow-list. + // Phase 2b: every nested object must be on the vetted allow-list. Resolve the list (and run its + // filter) once here rather than per nested class, since it does not vary across the loop. + $allowed_nested = self::get_allowed_nested_classes(); foreach ( $nested_classes as $nested_class ) { - if ( ! self::is_allowed_nested_class( $nested_class ) ) { + if ( ! self::is_allowed_nested_class( $nested_class, $allowed_nested ) ) { return self::handle_rejection( $data, $nested_class, $top_class, $nested_classes ); } } @@ -118,15 +120,21 @@ protected static function is_allowed_top_level_class( $class_name ) { * CronExpression family. Composite schedules that nest another schedule are also fine. The * default list is filterable so extenders can vet their own supporting classes. * - * @param string $class_name Class name discovered nested in the blob. + * @param string $class_name Class name discovered nested in the blob. + * @param string[]|null $allowed_nested Pre-resolved allow-list to reuse across a batch. When null, + * it is resolved (and its filter run) on this call. * @return bool */ - protected static function is_allowed_nested_class( $class_name ) { + protected static function is_allowed_nested_class( $class_name, ?array $allowed_nested = null ) { if ( ! is_string( $class_name ) || '' === $class_name ) { return false; } - if ( in_array( $class_name, self::get_allowed_nested_classes(), true ) ) { + if ( null === $allowed_nested ) { + $allowed_nested = self::get_allowed_nested_classes(); + } + + if ( in_array( $class_name, $allowed_nested, true ) ) { return true; } From 0db5a4479d505bf4a692c1d700c5284ef09ad454 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:58:40 -0700 Subject: [PATCH 05/16] perf: add single-pass fast path to schedule deserialization A blob referencing only Action Scheduler's own schedule classes and vetted support classes -- the common case -- is now hydrated in one restricted unserialize() instead of the inert-then-rehydrate two-phase walk. The two-phase path remains for third-party, filtered, or tampered blobs. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ActionScheduler_ScheduleDeserializer.php | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index 42cf509e..da29098d 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -23,6 +23,15 @@ * to do nothing. A blob referencing anything else is rejected the same way a corrupt blob already * is today, or — in shadow mode — allowed through untouched while a warning is surfaced. * + * As an optimization, deserialization first attempts a single restricted parse against the + * statically-known-safe set (Action Scheduler's own schedule classes plus the vetted support + * classes). When a blob references only those — the overwhelmingly common case — it is fully and + * safely hydrated in one pass and the two-phase walk above is skipped. Restricting instantiation to + * that set means nothing outside it can run, so if the result is a schedule with no placeholder left + * behind, every class named in the blob was already trusted. The inert two-phase path is used only + * for blobs that reference something outside the fast set: a third-party schedule class, a filtered + * support class, or a tampered/gadget blob. + * * @since 3.10.0 */ class ActionScheduler_ScheduleDeserializer { @@ -37,6 +46,23 @@ class ActionScheduler_ScheduleDeserializer { */ const MAX_GRAPH_DEPTH = 100; + /** + * Action Scheduler's own concrete schedule classes. + * + * Used to build the fast-path allow-list. This is a fixed, audited set of first-party classes; it + * is deliberately not filterable. Third-party schedule classes are handled by the two-phase path, + * which trusts any class implementing ActionScheduler_Schedule without needing to be listed here. + * + * @var string[] + */ + const KNOWN_SCHEDULE_CLASSES = array( + ActionScheduler_SimpleSchedule::class, + ActionScheduler_IntervalSchedule::class, + ActionScheduler_CronSchedule::class, + ActionScheduler_CanceledSchedule::class, + ActionScheduler_NullSchedule::class, + ); + /** * Deserialize a stored schedule blob without instantiating unexpected classes. * @@ -50,6 +76,21 @@ public static function unserialize( $data ) { return false; } + // Fast path: a single restricted parse against the statically-known-safe set. Instantiation is + // limited to Action Scheduler's own schedule classes plus the vetted support classes, so + // nothing outside that set can run — anything else becomes an inert __PHP_Incomplete_Class + // placeholder. If the result is a real schedule with no placeholder left anywhere in its graph, + // every class the blob named was already trusted, and this fully-hydrated object is exactly what + // the two-phase path below would have produced. This covers the overwhelmingly common case (a + // core schedule, no third-party classes) in one unserialize() instead of two. + $candidate = @unserialize( $data, array( 'allowed_classes' => self::get_fast_path_allowed_classes() ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged + if ( is_object( $candidate ) && self::is_allowed_top_level_class( get_class( $candidate ) ) ) { + $seen = array(); + if ( self::is_fully_hydrated( $candidate, $seen, 0 ) ) { + return $candidate; + } + } + // Phase 1: parse the blob without instantiating any class from it. This is inert: objects // become __PHP_Incomplete_Class placeholders, so no __wakeup()/__destruct()/autoload runs. $inert = @unserialize( $data, array( 'allowed_classes' => false ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged @@ -183,6 +224,70 @@ public static function get_allowed_nested_classes() { return (array) apply_filters( 'action_scheduler_allowed_nested_schedule_classes', $default ); } + /** + * The set of classes the fast path is willing to instantiate in its single restricted parse. + * + * Action Scheduler's own schedule classes plus the vetted nested support classes (the + * CronExpression family and any added via the action_scheduler_allowed_nested_schedule_classes + * filter). A blob referencing only these can be hydrated safely in one pass; anything else falls + * through to the two-phase path. + * + * @return string[] + */ + protected static function get_fast_path_allowed_classes() { + return array_merge( self::KNOWN_SCHEDULE_CLASSES, self::get_allowed_nested_classes() ); + } + + /** + * Whether a graph parsed by the fast path was hydrated entirely from trusted classes. + * + * True only if the value contains no __PHP_Incomplete_Class placeholder anywhere — the marker the + * fast path's restricted unserialize() leaves behind for a class outside its allow-list — and the + * whole graph could be walked within MAX_GRAPH_DEPTH. A placeholder means the blob named an + * untrusted class; an over-deep (or, guarded here, cyclic) graph means we cannot vouch for it. In + * either case we return false so the caller falls back to the two-phase vetting path. + * + * @param mixed $value The value to walk (object, array, or scalar). + * @param array $seen Object ids already visited, keyed by spl_object_id (by reference). + * @param int $depth Current recursion depth. + * @return bool + */ + protected static function is_fully_hydrated( $value, array &$seen, $depth ) { + if ( $depth > self::MAX_GRAPH_DEPTH ) { + return false; + } + + if ( is_object( $value ) ) { + if ( $value instanceof __PHP_Incomplete_Class ) { + return false; // A class the blob named was outside the fast-path set. + } + + $object_id = spl_object_id( $value ); + if ( isset( $seen[ $object_id ] ) ) { + return true; // Already walked (cycle or shared reference); nothing new to check. + } + $seen[ $object_id ] = true; + + foreach ( (array) $value as $child ) { + if ( ! self::is_fully_hydrated( $child, $seen, $depth + 1 ) ) { + return false; + } + } + + return true; + } + + if ( is_array( $value ) ) { + foreach ( $value as $child ) { + if ( ! self::is_fully_hydrated( $child, $seen, $depth + 1 ) ) { + return false; + } + } + } + + return true; + } + /** * Whether an unexpected class causes the blob to be rejected (true) or merely reported while the * legacy, unrestricted deserialization proceeds (false, "shadow mode"). From ea33c4ecb7019357f35bd6144a0d28cbdf3141e4 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:58:48 -0700 Subject: [PATCH 06/16] fix: allow built-in date/time value objects nested in schedules Adds DateTime, DateTimeImmutable, DateTimeZone and DateInterval to the default nested allow-list so third-party schedules that store one as a property keep deserializing. They define no exploitable magic-method sink. Also switches both allow-lists to ::class references. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ActionScheduler_ScheduleDeserializer.php | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index da29098d..cddfe765 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -198,15 +198,26 @@ protected static function is_allowed_nested_class( $class_name, ?array $allowed_ */ public static function get_allowed_nested_classes() { $default = array( - 'CronExpression', - 'CronExpression_FieldFactory', - 'CronExpression_AbstractField', - 'CronExpression_MinutesField', - 'CronExpression_HoursField', - 'CronExpression_DayOfMonthField', - 'CronExpression_MonthField', - 'CronExpression_DayOfWeekField', - 'CronExpression_YearField', + // The bundled CronExpression family, which ActionScheduler_CronSchedule nests. + CronExpression::class, + CronExpression_FieldFactory::class, + CronExpression_AbstractField::class, + CronExpression_MinutesField::class, + CronExpression_HoursField::class, + CronExpression_DayOfMonthField::class, + CronExpression_MonthField::class, + CronExpression_DayOfWeekField::class, + CronExpression_YearField::class, + + // Built-in date/time value objects. Action Scheduler's own schedules store dates as an + // int timestamp (see the schedules' __sleep()), so these are here for third-party schedule + // classes that keep a date/duration object as a property. They are safe to instantiate: + // none define a __destruct(), and their __wakeup() only restores internal state from + // scalars — there is no attacker-reachable side-effect sink to exploit. + DateTime::class, + DateTimeImmutable::class, + DateTimeZone::class, + DateInterval::class, ); /** From 758cbbded7eb2e9013489fa0e3bb3680fb66bdc3 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:58:57 -0700 Subject: [PATCH 07/16] fix: block top-level non-schedule blobs even in shadow mode Shadow (report-only) mode routed every rejection through an unrestricted unserialize(), so it re-instantiated a bare top-level gadget -- the exact issue #1318 PoC. Rejections are now split: a top-level non-schedule or an unwalkable graph is always blocked, and only an unexpected nested support class under an otherwise-valid schedule may pass through in shadow mode. The observability action now reports the outcome actually taken. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ActionScheduler_ScheduleDeserializer.php | 50 +++++++++++++------ 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index cddfe765..fad085a8 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -105,24 +105,29 @@ public static function unserialize( $data ) { $seen = array(); // A tampered blob can encode a self-referential or pathologically deep object graph. If we - // cannot fully walk it within a sane bound, we refuse to vouch for it and reject. + // cannot fully walk it within a sane bound, we refuse to vouch for it and reject. This is a + // structural failure, never eligible for shadow-mode passthrough: re-parsing an unwalkable + // graph unrestricted would revive the very DoS/injection risk we are guarding against. if ( ! self::collect_class_names( $inert, $nested_classes, true, $seen, 0 ) ) { - return self::handle_rejection( $data, $top_class, $top_class, $nested_classes ); + return self::handle_rejection( $data, $top_class, $top_class, $nested_classes, false ); } // Phase 2a: the outermost object must be a real, loaded schedule class. This is the same // contract ActionScheduler_Store::validate_schedule() already enforces, only applied before - // instantiation instead of after. + // instantiation instead of after. A top-level non-schedule (the issue #1318 PoC — a bare + // gadget) has no legitimate form, so it is blocked even in shadow mode: not shadow-eligible. if ( ! self::is_allowed_top_level_class( $top_class ) ) { - return self::handle_rejection( $data, $top_class, $top_class, $nested_classes ); + return self::handle_rejection( $data, $top_class, $top_class, $nested_classes, false ); } // Phase 2b: every nested object must be on the vetted allow-list. Resolve the list (and run its // filter) once here rather than per nested class, since it does not vary across the loop. + // Reaching here means the top-level object is a valid schedule; only an unexpected *nested* + // support class can fail now, which is the one case shadow mode may let through (eligible). $allowed_nested = self::get_allowed_nested_classes(); foreach ( $nested_classes as $nested_class ) { if ( ! self::is_allowed_nested_class( $nested_class, $allowed_nested ) ) { - return self::handle_rejection( $data, $nested_class, $top_class, $nested_classes ); + return self::handle_rejection( $data, $nested_class, $top_class, $nested_classes, true ); } } @@ -328,19 +333,28 @@ public static function is_enforced() { * Handle a blob that references a class outside the vetted set. * * Always surfaces the event for observability. Under enforcement (the default) the blob is - * rejected by returning false, which callers already handle like a corrupt schedule. In shadow - * mode the pre-hardening behaviour is preserved so a mis-tuned allow-list cannot disrupt a site. + * rejected by returning false, which callers already handle like a corrupt schedule. + * + * Shadow mode ("report only") relaxes this, but only for a $shadow_eligible rejection — one where + * the outermost object is a valid schedule and merely a *nested* support class was unexpected. + * That is the sole case where a mis-tuned allow-list could disrupt an otherwise-legitimate action, + * so it is the only case shadow mode lets through. Structural rejections ($shadow_eligible false) — + * a top-level non-schedule (the issue #1318 PoC) or an unwalkable object graph — have no legitimate + * form and are always rejected, even in shadow mode, so report-only can never be downgraded into + * reviving the vulnerability. * * @param string $data The raw serialized blob. * @param string $offending_class The first class that failed validation. * @param string $top_class The outermost class in the blob. * @param string[] $nested_classes All nested class names discovered in the blob. + * @param bool $shadow_eligible Whether shadow mode may let this particular rejection through. * @return object|false */ - protected static function handle_rejection( $data, $offending_class, $top_class, array $nested_classes ) { - // Resolve enforcement once so the reported value and the branch taken cannot disagree, and the - // action_scheduler_enforce_schedule_allowed_classes filter runs a single time. - $enforced = self::is_enforced(); + protected static function handle_rejection( $data, $offending_class, $top_class, array $nested_classes, $shadow_eligible ) { + // A rejection stands unless we are in shadow mode AND this rejection is eligible for report-only + // passthrough. Resolve it once so the reported outcome and the branch taken cannot disagree, and + // the action_scheduler_enforce_schedule_allowed_classes filter runs a single time. + $rejected = self::is_enforced() || ! $shadow_eligible; /** * Fires when a stored schedule blob references a class Action Scheduler did not expect. @@ -350,16 +364,20 @@ protected static function handle_rejection( $data, $offending_class, $top_class, * @param string $offending_class The first disallowed class encountered. * @param string $top_class The outermost class in the blob. * @param string[] $nested_classes All nested class names discovered in the blob. - * @param bool $enforced Whether the blob was rejected (true) or allowed through - * in shadow mode (false). + * @param bool $rejected Whether the blob was rejected (true) or allowed through in + * shadow mode (false). Always true for a structural rejection + * (top-level non-schedule or unwalkable graph), which shadow + * mode cannot relax. */ - do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $top_class, $nested_classes, $enforced ); + do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $top_class, $nested_classes, $rejected ); - if ( $enforced ) { + if ( $rejected ) { return false; } - // Shadow mode: behave exactly as Action Scheduler did before this change. + // Shadow mode, nested-only rejection: behave as Action Scheduler did before this change. This + // is reached only when the top-level object is a valid schedule, so the unrestricted parse can + // no longer instantiate a bare top-level gadget. return @unserialize( $data ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged } From 1a57497de2e35fda75a2dc939352a12345bea404 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:16:33 -0700 Subject: [PATCH 08/16] refactor: rework schedule deserializer as a single-walk instance Replaces the inert-then-rehydrate two-phase parse with a single restricted unserialize(): classes outside the trusted set become __PHP_Incomplete_Class placeholders that double as the list of unexpected classes to vet, so one walk of the graph yields both the hydrated object and everything still needing a trust decision. The separate all-inert parse and its walk are gone. A static unserialize() facade drives a short-lived instance -- the constructor takes the allow-lists, __invoke takes the blob, and per-blob walk state is reset on each invocation. The nested allow-list is resolved through its filter once per call (never process-cached) so a late- or context-registered filter is honoured. The top-level schedule gate runs before the walk, so a bare top-level gadget is rejected without walking a hostile graph. Marked @internal. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ActionScheduler_ScheduleDeserializer.php | 567 ++++++++---------- 1 file changed, 255 insertions(+), 312 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index fad085a8..d2f9d12b 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -1,41 +1,15 @@ false`, which instantiates nothing — every object - * in the payload becomes a harmless __PHP_Incomplete_Class placeholder. No constructor, - * __wakeup(), __destruct() or autoloading runs. - * 2. Inspect the class names actually present. Only if the outermost class implements - * ActionScheduler_Schedule, and every nested class is on a small vetted allow-list, do we - * re-run unserialize() a second time restricted to exactly that verified set of classes. + * Used to safely deserialize schedule information when retrieving a stored scheduled action. * - * The allow-list is derived, not hard-coded: any loaded schedule class (including third-party - * ones) is trusted automatically because it implements ActionScheduler_Schedule, so extenders need - * to do nothing. A blob referencing anything else is rejected the same way a corrupt blob already - * is today, or — in shadow mode — allowed through untouched while a warning is surfaced. + * This helps to guard against deserialization attacks, while maintaining backwards compatibility + * with older versions of Action Scheduler. * - * As an optimization, deserialization first attempts a single restricted parse against the - * statically-known-safe set (Action Scheduler's own schedule classes plus the vetted support - * classes). When a blob references only those — the overwhelmingly common case — it is fully and - * safely hydrated in one pass and the two-phase walk above is skipped. Restricting instantiation to - * that set means nothing outside it can run, so if the result is a schedule with no placeholder left - * behind, every class named in the blob was already trusted. The inert two-phase path is used only - * for blobs that reference something outside the fast set: a third-party schedule class, a filtered - * support class, or a tampered/gadget blob. - * - * @since 3.10.0 + * @internal This implementation is subject to change without notice or back-compat measures. + * @see https://github.com/woocommerce/action-scheduler/issues/1318 + * @since 4.1.0 */ class ActionScheduler_ScheduleDeserializer { - /** * Maximum object-graph depth we will walk while vetting a blob. * @@ -44,18 +18,15 @@ class ActionScheduler_ScheduleDeserializer { * * @var int */ - const MAX_GRAPH_DEPTH = 100; + private const MAX_GRAPH_DEPTH = 100; /** - * Action Scheduler's own concrete schedule classes. - * - * Used to build the fast-path allow-list. This is a fixed, audited set of first-party classes; it - * is deliberately not filterable. Third-party schedule classes are handled by the two-phase path, - * which trusts any class implementing ActionScheduler_Schedule without needing to be listed here. + * Action Scheduler's own concrete schedule classes. Along with any class that implements + * ActionScheduler_Schedule, these are always trusted. * * @var string[] */ - const KNOWN_SCHEDULE_CLASSES = array( + private const KNOWN_SCHEDULE_CLASSES = array( ActionScheduler_SimpleSchedule::class, ActionScheduler_IntervalSchedule::class, ActionScheduler_CronSchedule::class, @@ -64,228 +35,202 @@ class ActionScheduler_ScheduleDeserializer { ); /** - * Deserialize a stored schedule blob without instantiating unexpected classes. + * The default supporting classes Action Scheduler is willing to instantiate when nested in a schedule. * - * @param string $data The raw serialized schedule blob as stored in the database. - * @return ActionScheduler_Schedule|object|false The schedule object, or false if the blob is - * unusable (corrupt, or rejected while enforcing). - * Callers already treat false like a corrupt blob. + * @var string[] */ - public static function unserialize( $data ) { - if ( ! is_string( $data ) || '' === $data ) { - return false; - } - - // Fast path: a single restricted parse against the statically-known-safe set. Instantiation is - // limited to Action Scheduler's own schedule classes plus the vetted support classes, so - // nothing outside that set can run — anything else becomes an inert __PHP_Incomplete_Class - // placeholder. If the result is a real schedule with no placeholder left anywhere in its graph, - // every class the blob named was already trusted, and this fully-hydrated object is exactly what - // the two-phase path below would have produced. This covers the overwhelmingly common case (a - // core schedule, no third-party classes) in one unserialize() instead of two. - $candidate = @unserialize( $data, array( 'allowed_classes' => self::get_fast_path_allowed_classes() ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged - if ( is_object( $candidate ) && self::is_allowed_top_level_class( get_class( $candidate ) ) ) { - $seen = array(); - if ( self::is_fully_hydrated( $candidate, $seen, 0 ) ) { - return $candidate; - } - } + private const DEFAULT_NESTED_CLASSES = array( + // The bundled CronExpression family, which ActionScheduler_CronSchedule nests. + CronExpression::class, + CronExpression_FieldFactory::class, + CronExpression_AbstractField::class, + CronExpression_MinutesField::class, + CronExpression_HoursField::class, + CronExpression_DayOfMonthField::class, + CronExpression_MonthField::class, + CronExpression_DayOfWeekField::class, + CronExpression_YearField::class, + + // Additional trusted classes, likely to be used in schedule representations. + DateTime::class, + DateTimeImmutable::class, + DateTimeZone::class, + DateInterval::class, + ); - // Phase 1: parse the blob without instantiating any class from it. This is inert: objects - // become __PHP_Incomplete_Class placeholders, so no __wakeup()/__destruct()/autoload runs. - $inert = @unserialize( $data, array( 'allowed_classes' => false ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged + /** + * The raw serialized schedule blob being deserialized. + * + * @var string + */ + protected $source_data; - // A schedule is always an object graph. Anything else is corrupt/unexpected data. - if ( ! is_object( $inert ) ) { - return false; - } + /** + * The class name of the outermost object in the blob. + * + * @var string + */ + protected $outer_class; - $top_class = self::class_name_of( $inert ); - $nested_classes = array(); - $seen = array(); + /** + * List of classes that are known to be acceptable outer classes. + * + * @var string[] + */ + private $allowed_scheduler_classes; - // A tampered blob can encode a self-referential or pathologically deep object graph. If we - // cannot fully walk it within a sane bound, we refuse to vouch for it and reject. This is a - // structural failure, never eligible for shadow-mode passthrough: re-parsing an unwalkable - // graph unrestricted would revive the very DoS/injection risk we are guarding against. - if ( ! self::collect_class_names( $inert, $nested_classes, true, $seen, 0 ) ) { - return self::handle_rejection( $data, $top_class, $top_class, $nested_classes, false ); - } + /** + * Any nested objects must be instances of one of these classes. + * + * @var string[] + */ + private $allowed_nested_classes; - // Phase 2a: the outermost object must be a real, loaded schedule class. This is the same - // contract ActionScheduler_Store::validate_schedule() already enforces, only applied before - // instantiation instead of after. A top-level non-schedule (the issue #1318 PoC — a bare - // gadget) has no legitimate form, so it is blocked even in shadow mode: not shadow-eligible. - if ( ! self::is_allowed_top_level_class( $top_class ) ) { - return self::handle_rejection( $data, $top_class, $top_class, $nested_classes, false ); - } + /** + * Object IDs already visited during the walk, keyed by spl_object_id, to short-circuit cycles + * and shared references. + * + * @var array + */ + private $seen = array(); - // Phase 2b: every nested object must be on the vetted allow-list. Resolve the list (and run its - // filter) once here rather than per nested class, since it does not vary across the loop. - // Reaching here means the top-level object is a valid schedule; only an unexpected *nested* - // support class can fail now, which is the one case shadow mode may let through (eligible). - $allowed_nested = self::get_allowed_nested_classes(); - foreach ( $nested_classes as $nested_class ) { - if ( ! self::is_allowed_nested_class( $nested_class, $allowed_nested ) ) { - return self::handle_rejection( $data, $nested_class, $top_class, $nested_classes, true ); - } - } + /** + * Unexpected/disallowed class names discovered nested in the blob. + * + * @var string[] + */ + private $potential_offenders; - // All classes vetted: re-hydrate for real, restricting instantiation to exactly that set. - $allowed = array_values( array_unique( array_merge( array( $top_class ), $nested_classes ) ) ); + /** + * Safely deserialize a stored schedule blob without instantiating unexpected classes. + * + * @param mixed $blob The raw serialized schedule blob as stored in the database. + * + * @return ActionScheduler_Schedule|false + */ + public static function unserialize( $blob ) { + /** + * Filters the list of classes that are permitted as nested properties of a schedule object. + * + * The result of this filter is not memoized: this is deliberate, so that it can be changed + * if the context (including the current blog, in a multisite network) alters during the + * same request. + * + * @since 4.1.0 + * + * @param string[] $default List of allowed nested class names. + */ + $allowed_nested = (array) apply_filters( 'action_scheduler_allowed_nested_schedule_classes', self::DEFAULT_NESTED_CLASSES ); + $deserializer = new self( self::KNOWN_SCHEDULE_CLASSES, $allowed_nested ); + return $deserializer( $blob ); + } - // Silenced to match the stores' historical @unserialize() behaviour: a blob that parsed inert - // in phase 1 will parse here too, but we keep the same log-quiet contract regardless. - return @unserialize( $data, array( 'allowed_classes' => $allowed ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged + /** + * Build our ActionScheduler_ScheduleDeserializer instance. + * + * @param array $allowed_scheduler_classes + * @param array $allowed_nested_classes + */ + public function __construct( array $allowed_scheduler_classes, array $allowed_nested_classes ) { + $this->allowed_scheduler_classes = $allowed_scheduler_classes; + $this->allowed_nested_classes = $allowed_nested_classes; } /** - * Decide whether the outermost class in a schedule blob may be instantiated. + * Deserialize a blob of serialized data. * - * Allowed iff the class is loaded and implements ActionScheduler_Schedule. A class that is not - * loaded cannot be validated (and, prior to this change, already produced a broken action), so - * rejecting it here does not regress any case that previously worked. + * @param mixed $serialized_blob Data to unserialize. Expected to be a string. * - * @param string $class_name Class name discovered in the blob. - * @return bool + * @return ActionScheduler_Schedule|false */ - protected static function is_allowed_top_level_class( $class_name ) { - if ( ! is_string( $class_name ) || '' === $class_name || ! class_exists( $class_name ) ) { + public function __invoke( $serialized_blob ) { + if ( ! is_string( $serialized_blob ) || '' === $serialized_blob ) { return false; } - $implements = class_implements( $class_name ); + $this->seen = array(); + $this->potential_offenders = array(); + $this->source_data = $serialized_blob; - return is_array( $implements ) && isset( $implements['ActionScheduler_Schedule'] ); - } + // Initial attempt to unserialize, specifying all trusted classes: for default actions using + // supplied schedule classes, this should succeed without any problems. Otherwise, the result + // may contain one or more __PHP_Incomplete_Class references that we will need to examine. + $all_trusted_classes = array_merge( $this->allowed_scheduler_classes, $this->allowed_nested_classes ); + $candidate = @unserialize( $this->source_data, array( 'allowed_classes' => $all_trusted_classes ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged - /** - * Decide whether a nested class (a property of the schedule) may be instantiated. - * - * The only classes Action Scheduler itself ever nests inside a schedule are the bundled - * CronExpression family. Composite schedules that nest another schedule are also fine. The - * default list is filterable so extenders can vet their own supporting classes. - * - * @param string $class_name Class name discovered nested in the blob. - * @param string[]|null $allowed_nested Pre-resolved allow-list to reuse across a batch. When null, - * it is resolved (and its filter run) on this call. - * @return bool - */ - protected static function is_allowed_nested_class( $class_name, ?array $allowed_nested = null ) { - if ( ! is_string( $class_name ) || '' === $class_name ) { + // A schedule is always an object graph. + if ( ! is_object( $candidate ) ) { return false; } - if ( null === $allowed_nested ) { - $allowed_nested = self::get_allowed_nested_classes(); + $this->outer_class = $this->class_name_of( $candidate ); + + // Verify that we have a valid schedule representation, scanning for disallowed nested classes. + if ( ! $this->is_schedule_implementation( $this->outer_class ) || ! $this->scan_object_graph( $candidate, true, 0 ) ) { + return $this->reject( $this->outer_class, false ); } - if ( in_array( $class_name, $allowed_nested, true ) ) { - return true; + // If the candidate is an acceptable schedule class, and doesn't contain any potential offenders, it is safe. + if ( ! $candidate instanceof __PHP_Incomplete_Class && empty( $this->potential_offenders ) ) { + return $candidate; } - // A schedule nested inside another schedule (composite schedules) is safe by the same rule - // as the top-level check. - if ( class_exists( $class_name ) ) { - $implements = class_implements( $class_name ); - if ( is_array( $implements ) && isset( $implements['ActionScheduler_Schedule'] ) ) { - return true; + // Otherwise the top-level object is itself a valid schedule, but references one or more disallowed classes. + foreach ($this->potential_offenders as $offender ) { + if ( ! $this->is_allowed_nested_class( $offender, $this->allowed_nested_classes ) ) { + return $this->reject( $offender, true ); } } - return false; - } - - /** - * The supporting classes Action Scheduler is willing to instantiate when nested in a schedule. - * - * @return string[] - */ - public static function get_allowed_nested_classes() { - $default = array( - // The bundled CronExpression family, which ActionScheduler_CronSchedule nests. - CronExpression::class, - CronExpression_FieldFactory::class, - CronExpression_AbstractField::class, - CronExpression_MinutesField::class, - CronExpression_HoursField::class, - CronExpression_DayOfMonthField::class, - CronExpression_MonthField::class, - CronExpression_DayOfWeekField::class, - CronExpression_YearField::class, - - // Built-in date/time value objects. Action Scheduler's own schedules store dates as an - // int timestamp (see the schedules' __sleep()), so these are here for third-party schedule - // classes that keep a date/duration object as a property. They are safe to instantiate: - // none define a __destruct(), and their __wakeup() only restores internal state from - // scalars — there is no attacker-reachable side-effect sink to exploit. - DateTime::class, - DateTimeImmutable::class, - DateTimeZone::class, - DateInterval::class, + // If we reach this point, any potential offenders must in fact be safe. + $allowed = array_values( + array_unique( + array_merge( $all_trusted_classes, array( $this->outer_class ), $this->potential_offenders ) + ) ); - /** - * Filters the list of non-schedule classes that may be instantiated when nested inside a - * stored schedule blob during deserialization. - * - * Add the fully-qualified names of any supporting classes your custom schedule stores as - * properties. Schedule classes themselves (implementing ActionScheduler_Schedule) are - * always allowed and do not need to be listed here. - * - * @since 3.10.0 - * - * @param string[] $default List of allowed nested class names. - */ - return (array) apply_filters( 'action_scheduler_allowed_nested_schedule_classes', $default ); + // Re-run unserialize. + return @unserialize( $this->source_data, array( 'allowed_classes' => $allowed ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged } /** - * The set of classes the fast path is willing to instantiate in its single restricted parse. + * Walk the parsed graph once, recording unexpected (placeholder) classes and guarding traversal. * - * Action Scheduler's own schedule classes plus the vetted nested support classes (the - * CronExpression family and any added via the action_scheduler_allowed_nested_schedule_classes - * filter). A blob referencing only these can be hydrated safely in one pass; anything else falls - * through to the two-phase path. + * Objects whose class was outside the safe set are __PHP_Incomplete_Class placeholders; their + * original names are collected into $this->potential_offenders (the root is excluded — it is tracked + * separately as $this->outer_class). Real objects are trusted and only recursed into so a placeholder + * nested inside one is still found. A tampered blob can decode (via `r:`/`R:` references) into a + * self-referential or extremely deep graph, so visited objects are tracked to short-circuit cycles + * and shared references, and recursion depth is capped. * - * @return string[] - */ - protected static function get_fast_path_allowed_classes() { - return array_merge( self::KNOWN_SCHEDULE_CLASSES, self::get_allowed_nested_classes() ); - } - - /** - * Whether a graph parsed by the fast path was hydrated entirely from trusted classes. - * - * True only if the value contains no __PHP_Incomplete_Class placeholder anywhere — the marker the - * fast path's restricted unserialize() leaves behind for a class outside its allow-list — and the - * whole graph could be walked within MAX_GRAPH_DEPTH. A placeholder means the blob named an - * untrusted class; an over-deep (or, guarded here, cyclic) graph means we cannot vouch for it. In - * either case we return false so the caller falls back to the two-phase vetting path. + * @param mixed $value The value to walk (object, array, or scalar). + * @param bool $is_root Whether $value is the outermost object (excluded from $this->potential_offenders). + * @param int $depth Current recursion depth. * - * @param mixed $value The value to walk (object, array, or scalar). - * @param array $seen Object ids already visited, keyed by spl_object_id (by reference). - * @param int $depth Current recursion depth. - * @return bool + * @return bool True if the value was fully walked; false if it was too deep to vet safely. */ - protected static function is_fully_hydrated( $value, array &$seen, $depth ) { + private function scan_object_graph( $value, bool $is_root, int $depth ): bool { if ( $depth > self::MAX_GRAPH_DEPTH ) { return false; } if ( is_object( $value ) ) { - if ( $value instanceof __PHP_Incomplete_Class ) { - return false; // A class the blob named was outside the fast-path set. + $object_id = spl_object_id( $value ); + if ( isset( $this->seen[ $object_id ] ) ) { + return true; // Already walked (cycle or shared reference); nothing new to collect. } + $this->seen[ $object_id ] = true; - $object_id = spl_object_id( $value ); - if ( isset( $seen[ $object_id ] ) ) { - return true; // Already walked (cycle or shared reference); nothing new to check. + $is_placeholder = $value instanceof __PHP_Incomplete_Class; + if ( $is_placeholder && ! $is_root ) { + $this->potential_offenders[] = $this->class_name_of( $value ); } - $seen[ $object_id ] = true; - foreach ( (array) $value as $child ) { - if ( ! self::is_fully_hydrated( $child, $seen, $depth + 1 ) ) { + foreach ( (array) $value as $key => $child ) { + if ( $is_placeholder && '__PHP_Incomplete_Class_Name' === $key ) { + continue; + } + if ( ! $this->scan_object_graph( $child, false, $depth + 1 ) ) { return false; } } @@ -295,7 +240,7 @@ protected static function is_fully_hydrated( $value, array &$seen, $depth ) { if ( is_array( $value ) ) { foreach ( $value as $child ) { - if ( ! self::is_fully_hydrated( $child, $seen, $depth + 1 ) ) { + if ( ! $this->scan_object_graph( $child, false, $depth + 1 ) ) { return false; } } @@ -304,31 +249,6 @@ protected static function is_fully_hydrated( $value, array &$seen, $depth ) { return true; } - /** - * Whether an unexpected class causes the blob to be rejected (true) or merely reported while the - * legacy, unrestricted deserialization proceeds (false, "shadow mode"). - * - * Enforcement is on by default. Site owners can opt into shadow mode for a release to gather - * data before enforcing, without changing any stored data. - * - * @return bool - */ - public static function is_enforced() { - /** - * Filters whether Action Scheduler rejects schedule blobs referencing unexpected classes. - * - * Return false to run in "shadow mode": unexpected classes are reported via the - * `action_scheduler_unexpected_schedule_class` action but the blob is still deserialized - * exactly as it was before this hardening. Useful to confirm the allow-list is complete for - * your site before switching enforcement on. - * - * @since 3.10.0 - * - * @param bool $enforced Whether to reject blobs with unexpected classes. Default true. - */ - return (bool) apply_filters( 'action_scheduler_enforce_schedule_allowed_classes', true ); - } - /** * Handle a blob that references a class outside the vetted set. * @@ -336,21 +256,20 @@ public static function is_enforced() { * rejected by returning false, which callers already handle like a corrupt schedule. * * Shadow mode ("report only") relaxes this, but only for a $shadow_eligible rejection — one where - * the outermost object is a valid schedule and merely a *nested* support class was unexpected. - * That is the sole case where a mis-tuned allow-list could disrupt an otherwise-legitimate action, - * so it is the only case shadow mode lets through. Structural rejections ($shadow_eligible false) — - * a top-level non-schedule (the issue #1318 PoC) or an unwalkable object graph — have no legitimate - * form and are always rejected, even in shadow mode, so report-only can never be downgraded into - * reviving the vulnerability. + * the outermost object is a valid schedule and merely a *nested* class was unexpected. That is the + * sole case where a mis-tuned allow-list could disrupt an otherwise-legitimate action, so it is the + * only case shadow mode lets through. Structural rejections ($shadow_eligible false) — a top-level + * non-schedule (the issue #1318 PoC) or an unwalkable object graph — have no legitimate form and are + * always rejected, even in shadow mode, so report-only can never be downgraded into reviving the + * vulnerability. * - * @param string $data The raw serialized blob. - * @param string $offending_class The first class that failed validation. - * @param string $top_class The outermost class in the blob. - * @param string[] $nested_classes All nested class names discovered in the blob. - * @param bool $shadow_eligible Whether shadow mode may let this particular rejection through. - * @return object|false + * @param string $offending_class The class that triggered the rejection. + * @param bool $shadow_eligible Whether shadow mode may let this particular rejection through. + * @return ActionScheduler_Schedule|false False when rejected. A shadow-mode passthrough is only + * reached for a nested rejection, i.e. after the top-level + * schedule gate passed, so any object returned is a schedule. */ - protected static function handle_rejection( $data, $offending_class, $top_class, array $nested_classes, $shadow_eligible ) { + protected function reject( $offending_class, $shadow_eligible ) { // A rejection stands unless we are in shadow mode AND this rejection is eligible for report-only // passthrough. Resolve it once so the reported outcome and the branch taken cannot disagree, and // the action_scheduler_enforce_schedule_allowed_classes filter runs a single time. @@ -359,97 +278,121 @@ protected static function handle_rejection( $data, $offending_class, $top_class, /** * Fires when a stored schedule blob references a class Action Scheduler did not expect. * - * @since 3.10.0 + * @since 4.1.0 * - * @param string $offending_class The first disallowed class encountered. - * @param string $top_class The outermost class in the blob. - * @param string[] $nested_classes All nested class names discovered in the blob. - * @param bool $rejected Whether the blob was rejected (true) or allowed through in - * shadow mode (false). Always true for a structural rejection - * (top-level non-schedule or unwalkable graph), which shadow - * mode cannot relax. + * @param string $offending_class The class that triggered the rejection. + * @param string $outer_class The outermost class in the blob. + * @param string[] $unexpected_classes Every class the blob names that is outside the safe set. + * @param bool $rejected Whether the blob was rejected (true) or allowed through in + * shadow mode (false). Always true for a structural rejection + * (top-level non-schedule or unwalkable graph), which shadow + * mode cannot relax. */ - do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $top_class, $nested_classes, $rejected ); + do_action( 'action_scheduler_unexpected_schedule_class', $offending_class, $this->outer_class, $this->potential_offenders, $rejected ); if ( $rejected ) { return false; } - // Shadow mode, nested-only rejection: behave as Action Scheduler did before this change. This - // is reached only when the top-level object is a valid schedule, so the unrestricted parse can - // no longer instantiate a bare top-level gadget. - return @unserialize( $data ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged + // Shadow mode, nested-only rejection: behave as Action Scheduler did before this change. This is + // reached only when the top-level object is a valid schedule, so the unrestricted parse can no + // longer instantiate a bare top-level gadget. + return @unserialize( $this->source_data ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize, WordPress.PHP.NoSilencedErrors.Discouraged } /** - * Get the class name of an object parsed in "inert" mode. + * Decide whether the outermost class in a schedule blob may be instantiated. * - * Objects whose class was disallowed become __PHP_Incomplete_Class, which hides the original - * name behind get_class(); it lives in the __PHP_Incomplete_Class_Name pseudo-property instead. - * stdClass is never converted by PHP, so its real name is returned directly. + * Allowed if the class is loaded and implements ActionScheduler_Schedule. A class that is not + * loaded cannot be validated (and, prior to this change, already produced a broken action), so + * rejecting it here does not regress any case that previously worked. * - * @param object $maybe_incomplete Object parsed with allowed_classes => false. - * @return string The original class name, or '' if it cannot be determined. + * @param string $class_name Class name discovered in the blob. + * @return bool */ - protected static function class_name_of( $maybe_incomplete ) { - if ( $maybe_incomplete instanceof __PHP_Incomplete_Class ) { - $vars = (array) $maybe_incomplete; - return isset( $vars['__PHP_Incomplete_Class_Name'] ) ? (string) $vars['__PHP_Incomplete_Class_Name'] : ''; + private function is_schedule_implementation( string $class_name ): bool { + if ( '' === $class_name || ! class_exists( $class_name ) ) { + return false; } - return get_class( $maybe_incomplete ); + $implements = class_implements( $class_name ); + return is_array( $implements ) && isset( $implements['ActionScheduler_Schedule'] ); } /** - * Recursively collect the class names of every object nested inside a value. + * Decide whether a nested class (a property of the schedule) may be instantiated. * - * A tampered blob can decode (via `r:`/`R:` references) into a self-referential or extremely deep - * object graph. Walking that naively would recurse until the stack overflows, so we track objects - * already visited (to short-circuit cycles and shared references) and cap the recursion depth. + * The only classes Action Scheduler itself ever nests inside a schedule are the bundled + * CronExpression family. Composite schedules that nest another schedule are also fine. The + * default list is filterable so extenders can vet their own supporting classes. * - * @param mixed $value The value to walk (object, array, or scalar). - * @param string[] $found Accumulator of discovered nested class names (by reference). - * @param bool $is_root Whether $value is the outermost object (excluded from $found). - * @param array $seen Object ids already visited, keyed by spl_object_id (by reference). - * @param int $depth Current recursion depth. - * @return bool True if the value was fully walked; false if it was too deep to vet safely. + * @param string $class_name Class name discovered nested in the blob. + * @param string[] $allowed_nested The resolved nested allow-list to check against. + * + * @return bool */ - protected static function collect_class_names( $value, array &$found, $is_root, array &$seen, $depth ) { - if ( $depth > self::MAX_GRAPH_DEPTH ) { + private function is_allowed_nested_class( string $class_name, array $allowed_nested ): bool { + if ( '' === $class_name ) { return false; } - if ( is_object( $value ) ) { - $object_id = spl_object_id( $value ); - if ( isset( $seen[ $object_id ] ) ) { - return true; // Already walked (cycle or shared reference); nothing new to collect. - } - $seen[ $object_id ] = true; + if ( in_array( $class_name, $allowed_nested, true ) ) { + return true; + } - if ( ! $is_root ) { - $found[] = self::class_name_of( $value ); + // A schedule nested inside another schedule (composite schedules) is safe by the same rule + // as the top-level check. + if ( class_exists( $class_name ) ) { + $implements = class_implements( $class_name ); + if ( is_array( $implements ) && isset( $implements['ActionScheduler_Schedule'] ) ) { + return true; } + } - foreach ( (array) $value as $key => $child ) { - if ( '__PHP_Incomplete_Class_Name' === $key ) { - continue; - } - if ( ! self::collect_class_names( $child, $found, false, $seen, $depth + 1 ) ) { - return false; - } - } + return false; + } - return true; + /** + * Get the class name of an object parsed with a restricted allowed_classes set. + * + * Objects whose class was disallowed become __PHP_Incomplete_Class, which hides the original + * name behind get_class(); it lives in the __PHP_Incomplete_Class_Name pseudo-property instead. + * stdClass is never converted by PHP, so its real name is returned directly. + * + * @param object $maybe_incomplete Object parsed with a restricted allowed_classes set. + * @return string The original class name, or '' if it cannot be determined. + */ + private function class_name_of( object $maybe_incomplete ): string { + if ( $maybe_incomplete instanceof __PHP_Incomplete_Class ) { + $vars = (array) $maybe_incomplete; + return isset( $vars['__PHP_Incomplete_Class_Name'] ) ? (string) $vars['__PHP_Incomplete_Class_Name'] : ''; } - if ( is_array( $value ) ) { - foreach ( $value as $child ) { - if ( ! self::collect_class_names( $child, $found, false, $seen, $depth + 1 ) ) { - return false; - } - } - } + return get_class( $maybe_incomplete ); + } - return true; + /** + * Whether an unexpected class causes the blob to be rejected (true) or merely reported while the + * legacy, unrestricted deserialization proceeds (false, "shadow mode"). + * + * Enforcement is on by default. Site owners can opt into shadow mode for a release to gather + * data before enforcing, without changing any stored data. + * + * @return bool + */ + public static function is_enforced(): bool { + /** + * Filters whether Action Scheduler rejects schedule blobs referencing unexpected classes. + * + * Return false to run in "shadow mode": unexpected classes are reported via the + * `action_scheduler_unexpected_schedule_class` action but the blob is still deserialized + * exactly as it was before this hardening. Useful to confirm the allow-list is complete for + * your site before switching enforcement on. + * + * @since 4.1.0 + * + * @param bool $enforced Whether to reject blobs with unexpected classes. Default true. + */ + return (bool) apply_filters( 'action_scheduler_enforce_schedule_allowed_classes', true ); } } From 12238fdfb2bd966b6f74267fb793907be3e53ba1 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:16:33 -0700 Subject: [PATCH 09/16] test: cover schedule deserializer hardening behaviours Adds coverage for the paths the hardening introduced: shadow mode still blocks a top-level gadget but lets an unexpected nested class through; the observability action fires on rejection; the nested allow-list filter is honoured per call; a third-party schedule nesting built-in date/time objects round-trips; an overly deep graph is rejected; a reused deserializer instance does not leak state; the injected nested allow-list is authoritative; and a non-serialized post-store meta value is treated as an invalid action. Adds two third-party schedule fixtures. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/bootstrap.php | 2 + ...ler_Test_Custom_Schedule_With_Property.php | 72 ++++++ .../ActionScheduler_Test_Schedule_Helper.php | 29 +++ ...tionScheduler_ScheduleUnserialize_Test.php | 242 ++++++++++++++++++ 4 files changed, 345 insertions(+) create mode 100644 tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule_With_Property.php create mode 100644 tests/phpunit/helpers/ActionScheduler_Test_Schedule_Helper.php diff --git a/tests/bootstrap.php b/tests/bootstrap.php index b8daf758..df33ffac 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -34,6 +34,8 @@ include_once 'phpunit/helpers/ActionScheduler_Callbacks.php'; include_once 'phpunit/helpers/ActionScheduler_Test_Evil_Gadget.php'; include_once 'phpunit/helpers/ActionScheduler_Test_Custom_Schedule.php'; +include_once 'phpunit/helpers/ActionScheduler_Test_Schedule_Helper.php'; +include_once 'phpunit/helpers/ActionScheduler_Test_Custom_Schedule_With_Property.php'; include_once 'phpunit/ActionScheduler_Mocker.php'; include_once 'phpunit/ActionScheduler_Mock_Async_Request_QueueRunner.php'; include_once 'phpunit/jobstore/AbstractStoreTest.php'; diff --git a/tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule_With_Property.php b/tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule_With_Property.php new file mode 100644 index 00000000..0fbc4702 --- /dev/null +++ b/tests/phpunit/helpers/ActionScheduler_Test_Custom_Schedule_With_Property.php @@ -0,0 +1,72 @@ +at = new DateTime( '@' . $timestamp ); + $this->created = new DateTimeImmutable( '@' . $timestamp ); + $this->zone = new DateTimeZone( 'UTC' ); + $this->every = new DateInterval( 'P1D' ); + $this->helper = $helper; + } + + /** + * Get the next run date. + * + * @param null|DateTime $after Timestamp. + * @return DateTime|null + */ + public function next( ?DateTime $after = null ) { + return as_get_datetime_object( $this->at->getTimestamp() ); + } + + /** + * Recurring. + * + * @return bool + */ + public function is_recurring() { + return true; + } +} diff --git a/tests/phpunit/helpers/ActionScheduler_Test_Schedule_Helper.php b/tests/phpunit/helpers/ActionScheduler_Test_Schedule_Helper.php new file mode 100644 index 00000000..12127dd5 --- /dev/null +++ b/tests/phpunit/helpers/ActionScheduler_Test_Schedule_Helper.php @@ -0,0 +1,29 @@ +value = $value; + } +} diff --git a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php index bafd2ad3..2b122bc3 100644 --- a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php +++ b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php @@ -267,4 +267,246 @@ public function test_wp_post_store_round_trips_valid_schedule() { $this->assertInstanceOf( 'ActionScheduler_SimpleSchedule', $schedule ); $this->assertEquals( $time->getTimestamp(), $schedule->get_date()->getTimestamp() ); } + + public function tearDown(): void { + // These tests register filters/actions on the deserializer's hooks; clear them so nothing leaks + // into later tests running in the same process. + remove_all_filters( 'action_scheduler_allowed_nested_schedule_classes' ); + remove_all_filters( 'action_scheduler_enforce_schedule_allowed_classes' ); + remove_all_actions( 'action_scheduler_unexpected_schedule_class' ); + remove_all_actions( 'action_scheduler_failed_fetch_action' ); + parent::tearDown(); + } + + /** + * A serialized blob for a valid schedule (ActionScheduler_IntervalSchedule) that smuggles a gadget + * in as its (protected) recurrence property. Hand-crafted because __sleep() would otherwise drop a + * non-whitelisted property. + * + * @return string + */ + private function nested_gadget_blob() { + $nul = chr( 0 ); + $prop_ts = $nul . '*' . $nul . 'scheduled_timestamp'; // Protected property marker. + $prop_rec = $nul . '*' . $nul . 'recurrence'; // Protected property marker. + $gadget = 'ActionScheduler_Test_Evil_Gadget'; + $container = 'ActionScheduler_IntervalSchedule'; + + return 'O:' . strlen( $container ) . ':"' . $container . '":2:{' + . 's:' . strlen( $prop_ts ) . ':"' . $prop_ts . '";i:' . ( time() + HOUR_IN_SECONDS ) . ';' + . 's:' . strlen( $prop_rec ) . ':"' . $prop_rec . '";' + . 'O:' . strlen( $gadget ) . ':"' . $gadget . '":0:{}' + . '}'; + } + + /** + * Shadow mode ("report only") must NEVER let a bare top-level gadget be instantiated. Only an + * unexpected *nested* class inside an otherwise-valid schedule may pass through in shadow mode. + */ + public function test_shadow_mode_still_blocks_top_level_gadget() { + add_filter( 'action_scheduler_enforce_schedule_allowed_classes', '__return_false' ); + + $blob = serialize( new ActionScheduler_Test_Evil_Gadget() ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $result = ActionScheduler_ScheduleDeserializer::unserialize( $blob ); + + $this->assertFalse( $result, 'A top-level gadget was not rejected in shadow mode.' ); + $this->assertFalse( + ActionScheduler_Test_Evil_Gadget::$fired, + 'A top-level gadget was instantiated in shadow mode.' + ); + } + + /** + * Shadow mode DOES let an unexpected nested class through: this is its documented purpose (keep an + * otherwise-legitimate action working while an allow-list is being tuned). The tradeoff — and the + * reason enforcement is the default — is that the nested class is instantiated. + */ + public function test_shadow_mode_allows_unexpected_nested_class_through() { + add_filter( 'action_scheduler_enforce_schedule_allowed_classes', '__return_false' ); + + $result = ActionScheduler_ScheduleDeserializer::unserialize( $this->nested_gadget_blob() ); + + $this->assertInstanceOf( + 'ActionScheduler_IntervalSchedule', + $result, + 'Shadow mode did not pass a valid schedule with an unexpected nested class through.' + ); + $this->assertTrue( + ActionScheduler_Test_Evil_Gadget::$fired, + 'Shadow mode is report-only for nested classes, so the nested class is expected to run.' + ); + } + + /** + * A rejection must surface the action_scheduler_unexpected_schedule_class action for observability, + * reporting the offending class, the outer class, and that the blob was rejected. + */ + public function test_unexpected_class_action_fires_on_rejection() { + $captured = array(); + add_action( + 'action_scheduler_unexpected_schedule_class', + function ( $offending, $outer, $unexpected, $rejected ) use ( &$captured ) { + $captured = compact( 'offending', 'outer', 'unexpected', 'rejected' ); + }, + 10, + 4 + ); + + $blob = serialize( new ActionScheduler_Test_Evil_Gadget() ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $result = ActionScheduler_ScheduleDeserializer::unserialize( $blob ); + + $this->assertFalse( $result ); + $this->assertSame( 'ActionScheduler_Test_Evil_Gadget', $captured['offending'] ); + $this->assertSame( 'ActionScheduler_Test_Evil_Gadget', $captured['outer'] ); + $this->assertTrue( $captured['rejected'] ); + } + + /** + * The action_scheduler_allowed_nested_schedule_classes filter must be honored, and — because it is + * resolved per call rather than cached for the process — a class allow-listed only for the second + * call is rejected on the first and accepted on the second. + */ + public function test_nested_allow_list_filter_is_honored_per_call() { + $blob = serialize( new ActionScheduler_Test_Custom_Schedule_With_Property( time() + DAY_IN_SECONDS, new ActionScheduler_Test_Schedule_Helper( 42 ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + + // First call, no filter: the helper class is unexpected, so the blob is rejected. + $this->assertFalse( + ActionScheduler_ScheduleDeserializer::unserialize( $blob ), + 'An un-allow-listed nested support class should be rejected by default.' + ); + + // Second call, with the helper allow-listed via the filter: the blob is accepted. This also + // proves the first, filter-less call did not cache a stale allow-list for the process. + add_filter( + 'action_scheduler_allowed_nested_schedule_classes', + function ( $classes ) { + $classes[] = 'ActionScheduler_Test_Schedule_Helper'; + return $classes; + } + ); + + $this->assertInstanceOf( + 'ActionScheduler_Test_Custom_Schedule_With_Property', + ActionScheduler_ScheduleDeserializer::unserialize( $blob ), + 'A nested class allow-listed via the filter should be accepted.' + ); + } + + /** + * A third party schedule that keeps built-in date/time value objects (DateTime, DateTimeImmutable, + * DateTimeZone, DateInterval) as properties must round-trip with no configuration — those classes + * are on the default nested allow-list. + */ + public function test_third_party_schedule_nesting_datetime_objects_round_trips() { + $timestamp = time() + DAY_IN_SECONDS; + $blob = serialize( new ActionScheduler_Test_Custom_Schedule_With_Property( $timestamp ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + + $result = ActionScheduler_ScheduleDeserializer::unserialize( $blob ); + + $this->assertInstanceOf( 'ActionScheduler_Test_Custom_Schedule_With_Property', $result ); + $this->assertEquals( $timestamp, $result->next()->getTimestamp() ); + } + + /** + * A pathologically deep object graph must be rejected rather than walked into a stack overflow. + * (Cyclic graphs are covered separately; this covers sheer depth beyond MAX_GRAPH_DEPTH.) + */ + public function test_excessively_deep_graph_is_rejected() { + // A valid (third party) schedule class as the outer object so the top-level gate passes, wrapping + // a scalar buried in ~150 nested arrays — well beyond the depth we are willing to vet. + $class = 'ActionScheduler_Test_Custom_Schedule_With_Property'; + $deep = 'i:1;'; + for ( $i = 0; $i < 150; $i++ ) { + $deep = 'a:1:{i:0;' . $deep . '}'; + } + $blob = 'O:' . strlen( $class ) . ':"' . $class . '":1:{s:4:"deep";' . $deep . '}'; + + $this->assertFalse( + ActionScheduler_ScheduleDeserializer::unserialize( $blob ), + 'A graph deeper than the vetting bound should be rejected.' + ); + } + + /** + * The deserializer instance is reusable: invoking it for one blob must not leak walk state into the + * next. A rejected blob followed by a clean one must not taint the clean result. + */ + public function test_deserializer_instance_is_reusable() { + $deserializer = new ActionScheduler_ScheduleDeserializer( + array( 'ActionScheduler_Test_Custom_Schedule' ), + array() + ); + + // First: a blob nesting a gadget (rejected, populating internal offender/seen state). + $this->assertFalse( $deserializer( $this->nested_gadget_blob() ) ); + + // Then: a clean third party schedule must still deserialize correctly. + $clean = serialize( new ActionScheduler_Test_Custom_Schedule( time() + HOUR_IN_SECONDS ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $this->assertInstanceOf( + 'ActionScheduler_Test_Custom_Schedule', + $deserializer( $clean ), + 'Reusing a deserializer instance leaked state from a prior blob.' + ); + } + + /** + * The constructor's nested allow-list is authoritative: a class it lists is accepted when nested, + * and the same blob is rejected when it is not listed. + */ + public function test_constructor_nested_allow_list_is_used() { + $blob = serialize( new ActionScheduler_Test_Custom_Schedule_With_Property( time() + DAY_IN_SECONDS, new ActionScheduler_Test_Schedule_Helper( 7 ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + + $without = new ActionScheduler_ScheduleDeserializer( array(), array() ); + $this->assertFalse( $without( $blob ), 'Nested support classes absent from the allow-list must be rejected.' ); + + $with = new ActionScheduler_ScheduleDeserializer( + array(), + array( 'DateTime', 'DateTimeImmutable', 'DateTimeZone', 'DateInterval', 'ActionScheduler_Test_Schedule_Helper' ) + ); + $this->assertInstanceOf( + 'ActionScheduler_Test_Custom_Schedule_With_Property', + $with( $blob ), + 'A nested class present in the injected allow-list must be accepted.' + ); + } + + /** + * The post-based store reads the raw schedule meta. A non-serialized meta value cannot be a schedule + * object, so it is left for validate_schedule() to reject rather than being deserialized — which + * fetch_action() surfaces as a failed fetch and a null action, not a usable schedule. + */ + public function test_wp_post_store_rejects_non_serialized_schedule_meta() { + $store = new ActionScheduler_wpPostStore(); + $action = new ActionScheduler_Action( ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, array(), new ActionScheduler_SimpleSchedule( as_get_datetime_object() ) ); + $action_id = $store->save_action( $action ); + + global $wpdb; + // phpcs:disable WordPress.DB.SlowDBQuery.slow_db_query_meta_value, WordPress.DB.SlowDBQuery.slow_db_query_meta_key + $wpdb->update( + $wpdb->postmeta, + array( 'meta_value' => 'this-is-not-serialized' ), + array( + 'post_id' => $action_id, + 'meta_key' => ActionScheduler_wpPostStore::SCHEDULE_META_KEY, + ), + array( '%s' ), + array( '%d', '%s' ) + ); + // phpcs:enable WordPress.DB.SlowDBQuery.slow_db_query_meta_value, WordPress.DB.SlowDBQuery.slow_db_query_meta_key + clean_post_cache( $action_id ); + wp_cache_delete( $action_id, 'post_meta' ); + + $failed = false; + add_action( + 'action_scheduler_failed_fetch_action', + function () use ( &$failed ) { + $failed = true; + } + ); + + $fetched = $store->fetch_action( $action_id ); + + $this->assertTrue( $failed, 'A non-serialized schedule meta value was not treated as an invalid action.' ); + $this->assertInstanceOf( 'ActionScheduler_NullAction', $fetched ); + } } From c3af51cc1e5793fbee69c6030634062ee5e87a5c Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:20:46 -0700 Subject: [PATCH 10/16] fix: allow ActionScheduler_DateTime nested in schedules as_get_datetime_object() returns an ActionScheduler_DateTime (a DateTime subclass), so a third-party schedule that stores that idiomatic helper's result as a property nests one rather than a plain DateTime, and was rejected because only DateTime itself was on the default nested allow-list. It is a trivial subclass with no added magic methods, so it is safe to trust. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ActionScheduler_ScheduleDeserializer.php | 5 +++- ...tionScheduler_ScheduleUnserialize_Test.php | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index d2f9d12b..c5952685 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -51,7 +51,10 @@ class ActionScheduler_ScheduleDeserializer { CronExpression_DayOfWeekField::class, CronExpression_YearField::class, - // Additional trusted classes, likely to be used in schedule representations. + // Additional trusted classes, likely to be used in schedule representations. ActionScheduler_DateTime + // is included because as_get_datetime_object() returns one, so a third-party schedule that stores + // the result of that helper as a property nests it rather than a plain DateTime. + ActionScheduler_DateTime::class, DateTime::class, DateTimeImmutable::class, DateTimeZone::class, diff --git a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php index 2b122bc3..7503e66b 100644 --- a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php +++ b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php @@ -509,4 +509,27 @@ function () use ( &$failed ) { $this->assertTrue( $failed, 'A non-serialized schedule meta value was not treated as an invalid action.' ); $this->assertInstanceOf( 'ActionScheduler_NullAction', $fetched ); } + + /** + * as_get_datetime_object() returns an ActionScheduler_DateTime (a DateTime subclass), so a third + * party schedule that stores the result of that idiomatic helper as a property nests one rather than + * a plain DateTime. It must be on the default nested allow-list, or such a schedule would be rejected. + */ + public function test_third_party_schedule_nesting_action_scheduler_datetime_round_trips() { + // A third party schedule (protected $timestamp) holding an ActionScheduler_DateTime instead of an + // int. Hand-crafted so the nested object is genuinely an ActionScheduler_DateTime. + $nul = chr( 0 ); + $prop = $nul . '*' . $nul . 'timestamp'; + $class = 'ActionScheduler_Test_Custom_Schedule'; + $date = serialize( as_get_datetime_object( '2026-01-02 03:04:05' ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $blob = 'O:' . strlen( $class ) . ':"' . $class . '":1:{s:' . strlen( $prop ) . ':"' . $prop . '";' . $date . '}'; + + $result = ActionScheduler_ScheduleDeserializer::unserialize( $blob ); + + $this->assertInstanceOf( 'ActionScheduler_Test_Custom_Schedule', $result ); + + $property = new ReflectionProperty( 'ActionScheduler_Test_Custom_Schedule', 'timestamp' ); + $property->setAccessible( true ); + $this->assertInstanceOf( 'ActionScheduler_DateTime', $property->getValue( $result ) ); + } } From 9aefaabf938548f40f9e9b880fb847e0f72f2070 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:21:27 -0700 Subject: [PATCH 11/16] Keep commentary lean. --- classes/ActionScheduler_ScheduleDeserializer.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index c5952685..0760eee5 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -51,9 +51,7 @@ class ActionScheduler_ScheduleDeserializer { CronExpression_DayOfWeekField::class, CronExpression_YearField::class, - // Additional trusted classes, likely to be used in schedule representations. ActionScheduler_DateTime - // is included because as_get_datetime_object() returns one, so a third-party schedule that stores - // the result of that helper as a property nests it rather than a plain DateTime. + // Additional trusted classes, likely to be used in schedule representations. ActionScheduler_DateTime::class, DateTime::class, DateTimeImmutable::class, From 0434f0875999198a6f6a16a5435ec5412dc1172b Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:30:26 -0700 Subject: [PATCH 12/16] test: reject a gadget nested in a third-party schedule wrapper Covers the one attack shape the existing nested-gadget test did not: the outer object is a valid third-party schedule (a placeholder in the safe parse, not a core schedule), so the walk must recurse into that placeholder to find the nested gadget. Asserts the blob is rejected and the gadget's __wakeup() never runs. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- ...tionScheduler_ScheduleUnserialize_Test.php | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php index 7503e66b..a98e0f13 100644 --- a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php +++ b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php @@ -532,4 +532,25 @@ public function test_third_party_schedule_nesting_action_scheduler_datetime_roun $property->setAccessible( true ); $this->assertInstanceOf( 'ActionScheduler_DateTime', $property->getValue( $result ) ); } + + /** + * A gadget buried inside a valid third party schedule wrapper must be rejected without ever being + * instantiated. Unlike the core-schedule wrapper case, the outer object is itself a placeholder in + * the safe parse, so this exercises the walk recursing into a placeholder to find a nested one. + */ + public function test_gadget_nested_in_third_party_schedule_is_rejected_without_instantiation() { + $nul = chr( 0 ); + $prop = $nul . '*' . $nul . 'timestamp'; + $class = 'ActionScheduler_Test_Custom_Schedule'; // Valid third party schedule → placeholder in the safe parse. + $gadget = serialize( new ActionScheduler_Test_Evil_Gadget() ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $blob = 'O:' . strlen( $class ) . ':"' . $class . '":1:{s:' . strlen( $prop ) . ':"' . $prop . '";' . $gadget . '}'; + + $result = ActionScheduler_ScheduleDeserializer::unserialize( $blob ); + + $this->assertFalse( $result ); + $this->assertFalse( + ActionScheduler_Test_Evil_Gadget::$fired, + 'A gadget nested in a third party schedule was instantiated.' + ); + } } From 978ae2df5302e7dcee7387440736a15f0005d163 Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:19:40 -0700 Subject: [PATCH 13/16] Changelog. --- changelog.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/changelog.txt b/changelog.txt index d60ff345..58394ba2 100644 --- a/changelog.txt +++ b/changelog.txt @@ -3,6 +3,8 @@ = 4.1.0 - xxxx-xx-xx = * Use `wp_admin_notice()` to render admin notices, instead of custom HTML. * Fix an oversight in the lock implementation (used to rate-limit async request runners) that could result in a database error in unusual cases. +* Add protections to guard against the risk of object-injection/deserialization attacks when retrieving stored schedule data. +* Dev - Add filters (`action_scheduler_allowed_nested_schedule_classes`, `action_scheduler_enforce_schedule_allowed_classes`) and an action (`action_scheduler_unexpected_schedule_class`) to tune and observe schedule deserialization. * Dev - Updates the `wpPostStore` to capture any database errors that might occur while claiming actions. = 4.0.0 - 2026-06-16 = From 6c8560a591c2e525d67a39e5edfda1ee930952cd Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:08:48 -0700 Subject: [PATCH 14/16] Minor cleanup --- classes/ActionScheduler_ScheduleDeserializer.php | 6 +++--- .../jobstore/ActionScheduler_ScheduleUnserialize_Test.php | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index 0760eee5..834334a6 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -129,8 +129,8 @@ public static function unserialize( $blob ) { /** * Build our ActionScheduler_ScheduleDeserializer instance. * - * @param array $allowed_scheduler_classes - * @param array $allowed_nested_classes + * @param array $allowed_scheduler_classes Scheduler classes we accept. + * @param array $allowed_nested_classes Nested classes we accept. */ public function __construct( array $allowed_scheduler_classes, array $allowed_nested_classes ) { $this->allowed_scheduler_classes = $allowed_scheduler_classes; @@ -177,7 +177,7 @@ public function __invoke( $serialized_blob ) { } // Otherwise the top-level object is itself a valid schedule, but references one or more disallowed classes. - foreach ($this->potential_offenders as $offender ) { + foreach ( $this->potential_offenders as $offender ) { if ( ! $this->is_allowed_nested_class( $offender, $this->allowed_nested_classes ) ) { return $this->reject( $offender, true ); } diff --git a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php index a98e0f13..701280da 100644 --- a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php +++ b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php @@ -345,6 +345,7 @@ public function test_unexpected_class_action_fires_on_rejection() { $captured = array(); add_action( 'action_scheduler_unexpected_schedule_class', + // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed function ( $offending, $outer, $unexpected, $rejected ) use ( &$captured ) { $captured = compact( 'offending', 'outer', 'unexpected', 'rejected' ); }, From f97c63fe93102581d4f44f7a9273dc91b845cc9c Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:27:41 -0700 Subject: [PATCH 15/16] fix: treat a Throwable during schedule deserialization as corrupt data A tampered blob can keep a trusted schedule class as the outer object but replace a scalar property (e.g. scheduled_timestamp) with an object; the schedule's __wakeup() then feeds it to a DateTime constructor and throws a TypeError. That is an Error, so @ does not suppress it and the stores' catch (Exception) guards do not catch it -- one poisoned row could fatal a read (500 the admin list, stall a queue run). Wrap the two-pass deserialization in a catch (Throwable) that returns false, routing such failures into the existing corrupt-data path (null/cancelled). Not a regression (trunk ran the same __wakeup on the same bytes), but squarely within this change's goal of handling corrupt schedule data gracefully. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ActionScheduler_ScheduleDeserializer.php | 19 +++++++++++ ...tionScheduler_ScheduleUnserialize_Test.php | 32 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index 834334a6..a2ce6e50 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -153,6 +153,25 @@ public function __invoke( $serialized_blob ) { $this->potential_offenders = array(); $this->source_data = $serialized_blob; + try { + return $this->deserialize(); + } catch ( Throwable $e ) { + // Even with instantiation restricted to trusted classes, a tampered blob can still make one + // of them throw during unserialize(): e.g. a valid schedule whose scheduled_timestamp has + // been replaced with an object, which its __wakeup() then feeds to a DateTime constructor. + // That surfaces as an Error (which @ does not suppress and a `catch ( Exception )` does not + // catch). Treat any such failure as corrupt data and return false, which callers already + // handle gracefully, rather than letting it escape and fatal the read. + return false; + } + } + + /** + * Perform the two-pass deserialization for this instance's already-validated blob. + * + * @return ActionScheduler_Schedule|false + */ + private function deserialize() { // Initial attempt to unserialize, specifying all trusted classes: for default actions using // supplied schedule classes, this should succeed without any problems. Otherwise, the result // may contain one or more __PHP_Incomplete_Class references that we will need to examine. diff --git a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php index 701280da..ecdbaeb1 100644 --- a/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php +++ b/tests/phpunit/jobstore/ActionScheduler_ScheduleUnserialize_Test.php @@ -554,4 +554,36 @@ public function test_gadget_nested_in_third_party_schedule_is_rejected_without_i 'A gadget nested in a third party schedule was instantiated.' ); } + + /** + * A tampered blob can make an otherwise-trusted schedule class throw during deserialization — for + * example, a valid schedule whose `scheduled_timestamp` scalar has been replaced with an object, + * which the schedule's `__wakeup()` then feeds to a `DateTime` constructor. That surfaces as a + * `TypeError` (an `Error`, which `@` does not suppress and a `catch ( Exception )` does not catch). + * The deserializer must absorb it and report corrupt data, not let it escape and fatal the read. + */ + public function test_tampered_blob_that_makes_a_trusted_class_throw_is_reported_as_corrupt() { + $nul = chr( 0 ); + $prop = $nul . '*' . $nul . 'scheduled_timestamp'; // Protected property marker. + $class = 'ActionScheduler_IntervalSchedule'; // Trusted → instantiated (and woken) in the safe parse. + $evil = 'O:12:"Totally_Fake":0:{}'; // An object where a timestamp is expected. + $blob = 'O:' . strlen( $class ) . ':"' . $class . '":1:{s:' . strlen( $prop ) . ':"' . $prop . '";' . $evil . '}'; + + // Without the guard this call throws an uncaught TypeError and this test errors instead of passing. + $this->assertFalse( + ActionScheduler_ScheduleDeserializer::unserialize( $blob ), + 'A tampered blob that makes a trusted class throw should be reported as corrupt (false).' + ); + + // And the store must surface it as a handled (null) action rather than fataling the read. + $action_id = $this->store_action_with_raw_schedule( $blob ); + $store = new ActionScheduler_DBStore(); + $fetched = null; + try { + $fetched = $store->fetch_action( $action_id ); + } catch ( Exception $e ) { + unset( $e ); // A handled InvalidActionException is acceptable; an Error would have fataled above. + } + $this->assertNotNull( $fetched, 'Fetching a tampered action fataled instead of being handled.' ); + } } From 5af0374ee2f2393def86868a24542a8a560ec78c Mon Sep 17 00:00:00 2001 From: The Original Barry Hughes <3594411+barryhughes@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:55:17 -0700 Subject: [PATCH 16/16] Remove unnecessary commentary. --- classes/ActionScheduler_ScheduleDeserializer.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/classes/ActionScheduler_ScheduleDeserializer.php b/classes/ActionScheduler_ScheduleDeserializer.php index a2ce6e50..ffd142b0 100644 --- a/classes/ActionScheduler_ScheduleDeserializer.php +++ b/classes/ActionScheduler_ScheduleDeserializer.php @@ -156,12 +156,6 @@ public function __invoke( $serialized_blob ) { try { return $this->deserialize(); } catch ( Throwable $e ) { - // Even with instantiation restricted to trusted classes, a tampered blob can still make one - // of them throw during unserialize(): e.g. a valid schedule whose scheduled_timestamp has - // been replaced with an object, which its __wakeup() then feeds to a DateTime constructor. - // That surfaces as an Error (which @ does not suppress and a `catch ( Exception )` does not - // catch). Treat any such failure as corrupt data and return false, which callers already - // handle gracefully, rather than letting it escape and fatal the read. return false; } }