From 44eed37bc365dff5fb47aee61b141b2a57fdf57b Mon Sep 17 00:00:00 2001 From: Matias Griese Date: Thu, 14 Apr 2022 17:05:30 +0300 Subject: [PATCH] Continue filename vs realname separation in media --- system/languages/en.yaml | 1 - .../Common/Media/Traits/MediaUploadTrait.php | 158 +++++++----------- .../Grav/Common/Page/Medium/AbstractMedia.php | 5 +- .../Grav/Common/Page/Medium/LocalMedia.php | 147 ++++++---------- 4 files changed, 110 insertions(+), 201 deletions(-) diff --git a/system/languages/en.yaml b/system/languages/en.yaml index 1fdb0049c..dd9e5241d 100644 --- a/system/languages/en.yaml +++ b/system/languages/en.yaml @@ -133,7 +133,6 @@ GRAV: FILE_COULD_NOT_BE_DELETED: "File could not be deleted: %s" FILE_COULD_NOT_BE_RENAMED: "File could not be renamed: %s" BAD_FILENAME: "Bad filename: %s" - BAD_DESTINATION: "Bad destination path" BAD_FILE_EXTENSION: "The file extension for the file '%s' is not an accepted" BAD_MIMETYPE: "The MIME type '%s' for the file '%s' is not an accepted" MIMETYPE_MISMATCH: "The mime '%s' type does not match the file %s''" diff --git a/system/src/Grav/Common/Media/Traits/MediaUploadTrait.php b/system/src/Grav/Common/Media/Traits/MediaUploadTrait.php index 23aadb267..e65ef1b47 100644 --- a/system/src/Grav/Common/Media/Traits/MediaUploadTrait.php +++ b/system/src/Grav/Common/Media/Traits/MediaUploadTrait.php @@ -16,7 +16,6 @@ use Grav\Common\Language\Language; use Grav\Common\Page\Medium\Medium; use Grav\Common\Page\Medium\MediumFactory; use Grav\Common\Utils; -use Grav\Framework\Filesystem\Filesystem; use Grav\Framework\Form\FormFlashFile; use Grav\Framework\Mime\MimeTypes; use Psr\Http\Message\UploadedFileInterface; @@ -109,6 +108,18 @@ trait MediaUploadTrait return $this->checkFileMetadata($metadata, $filename, $settings); } + /** + * @param string $filename + * @return void + * @throws RuntimeException + */ + public function checkFilename(string $filename): void + { + if (!Utils::checkFilename($filename)) { + throw new RuntimeException($this->translate('GRAV.MEDIA.BAD_FILENAME', $filename), 400); + } + } + /** * Checks that file metadata meets the requirements. Returns new filename. * @@ -132,18 +143,13 @@ trait MediaUploadTrait $filename = mb_strtolower(Utils::generateRandomString(15) . '.' . $extension); } - // Destination is always needed (but it can be set in defaults). - $path = $this->getDestination($settings); - // Handle conflicting filename if needed. - if ($settings['avoid_overwriting'] && $this->fileExists($filename, $path)) { + if ($settings['avoid_overwriting'] && $this->fileExists($filename)) { $filename = date('YmdHis') . '-' . $filename; } // Check if the filename is allowed. - if (!Utils::checkFilename($filename)) { - throw new RuntimeException($this->translate('GRAV.MEDIA.BAD_FILENAME', $filename), 400); - } + $this->checkFilename($filename); // Check if the file extension is allowed. $extension = mb_strtolower($extension); @@ -228,56 +234,50 @@ trait MediaUploadTrait public function copyUploadedFile(UploadedFileInterface $uploadedFile, string $filename, array $settings = null): void { try { - // Add the defaults to the settings. - if (!$filename) { - throw new RuntimeException($this->translate('GRAV.MEDIA.BAD_FILENAME', '(N/A)'), 400); - } - - $this->clearCache(); - - $filesystem = Filesystem::getInstance(false); + // Check if the filename is allowed. + $this->checkFilename($filename); // Calculate path without the retina scaling factor. - $basename = $filesystem->basename($filename); - $pathname = $filesystem->pathname($filename); + [$base, $ext,,] = $this->getFileParts($filename); + $name = "{$base}.{$ext}"; - // Get name for the uploaded file. - [$base, $ext,,] = $this->getFileParts($basename); - $name = "{$pathname}{$base}.{$ext}"; - - $path = $this->getDestination($settings); + $this->clearCache(); // Upload file. if ($uploadedFile instanceof FormFlashFile) { // FormFlashFile needs some additional logic. if ($uploadedFile->getError() === \UPLOAD_ERR_OK) { // Move uploaded file. - $this->doMoveUploadedFile($uploadedFile, $filename, $path); - } elseif (strpos($filename, 'original/') === 0 && !$this->fileExists($filename, $path) && $this->fileExists($basename, $path)) { + $this->doMoveUploadedFile($uploadedFile, $filename); + } + // TODO: Add retina image support back? + /* + elseif (strpos($filename, 'original/') === 0 && !$this->fileExists($filename, $path) && $this->fileExists($basename, $path)) { // Original image support: override original image if it's the same as the uploaded image. $this->doCopy($basename, $filename, $path); } + */ // FormFlashFile may also contain metadata. $metadata = $uploadedFile->getMetaData(); if ($metadata) { // TODO: This overrides metadata if used with multiple retina image sizes. - $this->doSaveMetadata(['upload' => $metadata], $name, $path); + $this->doSaveMetadata(['upload' => $metadata], $name); } } else { // Not a FormFlashFile. - $this->doMoveUploadedFile($uploadedFile, $filename, $path); + $this->doMoveUploadedFile($uploadedFile, $filename); } // Post-processing: Special content sanitization for SVG. $mime = Utils::getMimeByFilename($filename); if (Utils::contains($mime, 'svg', false)) { - $this->doSanitizeSvg($filename, $path); + $this->doSanitizeSvg($filename); } // Add the new file into the media. // TODO: This overrides existing media sizes if used with multiple retina image sizes. - $this->doAddUploadedMedium($name, $filename, $path); + $this->doAddUploadedMedium($name, $filename); // Update media index. if (method_exists($this, 'updateIndex')) { @@ -302,10 +302,8 @@ trait MediaUploadTrait public function deleteFile(string $filename, array $settings = null): void { try { - // First check for allowed filename. - if (!Utils::checkFilename($filename)) { - throw new RuntimeException($this->translate('GRAV.MEDIA.BAD_FILENAME', $filename), 400); - } + // Check if the filename is allowed. + $this->checkFilename($filename); // Get base name of the file. [$base, $ext,,] = $this->getFileParts($filename); @@ -314,9 +312,7 @@ trait MediaUploadTrait // Remove file and all the associated metadata. $this->clearCache(); - $path = $this->getDestination($settings); - - $this->doRemove($name, $path); + $this->doRemove($name); // Update media index. if (method_exists($this, 'updateIndex')) { @@ -339,23 +335,20 @@ trait MediaUploadTrait public function renameFile(string $from, string $to, array $settings = null): void { try { - $filesystem = Filesystem::getInstance(false); + // Check if the filename is allowed. + $this->checkFilename($from); + $this->checkFilename($to); $this->clearCache(); - // Get base name of the file. - $pathname = $filesystem->pathname($from); - // Remove @2x, @3x and .meta.yaml - [$base, $ext,,] = $this->getFileParts($filesystem->basename($from)); - $from = "{$pathname}{$base}.{$ext}"; + [$base, $ext,,] = $this->getFileParts($from); + $from = "{$base}.{$ext}"; - [$base, $ext,,] = $this->getFileParts($filesystem->basename($to)); - $to = "{$pathname}{$base}.{$ext}"; + [$base, $ext,,] = $this->getFileParts($to); + $to = "{$base}.{$ext}"; - $path = $this->getDestination($settings); - - $this->doRename($from, $to, $path); + $this->doRename($from, $to); // Update media index. if (method_exists($this, 'updateIndex')) { @@ -386,99 +379,60 @@ trait MediaUploadTrait return $settings; } - /** - * @param array|null $settings - * @return string - */ - protected function getDestination(?array $settings = null): string - { - $settings = $this->getUploadSettings($settings); - $path = $settings['destination'] ?? $this->getPath(); - if (!$path) { - throw new RuntimeException($this->translate('GRAV.MEDIA.BAD_DESTINATION'), 400); - } - - return $path; - } - /** * Internal logic to move uploaded file. * * @param UploadedFileInterface $uploadedFile * @param string $filename - * @param string $path */ - abstract protected function doMoveUploadedFile(UploadedFileInterface $uploadedFile, string $filename, string $path): void; + abstract protected function doMoveUploadedFile(UploadedFileInterface $uploadedFile, string $filename): void; /** * Internal logic to copy file. * * @param string $src * @param string $dst - * @param string $path */ - abstract protected function doCopy(string $src, string $dst, string $path): void; + abstract protected function doCopy(string $src, string $dst): void; /** * Internal logic to rename file. * * @param string $from * @param string $to - * @param string $path */ - abstract protected function doRename(string $from, string $to, string $path): void; + abstract protected function doRename(string $from, string $to): void; /** * Internal logic to remove file. * * @param string $filename - * @param string $path */ - abstract protected function doRemove(string $filename, string $path): void; + abstract protected function doRemove(string $filename): void; /** * @param string $filename - * @param string $path */ - abstract protected function doSanitizeSvg(string $filename, string $path): void; + abstract protected function doSanitizeSvg(string $filename): void; /** * @param array $metadata * @param string $filename - * @param string $path */ - protected function doSaveMetadata(array $metadata, string $filename, string $path): void + protected function doSaveMetadata(array $metadata, string $filename): void { - $filepath = sprintf('%s/%s', $path, $filename); - - // Do not use streams internally. - $locator = $this->getLocator(); - if ($locator->isStream($filepath)) { - $filepath = (string)$locator->findResource($filepath, true, true); - } - - $file = YamlFile::instance($filepath . '.meta.yaml'); + $filepath = $this->getPath($filename . '.meta.yaml'); + $file = YamlFile::instance($filepath); $file->save($metadata); } /** * @param string $filename - * @param string $path */ - protected function doRemoveMetadata(string $filename, string $path): void + protected function doRemoveMetadata(string $filename): void { - $filepath = sprintf('%s/%s', $path, $filename); - - // Do not use streams internally. - $locator = $this->getLocator(); - if ($locator->isStream($filepath)) { - $filepath = (string)$locator->findResource($filepath, true); - if (!$filepath) { - return; - } - } - - $file = YamlFile::instance($filepath . '.meta.yaml'); + $filepath = $this->getPath($filename . '.meta.yaml'); + $file = YamlFile::instance($filepath); if ($file->exists()) { $file->delete(); } @@ -487,14 +441,12 @@ trait MediaUploadTrait /** * @param string $name * @param string $filename - * @param string $path */ - protected function doAddUploadedMedium(string $name, string $filename, string $path): void + protected function doAddUploadedMedium(string $name, string $filename): void { - $filepath = sprintf('%s/%s', $path, $filename); + $filepath = $this->getRealPath($filename); $medium = $this->createFromFile($filepath); - $realpath = $path . '/' . $name; - $this->add($realpath, $medium); + $this->add($name, $medium); } /** @@ -511,6 +463,8 @@ trait MediaUploadTrait abstract protected function getPath(): ?string; + abstract protected function getRealPath(string $filename, array $info = null): string; + abstract protected function getGrav(): Grav; abstract protected function getLocator(): UniformResourceLocator; diff --git a/system/src/Grav/Common/Page/Medium/AbstractMedia.php b/system/src/Grav/Common/Page/Medium/AbstractMedia.php index 44ccdb36f..d4df80ef7 100644 --- a/system/src/Grav/Common/Page/Medium/AbstractMedia.php +++ b/system/src/Grav/Common/Page/Medium/AbstractMedia.php @@ -528,13 +528,12 @@ abstract class AbstractMedia implements ExportInterface, MediaCollectionInterfac /** * @param string $filename - * @param string $destination * @return bool */ - abstract protected function fileExists(string $filename, string $destination): bool; + abstract protected function fileExists(string $filename): bool; /** - * @param string $filepath + * @param string $filename * @return array */ abstract protected function readImageSize(string $filename, array $info = null): array; diff --git a/system/src/Grav/Common/Page/Medium/LocalMedia.php b/system/src/Grav/Common/Page/Medium/LocalMedia.php index 447ea4cef..d453288f8 100644 --- a/system/src/Grav/Common/Page/Medium/LocalMedia.php +++ b/system/src/Grav/Common/Page/Medium/LocalMedia.php @@ -9,12 +9,12 @@ namespace Grav\Common\Page\Medium; +use Exception; use FilesystemIterator; use Grav\Common\Data\Blueprint; use Grav\Common\Filesystem\Folder; use Grav\Common\Media\Interfaces\MediaObjectInterface; use Grav\Common\Security; -use Grav\Framework\Filesystem\Filesystem; use Psr\Http\Message\UploadedFileInterface; use RuntimeException; use function dirname; @@ -157,7 +157,7 @@ abstract class LocalMedia extends AbstractMedia public function readFile(string $filename, array $info = null): string { error_clear_last(); - $filepath = $this->getPath($filename); + $filepath = $this->getRealPath($filename); $contents = @file_get_contents($filepath); if (false === $contents) { throw new RuntimeException('Reading media file failed: ' . (error_get_last()['message'] ?? sprintf('Cannot read %s', $filename))); @@ -167,14 +167,14 @@ abstract class LocalMedia extends AbstractMedia } /** - * @param string $filepath + * @param string $filename * @return resource * @throws RuntimeException */ public function readStream(string $filename, array $info = null) { error_clear_last(); - $filepath = $this->getPath($filename); + $filepath = $this->getRealPath($filename); $contents = @fopen($filepath, 'rb'); if (false === $contents) { throw new RuntimeException('Reading media file failed: ' . (error_get_last()['message'] ?? sprintf('Cannot open %s', $filename))); @@ -185,12 +185,25 @@ abstract class LocalMedia extends AbstractMedia /** * @param string $filename - * @param string $destination * @return bool */ - protected function fileExists(string $filename, string $destination): bool + protected function fileExists(string $filename): bool { - return is_file("{$destination}/{$filename}"); + $filepath = $this->getRealPath($filename); + + return is_file($filepath); + } + + /** + * Internal method to get real path to the media file. + * + * @param string $filename + * @param array|null $info + * @return string + */ + protected function getRealPath(string $filename, array $info = null): string + { + return $this->getPath($filename); } /** @@ -200,17 +213,13 @@ abstract class LocalMedia extends AbstractMedia protected function readImageSize(string $filename, array $info = null): array { error_clear_last(); - $filepath = $this->getPath($filename); + $filepath = $this->getRealPath($filename); $sizes = @getimagesize($filepath); if (false === $sizes) { throw new RuntimeException(error_get_last()['message'] ?? 'Unable to get image size'); } - $sizes = [ - 'width' => $sizes[0], - 'height' => $sizes[1], - 'mime' => $sizes['mime'] - ]; + $sizes = ['width' => $sizes[0], 'height' => $sizes[1], 'mime' => $sizes['mime']]; // TODO: This is going to be slow without any indexing! /* @@ -254,36 +263,14 @@ abstract class LocalMedia extends AbstractMedia } /** - * @param array|null $settings - * @return string - */ - protected function getDestination(?array $settings = null): string - { - $settings = $this->getUploadSettings($settings); - $path = $settings['destination'] ?? $this->getPath(); - if (!$path) { - throw new RuntimeException($this->translate('GRAV.MEDIA.BAD_DESTINATION'), 400); - } - - return $path; - } - - /** - * Internal logic to move uploaded file. + * Internal logic to move uploaded file to the media folder. * * @param UploadedFileInterface $uploadedFile * @param string $filename - * @param string $path */ - protected function doMoveUploadedFile(UploadedFileInterface $uploadedFile, string $filename, string $path): void + protected function doMoveUploadedFile(UploadedFileInterface $uploadedFile, string $filename): void { - $filepath = sprintf('%s/%s', $path, $filename); - - // Do not use streams internally. - $locator = $this->getLocator(); - if ($locator->isStream($filepath)) { - $filepath = (string)$locator->findResource($filepath, true, true); - } + $filepath = $this->getRealPath($filename); Folder::create(dirname($filepath)); @@ -291,22 +278,15 @@ abstract class LocalMedia extends AbstractMedia } /** - * Internal logic to copy file. + * Internal logic to copy file within the same media folder. * * @param string $src * @param string $dst - * @param string $path */ - protected function doCopy(string $src, string $dst, string $path): void + protected function doCopy(string $src, string $dst): void { - $src = sprintf('%s/%s', $path, $src); - $dst = sprintf('%s/%s', $path, $dst); - - // Do not use streams internally. - $locator = $this->getLocator(); - if ($locator->isStream($dst)) { - $dst = (string)$locator->findResource($dst, true, true); - } + $src = $this->getRealPath($src); + $dst = $this->getRealPath($dst); Folder::create(dirname($dst)); @@ -318,76 +298,57 @@ abstract class LocalMedia extends AbstractMedia * * @param string $from * @param string $to - * @param string $path */ - protected function doRename(string $from, string $to, string $path): void + protected function doRename(string $from, string $to): void { - $fromPath = $path . '/' . $from; - - $locator = $this->getLocator(); - if ($locator->isStream($fromPath)) { - $fromPath = $locator->findResource($fromPath, true, true); - } - + $fromPath = $this->getRealPath($from); if (!is_file($fromPath)) { return; } - $mediaPath = dirname($fromPath); - $toPath = $mediaPath . '/' . $to; - if ($locator->isStream($toPath)) { - $toPath = $locator->findResource($toPath, true, true); - } + $toPath = $this->getRealPath($to); if (is_file($toPath)) { // TODO: translate error message - throw new RuntimeException(sprintf('%s already exists (%s)', $to, $mediaPath), 500); + throw new RuntimeException(sprintf('%s already exists', $to), 500); } $result = rename($fromPath, $toPath); if (!$result) { // TODO: translate error message - throw new RuntimeException(sprintf('%s -> %s (%s)', $from, $to, $mediaPath), 500); + throw new RuntimeException(sprintf('%s -> %s', $from, $to), 500); } - // TODO: Add missing logic to handle retina files. + // TODO: Add missing logic to handle retina files (move out of here!). + /* if (is_file($fromPath . '.meta.yaml')) { $result = rename($fromPath . '.meta.yaml', $toPath . '.meta.yaml'); if (!$result) { // TODO: translate error message - throw new RuntimeException(sprintf('Meta %s -> %s (%s)', $from, $to, $mediaPath), 500); + throw new RuntimeException(sprintf('Meta %s -> %s', $from, $to), 500); } } + */ } /** - * Internal logic to remove file. + * Internal logic to remove a file. * * @param string $filename - * @param string $path + * @return void */ - protected function doRemove(string $filename, string $path): void + protected function doRemove(string $filename): void { - $filesystem = Filesystem::getInstance(false); - - $locator = $this->getLocator(); - $folder = $locator->isStream($path) ? (string)$locator->findResource($path, true, true) : $path; - - // If path doesn't exist, there's nothing to do. - $pathname = $filesystem->pathname($filename); - $targetPath = rtrim(sprintf('%s/%s', $folder, $pathname), '/'); - if (!is_dir($targetPath)) { - return; - } - - // Remove requested media file. - if ($this->fileExists($filename, $path)) { - $result = unlink("{$folder}/{$filename}"); + $filepath = $this->getRealPath($filename); + if ($this->fileExists($filepath)) { + $result = unlink($filepath); if (!$result) { - throw new RuntimeException($this->translate('PLUGIN_ADMIN.FILE_COULD_NOT_BE_DELETED') . ': ' . $filename, 500); + throw new RuntimeException($filename, 500); } } + // TODO: move this out of here! + /* // Remove associated metadata. $this->doRemoveMetadata($filename, $path); @@ -420,21 +381,17 @@ abstract class LocalMedia extends AbstractMedia } } } + */ } /** * @param string $filename - * @param string $path + * @return void + * @throws Exception */ - protected function doSanitizeSvg(string $filename, string $path): void + protected function doSanitizeSvg(string $filename): void { - $filepath = sprintf('%s/%s', $path, $filename); - - // Do not use streams internally. - $locator = $this->getLocator(); - if ($locator->isStream($filepath)) { - $filepath = (string)$locator->findResource($filepath, true, true); - } + $filepath = $this->getRealPath($filename); Security::sanitizeSVG($filepath); }