fix(actions): harden scheduled actions — security allowlists, trait verification, scan safety
- 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 <noreply@anthropic.com>
This commit is contained in:
parent
3c4011bdcb
commit
a0a0727c88
5 changed files with 128 additions and 76 deletions
|
|
@ -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()}");
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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])
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.");
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue