Outsource the consequences of a file scan from tag function, as this is not just tagging and caused a bug in the event handler where the quarantine was created empty.

Author: lennartdohmann

Makefile

@@ -15,7 +15,7 @@ all: build
 
 # Fetches dependencies and builds it
 .PHONY: build
-build:
+build: oc
 ifneq (,$(wildcard $(CURDIR)/composer.json))
 	make composer
 endif
@@ -36,7 +36,7 @@ endif
 # Installs and updates the composer dependencies. If composer is not installed
 # a copy is fetched from the web
 .PHONY: composer
-composer:
+composer: oc
 ifeq (,$(composer))
 	@echo "No composer command available, downloading a copy from the web"
 	mkdir -p $(build_tools_directory)
@@ -95,7 +95,6 @@ distclean: clean
 	rm -rf node_modules
 	rm -rf js/vendor
 	rm -rf js/node_modules
-	rm -rf nextcloud-server
 	rm -rf build
 	rm -f composer.lock
 
@@ -120,6 +119,7 @@ appstore: build
 	--exclude-vcs \
 	--exclude="$(source_build_directory)/.git" \
 	--exclude="$(source_build_directory)/.github" \
+	--exclude="$(source_build_directory)/nextcloud-server" \
 	--exclude="$(source_build_directory)/composer.json" \
 	--exclude="$(source_build_directory)/composer.json.license" \
 	--exclude="$(source_build_directory)/babel.config.js" \

lib/Controller/ScanController.php

@@ -8,6 +8,7 @@
 
 use Exception;
 use OCA\GDataVaas\AppInfo\Application;
+use OCA\GDataVaas\Service\FileService;
 use OCA\GDataVaas\Service\VerdictService;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
@@ -17,17 +18,30 @@
 use OCP\Files\NotPermittedException;
 use OCP\IAppConfig;
 use OCP\IRequest;
+use Psr\Log\LoggerInterface;
 use VaasSdk\Exceptions\VaasAuthenticationException;
+use VaasSdk\Verdict;
 
 class ScanController extends Controller {
+	private readonly LoggerInterface $logger;
 	private IAppConfig $config;
 	private VerdictService $verdictService;
+	private FileService $fileService;
 
-	public function __construct($appName, IRequest $request, VerdictService $verdictService, IAppConfig $config) {
+	public function __construct(
+		$appName,
+		IRequest $request,
+		VerdictService $verdictService,
+		IAppConfig $config,
+		FileService $fileService,
+		LoggerInterface $logger,
+	) {
 		parent::__construct($appName, $request);
 
+		$this->logger = $logger;
 		$this->config = $config;
 		$this->verdictService = $verdictService;
+		$this->fileService = $fileService;
 	}
 
 	/**
@@ -39,6 +53,14 @@ public function __construct($appName, IRequest $request, VerdictService $verdict
 	public function scan(int $fileId): JSONResponse {
 		try {
 			$verdict = $this->verdictService->scanFileById($fileId);
+			if ($verdict->verdict === Verdict::MALICIOUS) {
+				try {
+					$this->fileService->setMaliciousPrefixIfActivated($fileId);
+					$this->fileService->moveFileToQuarantineFolderIfDefined($fileId);
+				} catch (Exception $e) {
+					$this->logger->error("Failed to handle malicious file '{$fileId}': {$e->getMessage()}");
+				}
+			}
 			return new JSONResponse(['verdict' => $verdict->verdict->value], 200);
 		} catch (EntityTooLargeException) {
 			return new JSONResponse(

lib/EventListener/FileEventsListener.php

@@ -80,12 +80,27 @@ public function handle(Event $event): void {
 			}
 
 			if ($verdict->verdict->value == TagService::MALICIOUS) {
-				$this->sendErrorResponse(new VirusFoundException($verdict, $node->getName(), $node->getId()));
-				$this->fileService->deleteFile($node->getId());
-				if ($this->appConfig->getValueBool(Application::APP_ID, 'sendMailOnVirusUpload')) {
-					$this->mailService->notifyMaliciousUpload(
-						$verdict, $node->getPath(), $this->userSession->getUser()->getUID(), $node->getSize()
-					);
+				try {
+					$this->sendErrorResponse(new VirusFoundException($verdict, $node->getName(), $node->getId()));
+					$this->fileService->deleteFile($node->getId());
+					if ($this->appConfig->getValueBool(Application::APP_ID, 'sendMailOnVirusUpload')) {
+						$this->mailService->notifyMaliciousUpload(
+							$verdict, $node->getPath(), $this->userSession->getUser()->getUID(), $node->getSize()
+						);
+					}
+				} catch (Exception $e) {
+					try {
+						$this->fileService->setMaliciousPrefixIfActivated($node->getId());
+						$this->fileService->moveFileToQuarantineFolderIfDefined($node->getId());
+						$this->logger->error(
+							"Failed to block upload of malicious file '{$node->getName()}'.
+							File was quarantined instead. Error: {$e->getMessage()}"
+						);
+					} catch (Exception $e) {
+						$this->logger->error(
+							"Failed to set malicious prefix or move file to quarantine for '{$node->getName()}'. Error: {$e->getMessage()}"
+						);
+					}
 				}
 				exit;
 			}

lib/Service/ScanService.php

@@ -14,6 +14,7 @@
 use OCP\IAppConfig;
 use Psr\Log\LoggerInterface;
 use VaasSdk\Exceptions\VaasAuthenticationException;
+use VaasSdk\Verdict;
 
 class ScanService {
 
@@ -60,7 +61,15 @@ public function run(): int {
 		$fileIds = $this->getFileIdsToScan();
 		foreach ($fileIds as $fileId) {
 			try {
-				$this->verdictService->scanFileById($fileId);
+				$verdict = $this->verdictService->scanFileById($fileId);
+				if ($verdict->verdict === Verdict::MALICIOUS) {
+					try {
+						$this->fileService->setMaliciousPrefixIfActivated($fileId);
+						$this->fileService->moveFileToQuarantineFolderIfDefined($fileId);
+					} catch (Exception $e) {
+						$this->logger->error("Failed to handle malicious file '{$fileId}': {$e->getMessage()}");
+					}
+				}
 				$scanned += 1;
 			} catch (EntityTooLargeException) {
 				$this->logger->error(

lib/Service/VerdictService.php

@@ -246,11 +246,6 @@ private function tagFile(int $fileId, string $tagName): void {
 		switch ($tagName) {
 			case TagService::MALICIOUS:
 				$this->tagService->setTag($fileId, TagService::MALICIOUS, silent: false);
-				try {
-					$this->fileService->setMaliciousPrefixIfActivated($fileId);
-					$this->fileService->moveFileToQuarantineFolderIfDefined($fileId);
-				} catch (Exception) {
-				}
 				break;
 			case TagService::PUP:
 				$this->tagService->setTag($fileId, TagService::PUP, silent: false);