fix(files): more conversion tests and translate error messages

Signed-off-by: skjnldsv <[email protected]>

apps/files/lib/Controller/ConversionApiController.php

@@ -10,17 +10,20 @@
 namespace OCA\Files\Controller;
 
 use OC\Files\Utils\PathHelper;
+use OC\ForbiddenException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\ApiRoute;
 use OCP\AppFramework\Http\Attribute\NoAdminRequired;
 use OCP\AppFramework\Http\Attribute\UserRateLimit;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\AppFramework\OCS\OCSBadRequestException;
 use OCP\AppFramework\OCS\OCSException;
 use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
 use OCP\Files\Conversion\IConversionManager;
 use OCP\Files\File;
+use OCP\Files\GenericFileException;
 use OCP\Files\IRootFolder;
 use OCP\IL10N;
 use OCP\IRequest;
@@ -59,7 +62,8 @@ public function convert(int $fileId, string $targetMimeType, ?string $destinatio
 		$userFolder = $this->rootFolder->getUserFolder($this->userId);
 		$file = $userFolder->getFirstNodeById($fileId);
 
-		if (!($file instanceof File)) {
+		// Also throw a 404 if the file is not readable to not leak information
+		if (!($file instanceof File) || $file->isReadable() === false) {
 			throw new OCSNotFoundException($this->l10n->t('The file cannot be found'));
 		}
 
@@ -72,14 +76,18 @@ public function convert(int $fileId, string $targetMimeType, ?string $destinatio
 			}
 
 			if (!$userFolder->get($parentDir)->isCreatable()) {
-				throw new OCSForbiddenException();
+				throw new OCSForbiddenException($this->l10n->t('You do not have permission to create a file at the specified location'));
 			}
 
 			$destination = $userFolder->getFullPath($destination);
 		}
 
 		try {
 			$convertedFile = $this->fileConversionManager->convert($file, $targetMimeType, $destination);
+		} catch (ForbiddenException $e) {
+			throw new OCSForbiddenException($e->getMessage());
+		} catch (GenericFileException $e) {
+			throw new OCSBadRequestException($e->getMessage());
 		} catch (\Exception $e) {
 			logger('files')->error($e->getMessage(), ['exception' => $e]);
 			throw new OCSException($this->l10n->t('The file could not be converted.'));

build/integration/file_conversions/file_conversions.feature

@@ -76,6 +76,27 @@ Feature: conversions
     Then as "user0" the file "/image.jpg" exists
     Then as "user0" the file "/image.png" does not exist
 
+Scenario: Converting a file to a given path without extension fails
+    Given user "user0" uploads file "data/clouds.jpg" to "/image.jpg"
+    And User "user0" created a folder "/folder"
+    Then as "user0" the file "/image.jpg" exists
+    Then as "user0" the folder "/folder" exists
+    When user "user0" converts file "/image.jpg" to "image/png" and saves it to "/folder/image"
+    Then the HTTP status code should be "400"
+    Then the OCS status code should be "400"
+    Then as "user0" the file "/folder/image.png" does not exist
+    Then as "user0" the file "/image.png" does not exist
+
+  @local_storage
+  Scenario: Converting a file bigger than 100 MiB fails
+    Given file "/image.jpg" of size 108003328 is created in local storage
+    Then as "user0" the folder "/local_storage" exists
+    Then as "user0" the file "/local_storage/image.jpg" exists
+    When user "user0" converts file "/local_storage/image.jpg" to "image/png" and saves it to "/image.png"
+    Then the HTTP status code should be "400"
+    Then the OCS status code should be "400"
+    Then as "user0" the file "/image.png" does not exist
+
   Scenario: Forbid conversion to a destination without create permission
     Given user "user1" exists
     # Share the folder with user1

lib/private/Files/Conversion/ConversionManager.php

@@ -10,13 +10,16 @@
 namespace OC\Files\Conversion;
 
 use OC\AppFramework\Bootstrap\Coordinator;
+use OC\ForbiddenException;
 use OC\SystemConfig;
 use OCP\Files\Conversion\IConversionManager;
 use OCP\Files\Conversion\IConversionProvider;
 use OCP\Files\File;
 use OCP\Files\GenericFileException;
 use OCP\Files\IRootFolder;
+use OCP\IL10N;
 use OCP\ITempManager;
+use OCP\L10N\IFactory;
 use OCP\PreConditionNotMetException;
 use Psr\Container\ContainerExceptionInterface;
 use Psr\Container\ContainerInterface;
@@ -37,14 +40,17 @@ class ConversionManager implements IConversionManager {
 	/** @var list<IConversionProvider> */
 	private array $providers = [];
 
+	private IL10N $l10n;
 	public function __construct(
 		private Coordinator $coordinator,
 		private ContainerInterface $serverContainer,
 		private IRootFolder $rootFolder,
 		private ITempManager $tempManager,
 		private LoggerInterface $logger,
 		private SystemConfig $config,
+		IFactory $l10nFactory,
 	) {
+		$this->l10n = $l10nFactory->get('files');
 	}
 
 	public function hasProviders(): bool {
@@ -62,30 +68,29 @@ public function getProviders(): array {
 
 	public function convert(File $file, string $targetMimeType, ?string $destination = null): string {
 		if (!$this->hasProviders()) {
-			throw new PreConditionNotMetException('No file conversion providers available');
+			throw new PreConditionNotMetException($this->l10n->t('No file conversion providers available'));
 		}
 
 		// Operate in mebibytes
 		$fileSize = $file->getSize() / (1024 * 1024);
 		$threshold = $this->config->getValue('max_file_conversion_filesize', 100);
 		if ($fileSize > $threshold) {
-			throw new GenericFileException('File is too large to convert');
+			throw new GenericFileException($this->l10n->t('File is too large to convert'));
 		}
 
 		$fileMimeType = $file->getMimetype();
 		$validProvider = $this->getValidProvider($fileMimeType, $targetMimeType);
 
 		if ($validProvider !== null) {
-			$convertedFile = $validProvider->convertFile($file, $targetMimeType);
-
+			// Get the target extension given by the provider
 			$targetExtension = '';
 			foreach ($validProvider->getSupportedMimeTypes() as $mimeProvider) {
 				if ($mimeProvider->getTo() === $targetMimeType) {
 					$targetExtension = $mimeProvider->getExtension();
 					break;
 				}
 			}
-
+	
 			// If destination not provided, we use the same path
 			// as the original file, but with the new extension
 			if ($destination === null) {
@@ -94,11 +99,21 @@ public function convert(File $file, string $targetMimeType, ?string $destination
 				$destination = $parent->getFullPath($basename . '.' . $targetExtension);
 			}
 
+			// If destination doesn't match the target extension, we throw an error
+			if (pathinfo($destination, PATHINFO_EXTENSION) !== $targetExtension) {
+				throw new GenericFileException($this->l10n->t('Destination does not match conversion extension'));
+			}
+
+			// Check destination before converting
+			$this->checkDestination($destination);
+
+			// Convert the file and write it to the destination
+			$convertedFile = $validProvider->convertFile($file, $targetMimeType);
 			$convertedFile = $this->writeToDestination($destination, $convertedFile);
 			return $convertedFile->getPath();
 		}
 
-		throw new RuntimeException('Could not convert file');
+		throw new RuntimeException($this->l10n->t('Could not convert file'));
 	}
 
 	/**
@@ -127,14 +142,25 @@ private function getRegisteredProviders(): array {
 		return array_values(array_merge([], $this->preferredProviders, $this->providers));
 	}
 
+	private function checkDestination(string $destination): void {
+		if (!$this->rootFolder->nodeExists(dirname($destination))) {
+			throw new ForbiddenException($this->l10n->t('Destination does not exist'));
+		}
+
+		$folder = $this->rootFolder->get(dirname($destination));
+		if (!$folder->isCreatable()) {
+			throw new ForbiddenException($this->l10n->t('Destination is not creatable'));
+		}
+	}
+
 	private function writeToDestination(string $destination, mixed $content): File {
+		$this->checkDestination($destination);
+
 		if ($this->rootFolder->nodeExists($destination)) {
 			$file = $this->rootFolder->get($destination);
 			$parent = $file->getParent();
-			if (!$parent->isCreatable()) {
-				throw new GenericFileException('Destination is not creatable');
-			}
 
+			// Folder permissions is already checked in checkDestination method
 			$newName = $parent->getNonExistingName(basename($destination));
 			$destination = $parent->getFullPath($newName);
 		}