Merge pull request #10421 from nextcloud/bugfix/10204/add-negative-te…

…sts-for-bots

tests(bots): add negative tests for bots

docs/occ.md

@@ -10,10 +10,10 @@ Install a new bot on the server
 
 | Arguments | Description | Is required | Is array | Default |
 |---|---|---|---|---|
-| `name` | The name under which the messages will be posted | yes | no | *Required* |
-| `secret` | Secret used to validate API calls | yes | no | *Required* |
-| `url` | Webhook endpoint to post messages to | yes | no | *Required* |
-| `description` | Optional description shown in the admin settings | no | no | `NULL` |
+| `name` | The name under which the messages will be posted (min. 1 char, max. 64 chars) | yes | no | *Required* |
+| `secret` | Secret used to validate API calls (min. 40 chars, max. 128 chars) | yes | no | *Required* |
+| `url` | Webhook endpoint to post messages to (max. 4000 chars) | yes | no | *Required* |
+| `description` | Optional description shown in the admin settings (max. 4000 chars) | no | no | `NULL` |
 
 | Options | Description | Accept value | Is value required | Is multiple | Default |
 |---|---|---|---|---|---|

lib/Chat/MessageParser.php

@@ -118,7 +118,7 @@ protected function setActor(Message $message): void {
 	protected function getBotNameByUrlHashForConversation(string $token, string $urlHash): ?string {
 		if (!isset($this->botNames[$token])) {
 			$this->botNames[$token] = [];
-			$bots = $this->botService->getBotsForToken($token);
+			$bots = $this->botService->getBotsForToken($token, null);
 			foreach ($bots as $bot) {
 				$botServer = $bot->getBotServer();
 				$this->botNames[$token][$botServer->getUrlHash()] = $botServer->getName();

lib/Command/Bot/Install.php

@@ -29,13 +29,17 @@
 use OCA\Talk\Model\Bot;
 use OCA\Talk\Model\BotServer;
 use OCA\Talk\Model\BotServerMapper;
+use OCA\Talk\Service\BotService;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\DB\Exception;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class Install extends Base {
 	public function __construct(
+		private BotService $botService,
 		private BotServerMapper $botServerMapper,
 	) {
 		parent::__construct();
@@ -49,22 +53,22 @@ protected function configure(): void {
 			->addArgument(
 				'name',
 				InputArgument::REQUIRED,
-				'The name under which the messages will be posted'
+				'The name under which the messages will be posted (min. 1 char, max. 64 chars)'
 			)
 			->addArgument(
 				'secret',
 				InputArgument::REQUIRED,
-				'Secret used to validate API calls'
+				'Secret used to validate API calls (min. 40 chars, max. 128 chars)'
 			)
 			->addArgument(
 				'url',
 				InputArgument::REQUIRED,
-				'Webhook endpoint to post messages to'
+				'Webhook endpoint to post messages to (max. 4000 chars)'
 			)
 			->addArgument(
 				'description',
 				InputArgument::OPTIONAL,
-				'Optional description shown in the admin settings'
+				'Optional description shown in the admin settings (max. 4000 chars)'
 			)
 			->addOption(
 				'no-setup',
@@ -88,7 +92,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 		$name = $input->getArgument('name');
 		$secret = $input->getArgument('secret');
 		$url = $input->getArgument('url');
-		$description = $input->getArgument('description');
+		$description = $input->getArgument('description') ?? '';
 		$noSetup = $input->getOption('no-setup');
 
 		if (!empty($input->getOption('feature'))) {
@@ -97,6 +101,20 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 			$featureFlags = Bot::FEATURE_WEBHOOK + Bot::FEATURE_RESPONSE;
 		}
 
+		try {
+			$this->botService->validateBotParameters($name, $secret, $url, $description);
+		} catch (\InvalidArgumentException $e) {
+			$output->writeln('<error>' . $e->getMessage() . '</error>');
+			return 1;
+		}
+
+		try {
+			$this->botServerMapper->findByUrl($url);
+			$output->writeln('<error>Bot with the same URL is already registered</error>');
+			return 2;
+		} catch (DoesNotExistException) {
+		}
+
 		$bot = new BotServer();
 		$bot->setName($name);
 		$bot->setSecret($secret);
@@ -108,8 +126,13 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 		try {
 			$this->botServerMapper->insert($bot);
 		} catch (\Exception $e) {
-			$output->writeln('<error>' . get_class($e) . ': ' . $e->getMessage() . '</error>');
-			return 1;
+			if ($e instanceof Exception && $e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+				$output->writeln('<error>Bot with the same secret is already registered</error>');
+				return 3;
+			} else {
+				$output->writeln('<error>' . get_class($e) . ': ' . $e->getMessage() . '</error>');
+				return 1;
+			}
 		}
 
 

lib/Command/Bot/Setup.php

@@ -33,6 +33,7 @@
 use OCA\Talk\Model\BotConversationMapper;
 use OCA\Talk\Model\BotServerMapper;
 use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\DB\Exception;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
@@ -81,7 +82,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 				$this->roomManager->getRoomByToken($token);
 			} catch (RoomNotFoundException) {
 				$output->writeln('<error>Conversation could not be found by token: ' . $token . '</error>');
-				return 1;
+				$returnCode = 2;
 			}
 
 			$bot = new BotConversation();
@@ -93,8 +94,13 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 				$this->botConversationMapper->insert($bot);
 				$output->writeln('<info>Successfully set up for conversation ' . $token . '</info>');
 			} catch (\Exception $e) {
-				$output->writeln('<error>' . get_class($e) . ': ' . $e->getMessage() . '</error>');
-				$returnCode = 3;
+				if ($e instanceof Exception && $e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+					$output->writeln('<error>Bot is already set up for the conversation ' . $token . '</error>');
+					$returnCode = 3;
+				} else {
+					$output->writeln('<error>' . get_class($e) . ': ' . $e->getMessage() . '</error>');
+					$returnCode = 4;
+				}
 			}
 		}
 

lib/Controller/BotController.php

@@ -91,7 +91,7 @@ protected function getBotFromHeaders(string $token, string $message): Bot {
 			throw new \InvalidArgumentException('Invalid Signature received from bot response', Http::STATUS_BAD_REQUEST);
 		}
 
-		$bots = $this->botService->getBotsForToken($token);
+		$bots = $this->botService->getBotsForToken($token, Bot::FEATURE_RESPONSE);
 		foreach ($bots as $botAttempt) {
 			try {
 				$this->checksumVerificationService->validateRequest(

lib/Listener/BotListener.php

@@ -42,6 +42,7 @@
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\EventDispatcher\IEventListener;
 use OCP\Server;
+use Psr\Log\LoggerInterface;
 
 /**
  * @template-implements IEventListener<Event>
@@ -50,6 +51,8 @@ class BotListener implements IEventListener {
 	public function __construct(
 		protected BotServerMapper $botServerMapper,
 		protected BotConversationMapper $botConversationMapper,
+		protected BotService $botService,
+		protected LoggerInterface $logger,
 	) {
 	}
 
@@ -87,13 +90,28 @@ public function handle(Event $event): void {
 	}
 
 	protected function handleBotInstallEvent(BotInstallEvent $event): void {
+		try {
+			$this->botService->validateBotParameters($event->getName(), $event->getSecret(), $event->getUrl(), $event->getDescription());
+		} catch (\InvalidArgumentException $e) {
+			$this->logger->error('Invalid data in bot install event', ['exception' => $e]);
+			throw $e;
+		}
+
 		try {
 			$bot = $this->botServerMapper->findByUrlAndSecret($event->getUrl(), $event->getSecret());
 
 			$bot->setName($event->getName());
 			$bot->setDescription($event->getDescription());
 			$this->botServerMapper->update($bot);
 		} catch (DoesNotExistException) {
+			try {
+				$this->botServerMapper->findByUrl($event->getUrl());
+				$e = new \InvalidArgumentException('Bot with the same URL and a different secret is already registered');
+				$this->logger->error('Invalid data in bot install event', ['exception' => $e]);
+				throw $e;
+			} catch (DoesNotExistException) {
+			}
+
 			$bot = new BotServer();
 			$bot->setName($event->getName());
 			$bot->setDescription($event->getDescription());

lib/Service/BotService.php

@@ -65,7 +65,7 @@ public function __construct(
 	}
 
 	public function afterChatMessageSent(ChatParticipantEvent $event, MessageParser $messageParser): void {
-		$bots = $this->getBotsForToken($event->getRoom()->getToken());
+		$bots = $this->getBotsForToken($event->getRoom()->getToken(), Bot::FEATURE_WEBHOOK);
 		if (empty($bots)) {
 			return;
 		}
@@ -107,7 +107,7 @@ public function afterChatMessageSent(ChatParticipantEvent $event, MessageParser
 	}
 
 	public function afterSystemMessageSent(ChatEvent $event, MessageParser $messageParser): void {
-		$bots = $this->getBotsForToken($event->getRoom()->getToken());
+		$bots = $this->getBotsForToken($event->getRoom()->getToken(), Bot::FEATURE_WEBHOOK);
 		if (empty($bots)) {
 			return;
 		}
@@ -246,9 +246,10 @@ protected function getActor(Room $room): array {
 
 	/**
 	 * @param string $token
+	 * @param int|null $requiredFeature
 	 * @return Bot[]
 	 */
-	public function getBotsForToken(string $token): array {
+	public function getBotsForToken(string $token, ?int $requiredFeature): array {
 		$botConversations = $this->botConversationMapper->findForToken($token);
 
 		if (empty($botConversations)) {
@@ -271,8 +272,8 @@ public function getBotsForToken(string $token): array {
 			}
 			$botServer = $serversMap[$botConversation->getBotId()];
 
-			if (!($botServer->getFeatures() & Bot::FEATURE_WEBHOOK)) {
-				$this->logger->debug('Not sending webhook to bot ID ' . $botConversation->getBotId() . ' because the feature is disabled for it');
+			if ($requiredFeature && !($botServer->getFeatures() & $requiredFeature)) {
+				$this->logger->debug('Ignoring bot ID ' . $botConversation->getBotId() . ' because the feature (' . $requiredFeature . ') is disabled for it');
 				continue;
 			}
 
@@ -288,4 +289,27 @@ public function getBotsForToken(string $token): array {
 
 		return $bots;
 	}
+
+	/**
+	 * @throws \InvalidArgumentException
+	 */
+	public function validateBotParameters(string $name, string $secret, string $url, string $description): void {
+		$nameLength = strlen($name);
+		if ($nameLength === 0 || $nameLength > 64) {
+			throw new \InvalidArgumentException('The provided name is too short or too long (min. 1 char, max. 64 chars)');
+		}
+		$secretLength = strlen($secret);
+		if ($secretLength < 40 || $secretLength > 128) {
+			throw new \InvalidArgumentException('The provided secret is too short (min. 40 chars, max. 128 chars)');
+		}
+
+		$url = filter_var($url);
+		if (!$url || strlen($url) > 4000 || !(str_starts_with($url, 'http://') || str_starts_with($url, 'https://'))) {
+			throw new \InvalidArgumentException('The provided URL is not a valid URL');
+		}
+
+		if (strlen($description) > 4000) {
+			throw new \InvalidArgumentException('The provided description is too long (max. 4000 chars)');
+		}
+	}
 }

tests/integration/features/bootstrap/CommandLineTrait.php

@@ -87,11 +87,24 @@ public function runOcc($args = [], $env = null) {
 	 * @Given /^invoking occ with "([^"]*)"$/
 	 */
 	public function invokingTheCommand($cmd) {
+		// FIXME this way is deprecated
 		if (preg_match('/room-name:(?P<token>\w+)/', $cmd, $matches)) {
 			if (array_key_exists($matches['token'], self::$identifierToToken)) {
 				$cmd = preg_replace('/room-name:(\w+)/', self::$identifierToToken[$matches['token']], $cmd);
 			}
 		}
+
+		if (preg_match('/ROOM\((?P<name>\w+)\)/', $cmd, $matches)) {
+			if (array_key_exists($matches['name'], self::$identifierToToken)) {
+				$cmd = preg_replace('/ROOM\((\w+)\)/', self::$identifierToToken[$matches['name']], $cmd);
+			}
+		}
+		if (preg_match('/BOT\((?P<name>\w+)\)/', $cmd, $matches)) {
+			if (array_key_exists($matches['name'], self::$botNameToId)) {
+				$cmd = preg_replace('/BOT\((\w+)\)/', self::$botNameToId[$matches['name']], $cmd);
+			}
+		}
+
 		$args = explode(' ', $cmd);
 		$this->runOcc($args);
 	}
@@ -131,7 +144,10 @@ public function theCommandWasSuccessful() {
 
 			$msg = 'The command was not successful, exit code was ' . $this->lastCode . '.';
 			if (!empty($exceptions)) {
-				$msg .= ' Exceptions: ' . implode(', ', $exceptions);
+				$msg .= "\n" . ' Exceptions: ' . implode(', ', $exceptions);
+			} else {
+				$msg .= "\n" . ' ' . $this->lastStdOut;
+				$msg .= "\n" . ' ' . $this->lastStdErr;
 			}
 			throw new \Exception($msg);
 		} elseif (!empty($exceptions)) {
@@ -143,7 +159,7 @@ public function theCommandWasSuccessful() {
 	/**
 	 * @Then /^the command failed with exit code ([0-9]+)$/
 	 */
-	public function theCommandFailedWithExitCode($exitCode) {
+	public function theCommandFailedWithExitCode(int $exitCode) {
 		Assert::assertEquals($exitCode, $this->lastCode, 'The commands exit code did not match');
 	}