Merge pull request #334 from nextcloud/icap-improvements

Scanner improvements

appinfo/info.xml

@@ -14,7 +14,7 @@
 This application inspects files that are uploaded to Nextcloud for viruses before they are written to the Nextcloud storage. If a file is identified as a virus, it is either logged or not uploaded to the server. The application relies on the underlying ClamAV virus scanning engine, which the admin points Nextcloud to when configuring the application. Alternatively, a Kaspersky Scan Engine can be configured, which has to run on a separate server.
 For this app to be effective, the ClamAV virus definitions should be kept up to date. Also note that enabling this app will impact system performance as additional processing is required for every upload. More information is available in the Antivirus documentation.
 	]]></description>
-	<version>5.5.1</version>
+	<version>5.5.2</version>
 	<licence>agpl</licence>
 
 	<author>Manuel Delgado</author>

lib/AppInfo/Application.php

@@ -67,6 +67,7 @@ function (string $mountPoint, IStorage $storage) {
 					'isHomeStorage' => $storage->instanceOfStorage(IHomeStorage::class),
 					'eventDispatcher' => $eventDispatcher,
 					'trashEnabled' => $appManager->isEnabledForUser('files_trashbin'),
+					'mount_point' => $mountPoint,
 				]);
 			},
 			1

lib/AvirWrapper.php

@@ -23,30 +23,16 @@
 class AvirWrapper extends Wrapper {
 	/**
 	 * Modes that are used for writing
-	 * @var array
 	 */
-	private $writingModes = ['r+', 'w', 'w+', 'a', 'a+', 'x', 'x+', 'c', 'c+'];
-
-	/** @var ScannerFactory */
-	protected $scannerFactory;
-
-	/** @var IL10N */
-	protected $l10n;
-
-	/** @var LoggerInterface */
-	protected $logger;
-
-	/** @var ActivityManager */
-	protected $activityManager;
-
-	/** @var bool */
-	protected $isHomeStorage;
-
-	/** @var bool */
-	private $shouldScan = true;
-
-	/** @var bool */
-	private $trashEnabled;
+	private array $writingModes = ['r+', 'w', 'w+', 'a', 'a+', 'x', 'x+', 'c', 'c+'];
+	protected ScannerFactory$scannerFactory;
+	protected IL10N $l10n;
+	protected LoggerInterface$logger;
+	protected ActivityManager $activityManager;
+	protected bool $isHomeStorage;
+	private bool $shouldScan = true;
+	private bool $trashEnabled;
+	private string $mountPoint;
 
 	/**
 	 * @param array $parameters
@@ -59,6 +45,7 @@ public function __construct($parameters) {
 		$this->activityManager = $parameters['activityManager'];
 		$this->isHomeStorage = $parameters['isHomeStorage'];
 		$this->trashEnabled = $parameters['trashEnabled'];
+		$this->mountPoint = $parameters['mount_point'];
 
 		/** @var IEventDispatcher $eventDispatcher */
 		$eventDispatcher = $parameters['eventDispatcher'];
@@ -106,7 +93,7 @@ private function shouldWrap(string $path): bool {
 
 	private function wrapSteam(string $path, $stream) {
 		try {
-			$scanner = $this->scannerFactory->getScanner();
+			$scanner = $this->scannerFactory->getScanner($this->mountPoint . $path);
 			$scanner->initScanner();
 			return CallbackReadDataWrapper::wrap(
 				$stream,

lib/BackgroundJob/BackgroundScanner.php

@@ -278,7 +278,7 @@ protected function scanOneFile(File $file): void {
 		$this->eventDispatcher->dispatchTyped(new BeforeBackgroundScanEvent($file));
 
 		$item = $this->itemFactory->newItem($file, true);
-		$scanner = $this->scannerFactory->getScanner();
+		$scanner = $this->scannerFactory->getScanner($file->getPath());
 		$status = $scanner->scan($item);
 		$status->dispatch($item);
 	}

lib/Command/Scan.php

@@ -73,7 +73,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 			return 3;
 		}
 
-		$scanner = $this->scannerFactory->getScanner();
+		$scanner = $this->scannerFactory->getScanner($node->getPath());
 		if ($input->getOption('debug')) {
 			$scanner->setDebugCallback(function ($content) use ($output) {
 				$output->writeln($content);

lib/Command/Test.php

@@ -60,7 +60,7 @@ protected function configure() {
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$output->write("Scanning regular text: ");
-		$scanner = $this->scannerFactory->getScanner();
+		$scanner = $this->scannerFactory->getScanner('/foo.txt');
 		if ($input->getOption('debug')) {
 			$output->writeln("");
 			$scanner->setDebugCallback(function ($content) use ($output) {
@@ -77,7 +77,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 		}
 
 		$output->write("Scanning EICAR test file: ");
-		$scanner = $this->scannerFactory->getScanner();
+		$scanner = $this->scannerFactory->getScanner('/test-virus-eicar.txt');
 		if ($input->getOption('debug')) {
 			$output->writeln("");
 			$scanner->setDebugCallback(function ($content) use ($output) {
@@ -94,6 +94,25 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 			$output->writeln("<info>✓</info>");
 		}
 
+		// send a modified version of the EICAR because some scanners don't hold the scan request
+		// by default for files that haven't been seen before.
+		$output->write("Scanning modified EICAR test file: ");
+		$scanner = $this->scannerFactory->getScanner('/test-virus-eicar-modified.txt');
+		if ($input->getOption('debug')) {
+			$output->writeln("");
+			$scanner->setDebugCallback(function ($content) use ($output) {
+				$output->writeln($content);
+			});
+		}
+		$result = $scanner->scanString($eicar . uniqid());
+		if ($result->getNumericStatus() !== Status::SCANRESULT_INFECTED) {
+			$details = $result->getDetails();
+			$output->writeln("<error>❌ file not detected $details</error>");
+			return 1;
+		} else {
+			$output->writeln("<info>✓</info>");
+		}
+
 		return 0;
 	}
 }

lib/Controller/SettingsController.php

@@ -87,7 +87,7 @@ public function save(
 		$this->settings->setAvIcapTls((bool)$avIcapTls);
 
 		try {
-			$scanner = $this->scannerFactory->getScanner();
+			$scanner = $this->scannerFactory->getScanner('/self-test.txt');
 			$result = $scanner->scanString("dummy scan content");
 			$success = $result->getNumericStatus() == Status::SCANRESULT_CLEAN;
 			$message = $success ? $this->l10n->t('Saved') : 'unexpected scan results for test content';

lib/Scanner/ICAP.php

@@ -72,19 +72,23 @@ public function __construct(
 	public function initScanner() {
 		parent::initScanner();
 		$this->writeHandle = fopen("php://temp", 'w+');
+		$path = '/' . trim($this->path, '/');
+		if (str_contains($path, '.ocTransferId') && str_ends_with($path, '.part')) {
+			[$path] = explode('.ocTransferId', $path, 2);
+		}
 		if ($this->mode === ICAPClient::MODE_REQ_MOD) {
 			$this->request = $this->icapClient->reqmod($this->service, [
 				'Allow' => 204,
 			], [
-				"PUT / HTTP/1.0",
-				"Host: 127.0.0.1"
+				"PUT $path HTTP/1.0",
+				"Host: nextcloud"
 			]);
 		} else {
 			$this->request = $this->icapClient->respmod($this->service, [
 				'Allow' => 204,
 			], [
-				"GET / HTTP/1.0",
-				"Host: 127.0.0.1"
+				"GET $path HTTP/1.0",
+				"Host: nextcloud"
 			], [
 				"HTTP/1.0 200 OK",
 				"Content-Length: 1", // a dummy, non-zero, content length seems to be enough

lib/Scanner/ScannerBase.php

@@ -52,6 +52,7 @@ abstract class ScannerBase implements IScanner {
 	protected ?string $lastChunk = null;
 	protected bool $isLogUsed = false;
 	protected bool $isAborted = false;
+	protected string $path = '';
 
 	public function __construct(AppConfig $config, LoggerInterface $logger, StatusFactory $statusFactory) {
 		$this->appConfig = $config;
@@ -244,4 +245,8 @@ protected function prepareChunk($data) {
 	public function setDebugCallback(callable $callback): void {
 		// unsupported
 	}
+
+	public function setPath(string $path): void {
+		$this->path = $path;
+	}
 }

lib/Scanner/ScannerFactory.php

@@ -25,7 +25,7 @@ public function __construct(AppConfig $appConfig, IServerContainer $serverContai
 	 *
 	 * @return IScanner
 	 */
-	public function getScanner() {
+	public function getScanner(string $path) {
 		$avMode = $this->appConfig->getAvMode();
 		switch ($avMode) {
 			case 'daemon':
@@ -45,8 +45,9 @@ public function getScanner() {
 				throw new \InvalidArgumentException('Application is misconfigured. Please check the settings at the admin page. Invalid mode: ' . $avMode);
 		}
 
-		/** @var IScanner $scanner */
+		/** @var ScannerBase $scanner */
 		$scanner = $this->serverContainer->resolve($scannerClass);
+		$scanner->setPath($path);
 		return $scanner;
 	}
 }

tests/AvirWrapperTest.php

@@ -77,6 +77,7 @@ protected function setUp(): void {
 			'isHomeStorage' => true,
 			'eventDispatcher' => $this->createMock(EventDispatcherInterface::class),
 			'trashEnabled' => true,
+			'mount_point' => '/' . self::UID . '/files/',
 		]);
 
 		$this->config->expects($this->any())