From 34f1ae62a13686e28ed733919ff37b368def8654 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 14 Oct 2025 13:56:42 +0200 Subject: [PATCH] fix(jobs): Limit command jobs to known cases Signed-off-by: Joas Schilling --- build/psalm-baseline.xml | 34 +++++++--- lib/composer/composer/autoload_classmap.php | 2 - lib/composer/composer/autoload_static.php | 2 - lib/private/Command/AsyncBus.php | 39 ++++-------- lib/private/Command/CallableJob.php | 21 ------- lib/private/Command/ClosureJob.php | 23 ------- lib/private/Command/CommandJob.php | 8 ++- lib/private/Command/CronBus.php | 38 ++--------- lib/private/Command/QueueBus.php | 43 ++++++------- lib/public/Command/IBus.php | 8 ++- lib/public/Command/ICommand.php | 1 + tests/lib/Command/AsyncBusTestCase.php | 70 --------------------- 12 files changed, 75 insertions(+), 214 deletions(-) delete mode 100644 lib/private/Command/CallableJob.php delete mode 100644 lib/private/Command/ClosureJob.php diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index a739d12529e..71d9fe016a4 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1795,6 +1795,9 @@ + + + @@ -1826,6 +1829,9 @@ + + + @@ -2046,6 +2052,11 @@ + + + + + @@ -2095,6 +2106,9 @@ + + + @@ -3468,15 +3482,19 @@ }, $context->getCalendarProviders())]]> - - - - + + + + + + - - - - + + + + + + diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 2f031d2fc11..1810eab032c 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1220,8 +1220,6 @@ return array( 'OC\\Collaboration\\Resources\\Resource' => $baseDir . '/lib/private/Collaboration/Resources/Resource.php', 'OC\\Color' => $baseDir . '/lib/private/Color.php', 'OC\\Command\\AsyncBus' => $baseDir . '/lib/private/Command/AsyncBus.php', - 'OC\\Command\\CallableJob' => $baseDir . '/lib/private/Command/CallableJob.php', - 'OC\\Command\\ClosureJob' => $baseDir . '/lib/private/Command/ClosureJob.php', 'OC\\Command\\CommandJob' => $baseDir . '/lib/private/Command/CommandJob.php', 'OC\\Command\\CronBus' => $baseDir . '/lib/private/Command/CronBus.php', 'OC\\Command\\FileAccess' => $baseDir . '/lib/private/Command/FileAccess.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index a09b068bec4..753e56107d0 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1261,8 +1261,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Collaboration\\Resources\\Resource' => __DIR__ . '/../../..' . '/lib/private/Collaboration/Resources/Resource.php', 'OC\\Color' => __DIR__ . '/../../..' . '/lib/private/Color.php', 'OC\\Command\\AsyncBus' => __DIR__ . '/../../..' . '/lib/private/Command/AsyncBus.php', - 'OC\\Command\\CallableJob' => __DIR__ . '/../../..' . '/lib/private/Command/CallableJob.php', - 'OC\\Command\\ClosureJob' => __DIR__ . '/../../..' . '/lib/private/Command/ClosureJob.php', 'OC\\Command\\CommandJob' => __DIR__ . '/../../..' . '/lib/private/Command/CommandJob.php', 'OC\\Command\\CronBus' => __DIR__ . '/../../..' . '/lib/private/Command/CronBus.php', 'OC\\Command\\FileAccess' => __DIR__ . '/../../..' . '/lib/private/Command/FileAccess.php', diff --git a/lib/private/Command/AsyncBus.php b/lib/private/Command/AsyncBus.php index fb7860c9dd8..54dc6d94e5a 100644 --- a/lib/private/Command/AsyncBus.php +++ b/lib/private/Command/AsyncBus.php @@ -1,5 +1,7 @@ canRunAsync($command)) { $this->queueCommand($command); } else { @@ -36,39 +36,29 @@ abstract class AsyncBus implements IBus { /** * Queue a command in the bus - * - * @param \OCP\Command\ICommand | callable $command */ - abstract protected function queueCommand($command); + abstract protected function queueCommand(ICommand $command); /** * Require all commands using a trait to be run synchronous * * @param string $trait */ - public function requireSync($trait) { + public function requireSync(string $trait): void { $this->syncTraits[] = trim($trait, '\\'); } - /** - * @param \OCP\Command\ICommand | callable $command - */ - private function runCommand($command) { - if ($command instanceof ICommand) { - $command->handle(); - } else { - $command(); - } + private function runCommand(ICommand $command): void { + $command->handle(); } /** - * @param \OCP\Command\ICommand | callable $command * @return bool */ - private function canRunAsync($command) { + private function canRunAsync(ICommand $command): bool { $traits = $this->getTraits($command); foreach ($traits as $trait) { - if (in_array($trait, $this->syncTraits)) { + if (in_array($trait, $this->syncTraits, true)) { return false; } } @@ -76,14 +66,9 @@ abstract class AsyncBus implements IBus { } /** - * @param \OCP\Command\ICommand | callable $command * @return string[] */ - private function getTraits($command) { - if ($command instanceof ICommand) { - return class_uses($command); - } else { - return []; - } + private function getTraits(ICommand $command): array { + return class_uses($command); } } diff --git a/lib/private/Command/CallableJob.php b/lib/private/Command/CallableJob.php deleted file mode 100644 index 8744977a81e..00000000000 --- a/lib/private/Command/CallableJob.php +++ /dev/null @@ -1,21 +0,0 @@ -getClosure(); - if (is_callable($callable)) { - $callable(); - } else { - throw new \InvalidArgumentException('Invalid serialized callable'); - } - } -} diff --git a/lib/private/Command/CommandJob.php b/lib/private/Command/CommandJob.php index 41ca3a9cb40..5ea2b35bf73 100644 --- a/lib/private/Command/CommandJob.php +++ b/lib/private/Command/CommandJob.php @@ -15,7 +15,13 @@ use OCP\Command\ICommand; */ class CommandJob extends QueuedJob { protected function run($argument) { - $command = unserialize($argument); + $command = unserialize($argument, ['allowed_classes' => [ + \Test\Command\SimpleCommand::class, + \Test\Command\StateFullCommand::class, + \Test\Command\FilesystemCommand::class, + \OCA\Files_Trashbin\Command\Expire::class, + \OCA\Files_Versions\Command\Expire::class, + ]]); if ($command instanceof ICommand) { $command->handle(); } else { diff --git a/lib/private/Command/CronBus.php b/lib/private/Command/CronBus.php index 31d31a25d42..1fa8cd45e7a 100644 --- a/lib/private/Command/CronBus.php +++ b/lib/private/Command/CronBus.php @@ -1,13 +1,13 @@ jobList->add($this->getJobClass($command), $this->serializeCommand($command)); - } - - /** - * @param ICommand|callable $command - * @return class-string - */ - private function getJobClass($command): string { - if ($command instanceof \Closure) { - return ClosureJob::class; - } elseif (is_callable($command)) { - return CallableJob::class; - } elseif ($command instanceof ICommand) { - return CommandJob::class; - } else { - throw new \InvalidArgumentException('Invalid command'); - } - } - - /** - * @param ICommand|callable $command - * @return string - */ - private function serializeCommand($command): string { - if ($command instanceof \Closure) { - return serialize(new SerializableClosure($command)); - } elseif (is_callable($command) || $command instanceof ICommand) { - return serialize($command); - } else { - throw new \InvalidArgumentException('Invalid command'); - } + protected function queueCommand(ICommand $command): void { + $this->jobList->add(CommandJob::class, serialize($command)); } } diff --git a/lib/private/Command/QueueBus.php b/lib/private/Command/QueueBus.php index ed7d43b38b6..2712ef731a4 100644 --- a/lib/private/Command/QueueBus.php +++ b/lib/private/Command/QueueBus.php @@ -1,5 +1,7 @@ queue[] = $command; } /** * Require all commands using a trait to be run synchronous - * - * @param string $trait */ - public function requireSync($trait) { + public function requireSync(string $trait): void { } - /** - * @param \OCP\Command\ICommand | callable $command - */ - private function runCommand($command) { - if ($command instanceof ICommand) { - // ensure the command can be serialized - $serialized = serialize($command); - if (strlen($serialized) > 4000) { - throw new \InvalidArgumentException('Trying to push a command which serialized form can not be stored in the database (>4000 character)'); - } - $unserialized = unserialize($serialized); - $unserialized->handle(); - } else { - $command(); + private function runCommand(ICommand $command): void { + // ensure the command can be serialized + $serialized = serialize($command); + if (strlen($serialized) > 4000) { + throw new \InvalidArgumentException('Trying to push a command which serialized form can not be stored in the database (>4000 character)'); } + $unserialized = unserialize($serialized, ['allowed_classes' => [ + \Test\Command\SimpleCommand::class, + \Test\Command\StateFullCommand::class, + \Test\Command\FilesystemCommand::class, + \OCA\Files_Trashbin\Command\Expire::class, + \OCA\Files_Versions\Command\Expire::class, + ]]); + $unserialized->handle(); } - public function run() { + public function run(): void { while ($command = array_shift($this->queue)) { $this->runCommand($command); } diff --git a/lib/public/Command/IBus.php b/lib/public/Command/IBus.php index 619b0c42109..35f26abb58c 100644 --- a/lib/public/Command/IBus.php +++ b/lib/public/Command/IBus.php @@ -10,16 +10,18 @@ namespace OCP\Command; /** * Interface IBus * + * @deprecated 33.0.0 The interface is considered internal going forward and should not be implemented by apps anymore * @since 8.1.0 */ interface IBus { /** * Schedule a command to be fired * - * @param \OCP\Command\ICommand | callable $command + * @param \OCP\Command\ICommand $command + * @since 33.0.0 Only allowed for {@see \OCA\Files_Trashbin\Command\Expire} and {@see \OCA\Files_Versions\Command\Expire} * @since 8.1.0 */ - public function push($command); + public function push(ICommand $command): void; /** * Require all commands using a trait to be run synchronous @@ -27,5 +29,5 @@ interface IBus { * @param string $trait * @since 8.1.0 */ - public function requireSync($trait); + public function requireSync(string $trait): void; } diff --git a/lib/public/Command/ICommand.php b/lib/public/Command/ICommand.php index 33435ad22db..e063bb5f97b 100644 --- a/lib/public/Command/ICommand.php +++ b/lib/public/Command/ICommand.php @@ -10,6 +10,7 @@ namespace OCP\Command; /** * Interface ICommand * + * @deprecated 33.0.0 The interface is considered internal going forward and should not be implemented by apps anymore * @since 8.1.0 */ interface ICommand { diff --git a/tests/lib/Command/AsyncBusTestCase.php b/tests/lib/Command/AsyncBusTestCase.php index bb47de30b11..1b9aeea9bdc 100644 --- a/tests/lib/Command/AsyncBusTestCase.php +++ b/tests/lib/Command/AsyncBusTestCase.php @@ -38,23 +38,6 @@ class FilesystemCommand implements ICommand { } } -function basicFunction() { - AsyncBusTestCase::$lastCommand = 'function'; -} - -// clean class to prevent phpunit putting closure in $this -class ThisClosureTest { - private function privateMethod() { - AsyncBusTestCase::$lastCommand = 'closure-this'; - } - - public function test(IBus $bus) { - $bus->push(function (): void { - $this->privateMethod(); - }); - } -} - abstract class AsyncBusTestCase extends TestCase { /** * Basic way to check output from a command @@ -105,59 +88,6 @@ abstract class AsyncBusTestCase extends TestCase { $this->assertEquals('foo', self::$lastCommand); } - public function testStaticCallable(): void { - $this->getBus()->push(['\Test\Command\AsyncBusTestCase', 'DummyCommand']); - $this->runJobs(); - $this->assertEquals('static', self::$lastCommand); - } - - public function testMemberCallable(): void { - $command = new StateFullCommand('bar'); - $this->getBus()->push([$command, 'handle']); - $this->runJobs(); - $this->assertEquals('bar', self::$lastCommand); - } - - public function testFunctionCallable(): void { - $this->getBus()->push('\Test\Command\BasicFunction'); - $this->runJobs(); - $this->assertEquals('function', self::$lastCommand); - } - - public function testClosure(): void { - $this->getBus()->push(function (): void { - AsyncBusTestCase::$lastCommand = 'closure'; - }); - $this->runJobs(); - $this->assertEquals('closure', self::$lastCommand); - } - - public function testClosureSelf(): void { - $this->getBus()->push(function (): void { - AsyncBusTestCase::$lastCommand = 'closure-self'; - }); - $this->runJobs(); - $this->assertEquals('closure-self', self::$lastCommand); - } - - - public function testClosureThis(): void { - // clean class to prevent phpunit putting closure in $this - $test = new ThisClosureTest(); - $test->test($this->getBus()); - $this->runJobs(); - $this->assertEquals('closure-this', self::$lastCommand); - } - - public function testClosureBind(): void { - $state = 'bar'; - $this->getBus()->push(function () use ($state): void { - AsyncBusTestCase::$lastCommand = 'closure-' . $state; - }); - $this->runJobs(); - $this->assertEquals('closure-bar', self::$lastCommand); - } - public function testFileFileAccessCommand(): void { $this->getBus()->push(new FilesystemCommand()); $this->assertEquals('', self::$lastCommand);