From 3358883f00054f61f5cbbe8669f304e952b774d3 Mon Sep 17 00:00:00 2001 From: Lennart Dohmann Date: Wed, 27 Aug 2025 14:23:54 +0000 Subject: [PATCH] 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. --- Makefile | 6 +++--- lib/Controller/ScanController.php | 24 ++++++++++++++++++++- lib/EventListener/FileEventsListener.php | 27 ++++++++++++++++++------ lib/Service/ScanService.php | 11 +++++++++- lib/Service/VerdictService.php | 5 ----- 5 files changed, 57 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index 20cbadd5..156d598c 100644 --- a/Makefile +++ b/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" \ diff --git a/lib/Controller/ScanController.php b/lib/Controller/ScanController.php index 6e364aa7..d6316d3b 100644 --- a/lib/Controller/ScanController.php +++ b/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( diff --git a/lib/EventListener/FileEventsListener.php b/lib/EventListener/FileEventsListener.php index c150ca36..d454c5c7 100644 --- a/lib/EventListener/FileEventsListener.php +++ b/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; } diff --git a/lib/Service/ScanService.php b/lib/Service/ScanService.php index 8f116bf3..7590dcb7 100644 --- a/lib/Service/ScanService.php +++ b/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( diff --git a/lib/Service/VerdictService.php b/lib/Service/VerdictService.php index e4fc8036..b7c6638b 100644 --- a/lib/Service/VerdictService.php +++ b/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);