diff --git a/apps/files/lib/Controller/ConversionApiController.php b/apps/files/lib/Controller/ConversionApiController.php index 40ea45451ad09..afdc44073492b 100644 --- a/apps/files/lib/Controller/ConversionApiController.php +++ b/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,7 +76,7 @@ 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); @@ -80,6 +84,10 @@ public function convert(int $fileId, string $targetMimeType, ?string $destinatio 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.')); diff --git a/build/integration/file_conversions/file_conversions.feature b/build/integration/file_conversions/file_conversions.feature index f56dca0ebb49c..92dc11a647a33 100644 --- a/build/integration/file_conversions/file_conversions.feature +++ b/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 diff --git a/lib/private/Files/Conversion/ConversionManager.php b/lib/private/Files/Conversion/ConversionManager.php index e6ec11b1cf4bf..0a9803b3f421e 100644 --- a/lib/private/Files/Conversion/ConversionManager.php +++ b/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,6 +40,7 @@ class ConversionManager implements IConversionManager { /** @var list */ private array $providers = []; + private IL10N $l10n; public function __construct( private Coordinator $coordinator, private ContainerInterface $serverContainer, @@ -44,7 +48,9 @@ public function __construct( private ITempManager $tempManager, private LoggerInterface $logger, private SystemConfig $config, + IFactory $l10nFactory, ) { + $this->l10n = $l10nFactory->get('files'); } public function hasProviders(): bool { @@ -62,22 +68,21 @@ 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) { @@ -85,7 +90,7 @@ public function convert(File $file, string $targetMimeType, ?string $destination 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); }