From a0a0727c88bbc6fa0a0453f6461e39cff0d3f267 Mon Sep 17 00:00:00 2001 From: Snider Date: Thu, 12 Mar 2026 13:56:14 +0000 Subject: [PATCH] =?UTF-8?q?fix(actions):=20harden=20scheduled=20actions=20?= =?UTF-8?q?=E2=80=94=20security=20allowlists,=20trait=20verification,=20sc?= =?UTF-8?q?an=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add ALLOWED_NAMESPACES prefix allowlist to ScheduleServiceProvider - Add ALLOWED_FREQUENCIES method allowlist (prevents arbitrary method dispatch) - Verify Action trait on scheduled classes before dispatch - Move try/catch inside foreach for per-action isolation - Add empty-scan guard to ScheduleSyncCommand (prevents disabling all rows) - Consolidate ScheduledActionScanner to single tokenisation pass - Cast numeric frequency args via ctype_digit() in ScheduledAction Co-Authored-By: Claude Opus 4.6 --- src/Core/Actions/ScheduleServiceProvider.php | 82 ++++++++++++++-- src/Core/Actions/ScheduledAction.php | 10 +- src/Core/Actions/ScheduledActionScanner.php | 93 +++++++------------ .../Console/Commands/ScheduleSyncCommand.php | 17 ++-- tests/Feature/ScheduledActionModelTest.php | 2 +- 5 files changed, 128 insertions(+), 76 deletions(-) diff --git a/src/Core/Actions/ScheduleServiceProvider.php b/src/Core/Actions/ScheduleServiceProvider.php index 54c5c69..2d525e2 100644 --- a/src/Core/Actions/ScheduleServiceProvider.php +++ b/src/Core/Actions/ScheduleServiceProvider.php @@ -26,6 +26,27 @@ use Illuminate\Support\ServiceProvider; */ class ScheduleServiceProvider extends ServiceProvider { + /** + * Allowed namespace prefixes — prevents autoloading of classes from unexpected namespaces. + */ + private const ALLOWED_NAMESPACES = ['App\\', 'Core\\', 'Mod\\']; + + /** + * Allowed frequency methods — prevents arbitrary method dispatch from DB strings. + */ + private const ALLOWED_FREQUENCIES = [ + 'everyMinute', 'everyTwoMinutes', 'everyThreeMinutes', 'everyFourMinutes', + 'everyFiveMinutes', 'everyTenMinutes', 'everyFifteenMinutes', 'everyThirtyMinutes', + 'hourly', 'hourlyAt', 'everyOddHour', 'everyTwoHours', 'everyThreeHours', + 'everyFourHours', 'everySixHours', + 'daily', 'dailyAt', 'twiceDaily', 'twiceDailyAt', + 'weekly', 'weeklyOn', + 'monthly', 'monthlyOn', 'twiceMonthly', 'lastDayOfMonth', + 'quarterly', 'quarterlyOn', + 'yearly', 'yearlyOn', + 'cron', + ]; + public function boot(): void { if (! $this->app->runningInConsole()) { @@ -33,29 +54,72 @@ class ScheduleServiceProvider extends ServiceProvider } // Guard against table not existing (pre-migration) - if (! Schema::hasTable('scheduled_actions')) { + try { + if (! Schema::hasTable('scheduled_actions')) { + return; + } + } catch (\Throwable) { + // DB unreachable — skip gracefully so scheduler doesn't crash return; } $this->app->booted(function () { - $schedule = $this->app->make(Schedule::class); + $this->registerScheduledActions(); + }); + } - $actions = ScheduledAction::enabled()->get(); + private function registerScheduledActions(): void + { + $schedule = $this->app->make(Schedule::class); + $actions = ScheduledAction::enabled()->get(); - foreach ($actions as $action) { + foreach ($actions as $action) { + try { $class = $action->action_class; + // Validate namespace prefix against allowlist + $hasAllowedNamespace = false; + + foreach (self::ALLOWED_NAMESPACES as $prefix) { + if (str_starts_with($class, $prefix)) { + $hasAllowedNamespace = true; + + break; + } + } + + if (! $hasAllowedNamespace) { + logger()->warning("Scheduled action {$class} has disallowed namespace — skipping"); + + continue; + } + if (! class_exists($class)) { continue; } + // Verify the class uses the Action trait + if (! in_array(\Core\Actions\Action::class, class_uses_recursive($class), true)) { + logger()->warning("Scheduled action {$class} does not use the Action trait — skipping"); + + continue; + } + + // Validate frequency method against allowlist + $method = $action->frequencyMethod(); + + if (! in_array($method, self::ALLOWED_FREQUENCIES, true)) { + logger()->warning("Scheduled action {$class} has invalid frequency method: {$method}"); + + continue; + } + $event = $schedule->call(function () use ($class, $action) { $class::run(); $action->markRun(); })->name($class); // Apply frequency - $method = $action->frequencyMethod(); $args = $action->frequencyArgs(); $event->{$method}(...$args); @@ -64,10 +128,16 @@ class ScheduleServiceProvider extends ServiceProvider $event->withoutOverlapping(); } + if ($action->run_in_background) { + $event->runInBackground(); + } + if ($action->timezone) { $event->timezone($action->timezone); } + } catch (\Throwable $e) { + logger()->warning("Failed to register scheduled action: {$action->action_class} — {$e->getMessage()}"); } - }); + } } } diff --git a/src/Core/Actions/ScheduledAction.php b/src/Core/Actions/ScheduledAction.php index a32d1be..298832c 100644 --- a/src/Core/Actions/ScheduledAction.php +++ b/src/Core/Actions/ScheduledAction.php @@ -76,8 +76,11 @@ class ScheduledAction extends Model * Parse the frequency string and return the arguments. * * 'dailyAt:09:00' → ['09:00'] - * 'weeklyOn:1,09:00' → ['1', '09:00'] + * 'weeklyOn:1,09:00' → [1, '09:00'] * 'everyMinute' → [] + * + * Numeric strings are cast to integers so methods like weeklyOn() + * receive the correct types. */ public function frequencyArgs(): array { @@ -87,7 +90,10 @@ class ScheduledAction extends Model return []; } - return explode(',', $parts[1]); + return array_map( + static fn (string $arg) => ctype_digit($arg) ? (int) $arg : $arg, + explode(',', $parts[1]) + ); } /** diff --git a/src/Core/Actions/ScheduledActionScanner.php b/src/Core/Actions/ScheduledActionScanner.php index b5237ae..8c22869 100644 --- a/src/Core/Actions/ScheduledActionScanner.php +++ b/src/Core/Actions/ScheduledActionScanner.php @@ -101,20 +101,49 @@ class ScheduledActionScanner return null; } + $tokens = token_get_all($contents); $namespace = null; $class = null; - foreach (token_get_all($contents) as $token) { + $count = count($tokens); + + for ($i = 0; $i < $count; $i++) { + $token = $tokens[$i]; + if (! is_array($token)) { continue; } if ($token[0] === T_NAMESPACE) { - $namespace = $this->extractNamespace($contents); + $namespaceParts = []; + + for ($j = $i + 1; $j < $count; $j++) { + $t = $tokens[$j]; + + if (is_array($t) && in_array($t[0], [T_NAME_QUALIFIED, T_STRING, T_NS_SEPARATOR], true)) { + $namespaceParts[] = $t[1]; + } elseif ($t === ';' || $t === '{') { + break; + } + } + + $namespace = implode('', $namespaceParts) ?: null; } if ($token[0] === T_CLASS) { - $class = $this->extractClassName($contents); + for ($j = $i + 1; $j < $count; $j++) { + $t = $tokens[$j]; + + if (is_array($t) && $t[0] === T_WHITESPACE) { + continue; + } + if (is_array($t) && $t[0] === T_STRING) { + $class = $t[1]; + } + + break; + } + break; } } @@ -125,62 +154,4 @@ class ScheduledActionScanner return $namespace !== null ? "{$namespace}\\{$class}" : $class; } - - /** - * Extract the namespace string from file contents. - */ - private function extractNamespace(string $contents): ?string - { - $tokens = token_get_all($contents); - $capture = false; - $parts = []; - - foreach ($tokens as $token) { - if (is_array($token) && $token[0] === T_NAMESPACE) { - $capture = true; - - continue; - } - - if ($capture) { - if (is_array($token) && in_array($token[0], [T_NAME_QUALIFIED, T_STRING, T_NS_SEPARATOR], true)) { - $parts[] = $token[1]; - } elseif ($token === ';' || $token === '{') { - break; - } - } - } - - return ! empty($parts) ? implode('', $parts) : null; - } - - /** - * Extract class name from tokens after T_CLASS. - */ - private function extractClassName(string $contents): ?string - { - $tokens = token_get_all($contents); - $nextIsClass = false; - - foreach ($tokens as $token) { - if (is_array($token) && $token[0] === T_CLASS) { - $nextIsClass = true; - - continue; - } - - if ($nextIsClass && is_array($token)) { - if ($token[0] === T_WHITESPACE) { - continue; - } - if ($token[0] === T_STRING) { - return $token[1]; - } - - return null; - } - } - - return null; - } } diff --git a/src/Core/Console/Commands/ScheduleSyncCommand.php b/src/Core/Console/Commands/ScheduleSyncCommand.php index 1cbce72..e652e05 100644 --- a/src/Core/Console/Commands/ScheduleSyncCommand.php +++ b/src/Core/Console/Commands/ScheduleSyncCommand.php @@ -74,13 +74,18 @@ class ScheduleSyncCommand extends Command // Disable actions no longer in codebase $discoveredClasses = array_keys($discovered); - $stale = ScheduledAction::where('is_enabled', true) - ->whereNotIn('action_class', $discoveredClasses) - ->get(); - foreach ($stale as $action) { - $action->update(['is_enabled' => false]); - $disabled++; + if (empty($discoveredClasses)) { + $this->warn('No scheduled actions discovered — skipping stale cleanup to avoid disabling all rows.'); + } else { + $stale = ScheduledAction::where('is_enabled', true) + ->whereNotIn('action_class', $discoveredClasses) + ->get(); + + foreach ($stale as $action) { + $action->update(['is_enabled' => false]); + $disabled++; + } } $this->info("Schedule sync complete: {$added} added, {$disabled} disabled, {$unchanged} unchanged."); diff --git a/tests/Feature/ScheduledActionModelTest.php b/tests/Feature/ScheduledActionModelTest.php index f7225c2..804c42a 100644 --- a/tests/Feature/ScheduledActionModelTest.php +++ b/tests/Feature/ScheduledActionModelTest.php @@ -70,7 +70,7 @@ class ScheduledActionModelTest extends TestCase { $action = new ScheduledAction(['frequency' => 'weeklyOn:1,09:00']); $this->assertSame('weeklyOn', $action->frequencyMethod()); - $this->assertSame(['1', '09:00'], $action->frequencyArgs()); + $this->assertSame([1, '09:00'], $action->frequencyArgs()); } public function test_mark_run_updates_last_run_at(): void