Merge pull request #56630 from nextcloud/carl/workflowengine-hardening

refactor(workflowengine): Check if class is correct

Author: AndyScherzinger

apps/workflowengine/lib/Manager.php

@@ -422,16 +422,18 @@ public function deleteOperation(int $id, ScopeContext $scopeContext): bool {
 	 * @param array $events
 	 */
 	protected function validateEvents(string $entity, array $events, IOperation $operation): void {
+		/** @psalm-suppress TaintedCallable newInstance is not called */
+		$reflection = new \ReflectionClass($entity);
+		if ($entity !== IEntity::class && !in_array(IEntity::class, $reflection->getInterfaceNames())) {
+			throw new \UnexpectedValueException($this->l->t('Entity %s is invalid', [$entity]));
+		}
+
 		try {
 			$instance = $this->container->get($entity);
 		} catch (ContainerExceptionInterface $e) {
 			throw new \UnexpectedValueException($this->l->t('Entity %s does not exist', [$entity]));
 		}
 
-		if (!$instance instanceof IEntity) {
-			throw new \UnexpectedValueException($this->l->t('Entity %s is invalid', [$entity]));
-		}
-
 		if (empty($events)) {
 			if (!$operation instanceof IComplexOperation) {
 				throw new \UnexpectedValueException($this->l->t('No events are chosen.'));
@@ -458,17 +460,23 @@ protected function validateEvents(string $entity, array $events, IOperation $ope
 	 * @throws \UnexpectedValueException
 	 */
 	public function validateOperation(string $class, string $name, array $checks, string $operation, ScopeContext $scope, string $entity, array $events): void {
+		if (strlen($operation) > IManager::MAX_OPERATION_VALUE_BYTES) {
+			throw new \UnexpectedValueException($this->l->t('The provided operation data is too long'));
+		}
+
+		/** @psalm-suppress TaintedCallable newInstance is not called */
+		$reflection = new \ReflectionClass($class);
+		if ($class !== IOperation::class && !in_array(IOperation::class, $reflection->getInterfaceNames())) {
+			throw new \UnexpectedValueException($this->l->t('Operation %s is invalid', [$class]) . join(', ', $reflection->getInterfaceNames()));
+		}
+
 		try {
 			/** @var IOperation $instance */
 			$instance = $this->container->get($class);
 		} catch (ContainerExceptionInterface $e) {
 			throw new \UnexpectedValueException($this->l->t('Operation %s does not exist', [$class]));
 		}
 
-		if (!($instance instanceof IOperation)) {
-			throw new \UnexpectedValueException($this->l->t('Operation %s is invalid', [$class]));
-		}
-
 		if (!$instance->isAvailableForScope($scope->getScope())) {
 			throw new \UnexpectedValueException($this->l->t('Operation %s is invalid', [$class]));
 		}
@@ -479,38 +487,35 @@ public function validateOperation(string $class, string $name, array $checks, st
 			throw new \UnexpectedValueException($this->l->t('At least one check needs to be provided'));
 		}
 
-		if (strlen($operation) > IManager::MAX_OPERATION_VALUE_BYTES) {
-			throw new \UnexpectedValueException($this->l->t('The provided operation data is too long'));
-		}
-
 		$instance->validateOperation($name, $checks, $operation);
 
 		foreach ($checks as $check) {
 			if (!is_string($check['class'])) {
 				throw new \UnexpectedValueException($this->l->t('Invalid check provided'));
 			}
 
+			if (strlen((string)$check['value']) > IManager::MAX_CHECK_VALUE_BYTES) {
+				throw new \UnexpectedValueException($this->l->t('The provided check value is too long'));
+			}
+
+			$reflection = new \ReflectionClass($check['class']);
+			if ($check['class'] !== ICheck::class && !in_array(ICheck::class, $reflection->getInterfaceNames())) {
+				throw new \UnexpectedValueException($this->l->t('Check %s is invalid', [$class]));
+			}
+
 			try {
 				/** @var ICheck $instance */
 				$instance = $this->container->get($check['class']);
 			} catch (ContainerExceptionInterface) {
 				throw new \UnexpectedValueException($this->l->t('Check %s does not exist', [$class]));
 			}
 
-			if (!($instance instanceof ICheck)) {
-				throw new \UnexpectedValueException($this->l->t('Check %s is invalid', [$class]));
-			}
-
 			if (!empty($instance->supportedEntities())
 				&& !in_array($entity, $instance->supportedEntities())
 			) {
 				throw new \UnexpectedValueException($this->l->t('Check %s is not allowed with this entity', [$class]));
 			}
 
-			if (strlen((string)$check['value']) > IManager::MAX_CHECK_VALUE_BYTES) {
-				throw new \UnexpectedValueException($this->l->t('The provided check value is too long'));
-			}
-
 			$instance->validateCheck($check['operator'], $check['value']);
 		}
 	}

apps/workflowengine/tests/ManagerTest.php

@@ -12,6 +12,7 @@
 use OCA\WorkflowEngine\Manager;
 use OCP\AppFramework\QueryException;
 use OCP\AppFramework\Services\IAppConfig;
+use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Events\Node\NodeCreatedEvent;
 use OCP\Files\IRootFolder;
@@ -31,11 +32,42 @@
 use OCP\WorkflowEngine\IEntityEvent;
 use OCP\WorkflowEngine\IManager;
 use OCP\WorkflowEngine\IOperation;
+use OCP\WorkflowEngine\IRuleMatcher;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
+class TestAdminOp implements IOperation {
+	public function getDisplayName(): string {
+		return 'Admin';
+	}
+
+	public function getDescription(): string {
+		return '';
+	}
+
+	public function getIcon(): string {
+		return '';
+	}
+
+	public function isAvailableForScope(int $scope): bool {
+		return true;
+	}
+
+	public function validateOperation(string $name, array $checks, string $operation): void {
+	}
+
+	public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatcher): void {
+	}
+}
+
+class TestUserOp extends TestAdminOp {
+	public function getDisplayName(): string {
+		return 'User';
+	}
+}
+
 /**
  * Class ManagerTest
  *
@@ -393,19 +425,19 @@ public function testUpdateOperation(): void {
 		$opId1 = $this->invokePrivate(
 			$this->manager,
 			'insertOperation',
-			['OCA\WFE\TestAdminOp', 'Test01', [11, 22], 'foo', $entity, []]
+			[TestAdminOp::class, 'Test01', [11, 22], 'foo', $entity, []]
 		);
 		$this->invokePrivate($this->manager, 'addScope', [$opId1, $adminScope]);
 
 		$opId2 = $this->invokePrivate(
 			$this->manager,
 			'insertOperation',
-			['OCA\WFE\TestUserOp', 'Test02', [33, 22], 'bar', $entity, []]
+			[TestUserOp::class, 'Test02', [33, 22], 'bar', $entity, []]
 		);
 		$this->invokePrivate($this->manager, 'addScope', [$opId2, $userScope]);
 
-		$check1 = ['class' => 'OCA\WFE\C22', 'operator' => 'eq', 'value' => 'asdf'];
-		$check2 = ['class' => 'OCA\WFE\C33', 'operator' => 'eq', 'value' => 23456];
+		$check1 = ['class' => ICheck::class, 'operator' => 'eq', 'value' => 'asdf'];
+		$check2 = ['class' => ICheck::class, 'operator' => 'eq', 'value' => 23456];
 
 		/** @noinspection PhpUnhandledExceptionInspection */
 		$op = $this->manager->updateOperation($opId1, 'Test01a', [$check1, $check2], 'foohur', $adminScope, $entity, ['\OCP\Files::postDelete']);
@@ -667,11 +699,6 @@ public function testValidateOperationDataLengthError(): void {
 			->method('getScope')
 			->willReturn(IManager::SCOPE_ADMIN);
 
-		$operationMock->expects($this->once())
-			->method('isAvailableForScope')
-			->with(IManager::SCOPE_ADMIN)
-			->willReturn(true);
-
 		$operationMock->expects($this->never())
 			->method('validateOperation');
 
@@ -719,7 +746,7 @@ public function testValidateOperationScopeNotAvailable(): void {
 			'operator' => 'is',
 			'value' => 'barfoo',
 		];
-		$operationData = str_pad('', IManager::MAX_OPERATION_VALUE_BYTES + 1, 'FooBar');
+		$operationData = str_pad('', IManager::MAX_OPERATION_VALUE_BYTES - 1, 'FooBar');
 
 		$operationMock = $this->createMock(IOperation::class);
 		$entityMock = $this->createMock(IEntity::class);