From 1cbb2a365e9d361e628933027781b44713fb3738 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Thu, 9 Apr 2020 22:45:49 +0200 Subject: [PATCH 1/2] Validate PSR-compatible file upload Instead of converting the uploaded file object to an UploadedFile instance from Symfony, because the latter is compatible with Laravel's validation, let's re-implement the validation for the three rules we were using. The benefit: we can now avoid copying the uploaded file to a temporary location just to do the wrapping. In the next step, we will remove the temporary file and let the uploader / Intervention Image handle the PSR stream directly. --- src/User/AvatarValidator.php | 76 +++++++++++++++++++++--- src/User/Command/UploadAvatarHandler.php | 11 +--- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/src/User/AvatarValidator.php b/src/User/AvatarValidator.php index 49addf194..19ee3ca0e 100644 --- a/src/User/AvatarValidator.php +++ b/src/User/AvatarValidator.php @@ -10,14 +10,76 @@ namespace Flarum\User; use Flarum\Foundation\AbstractValidator; +use Flarum\Foundation\ValidationException; +use Psr\Http\Message\UploadedFileInterface; +use Symfony\Component\Mime\MimeTypes; class AvatarValidator extends AbstractValidator { - protected $rules = [ - 'avatar' => [ - 'required', - 'mimes:jpeg,png,bmp,gif', - 'max:2048' - ] - ]; + /** + * Throw an exception if a model is not valid. + * + * @param array $attributes + */ + public function assertValid(array $attributes) + { + $this->assertFileRequired($attributes['avatar']); + $this->assertFileMimes($attributes['avatar']); + $this->assertFileSize($attributes['avatar']); + } + + protected function assertFileRequired(UploadedFileInterface $file) + { + if ($file->getError() !== UPLOAD_ERR_OK) { + $this->raise('required'); + } + } + + protected function assertFileMimes(UploadedFileInterface $file) + { + $allowedTypes = $this->getAllowedTypes(); + + // Block PHP files masquerading as images + $phpExtensions = ['php', 'php3', 'php4', 'php5', 'phtml']; + $fileExtension = pathinfo($file->getClientFilename(), PATHINFO_EXTENSION); + + if (in_array(trim(strtolower($fileExtension)), $phpExtensions)) { + $this->raise('mimes', [':values' => implode(', ', $allowedTypes)]); + } + + $guessedExtension = MimeTypes::getDefault()->getExtensions($file->getClientMediaType())[0] ?? null; + + if (! in_array($guessedExtension, $allowedTypes)) { + $this->raise('mimes', [':values' => implode(', ', $allowedTypes)]); + } + } + + protected function assertFileSize(UploadedFileInterface $file) + { + $maxSize = $this->getMaxSize(); + + if ($file->getSize() / 1024 > $maxSize) { + $this->raise('max.file', [':max' => $maxSize]); + } + } + + protected function raise($error, array $parameters = []) + { + $message = $this->translator->trans( + "validation.$error", + $parameters + [':attribute' => 'avatar'] + ); + + throw new ValidationException(['avatar' => $message]); + } + + protected function getMaxSize() + { + return 2048; + } + + protected function getAllowedTypes() + { + return ['jpeg', 'png', 'bmp', 'gif']; + } } diff --git a/src/User/Command/UploadAvatarHandler.php b/src/User/Command/UploadAvatarHandler.php index 02e9e04c2..1803c4cc5 100644 --- a/src/User/Command/UploadAvatarHandler.php +++ b/src/User/Command/UploadAvatarHandler.php @@ -18,7 +18,6 @@ use Flarum\User\Event\AvatarSaving; use Flarum\User\UserRepository; use Illuminate\Contracts\Events\Dispatcher; use Intervention\Image\ImageManager; -use Symfony\Component\HttpFoundation\File\UploadedFile; class UploadAvatarHandler { @@ -65,6 +64,7 @@ class UploadAvatarHandler * @param UploadAvatar $command * @return \Flarum\User\User * @throws \Flarum\User\Exception\PermissionDeniedException + * @throws \Flarum\Foundation\ValidationException */ public function handle(UploadAvatar $command) { @@ -82,15 +82,6 @@ class UploadAvatarHandler $file->moveTo($tmpFile); try { - $file = new UploadedFile( - $tmpFile, - $file->getClientFilename(), - $file->getClientMediaType(), - $file->getSize(), - $file->getError(), - true - ); - $this->validator->assertValid(['avatar' => $file]); $image = (new ImageManager)->make($tmpFile); From 1fa37a7a6a755abd4686994ef0bb5f0b432f45ec Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Fri, 3 Apr 2020 16:47:55 +0200 Subject: [PATCH 2/2] Simplify uploads, inject filesystem instances This avoids injecting the Application god class and assembling default file locations in multiple places. In addition, we no longer use the `MountManager` for these uploads. It only added complexity (by moving tmp files around) and will not be available in the next major release of Flysystem. Note: Passing PSR upload streams to Intervention Image requires an explicit upgrade of the library. (Very likely, users have already updated to the newer versions, as the old constraint allowed it, but we should be explicit for correctness' sake.) --- composer.json | 2 +- .../Controller/DeleteFaviconController.php | 19 +++++----- src/Api/Controller/DeleteLogoController.php | 19 +++++----- .../Controller/UploadFaviconController.php | 36 +++++++------------ src/Api/Controller/UploadLogoController.php | 31 ++++++---------- src/Settings/SettingsServiceProvider.php | 20 +++++++++++ src/User/Command/UploadAvatarHandler.php | 33 +++++------------ 7 files changed, 68 insertions(+), 92 deletions(-) diff --git a/composer.json b/composer.json index 40bde9c5b..2ecc9ec07 100644 --- a/composer.json +++ b/composer.json @@ -56,7 +56,7 @@ "illuminate/support": "5.7.*", "illuminate/validation": "5.7.*", "illuminate/view": "5.7.*", - "intervention/image": "^2.3.0", + "intervention/image": "^2.5.0", "laminas/laminas-diactoros": "^1.8.4", "laminas/laminas-httphandlerrunner": "^1.0", "laminas/laminas-stratigility": "^3.0", diff --git a/src/Api/Controller/DeleteFaviconController.php b/src/Api/Controller/DeleteFaviconController.php index 0ce92feac..172ac3906 100644 --- a/src/Api/Controller/DeleteFaviconController.php +++ b/src/Api/Controller/DeleteFaviconController.php @@ -9,12 +9,10 @@ namespace Flarum\Api\Controller; -use Flarum\Foundation\Application; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\AssertPermissionTrait; use Laminas\Diactoros\Response\EmptyResponse; -use League\Flysystem\Adapter\Local; -use League\Flysystem\Filesystem; +use League\Flysystem\FilesystemInterface; use Psr\Http\Message\ServerRequestInterface; class DeleteFaviconController extends AbstractDeleteController @@ -27,17 +25,18 @@ class DeleteFaviconController extends AbstractDeleteController protected $settings; /** - * @var Application + * @var FilesystemInterface */ - protected $app; + protected $uploadDir; /** * @param SettingsRepositoryInterface $settings + * @param FilesystemInterface $uploadDir */ - public function __construct(SettingsRepositoryInterface $settings, Application $app) + public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir) { $this->settings = $settings; - $this->app = $app; + $this->uploadDir = $uploadDir; } /** @@ -51,10 +50,8 @@ class DeleteFaviconController extends AbstractDeleteController $this->settings->set('favicon_path', null); - $uploadDir = new Filesystem(new Local($this->app->publicPath().'/assets')); - - if ($uploadDir->has($path)) { - $uploadDir->delete($path); + if ($this->uploadDir->has($path)) { + $this->uploadDir->delete($path); } return new EmptyResponse(204); diff --git a/src/Api/Controller/DeleteLogoController.php b/src/Api/Controller/DeleteLogoController.php index fb097ff48..34e630445 100644 --- a/src/Api/Controller/DeleteLogoController.php +++ b/src/Api/Controller/DeleteLogoController.php @@ -9,12 +9,10 @@ namespace Flarum\Api\Controller; -use Flarum\Foundation\Application; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\AssertPermissionTrait; use Laminas\Diactoros\Response\EmptyResponse; -use League\Flysystem\Adapter\Local; -use League\Flysystem\Filesystem; +use League\Flysystem\FilesystemInterface; use Psr\Http\Message\ServerRequestInterface; class DeleteLogoController extends AbstractDeleteController @@ -27,17 +25,18 @@ class DeleteLogoController extends AbstractDeleteController protected $settings; /** - * @var Application + * @var FilesystemInterface */ - protected $app; + protected $uploadDir; /** * @param SettingsRepositoryInterface $settings + * @param FilesystemInterface $uploadDir */ - public function __construct(SettingsRepositoryInterface $settings, Application $app) + public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir) { $this->settings = $settings; - $this->app = $app; + $this->uploadDir = $uploadDir; } /** @@ -51,10 +50,8 @@ class DeleteLogoController extends AbstractDeleteController $this->settings->set('logo_path', null); - $uploadDir = new Filesystem(new Local($this->app->publicPath().'/assets')); - - if ($uploadDir->has($path)) { - $uploadDir->delete($path); + if ($this->uploadDir->has($path)) { + $this->uploadDir->delete($path); } return new EmptyResponse(204); diff --git a/src/Api/Controller/UploadFaviconController.php b/src/Api/Controller/UploadFaviconController.php index 40e2ac056..f8d789f50 100644 --- a/src/Api/Controller/UploadFaviconController.php +++ b/src/Api/Controller/UploadFaviconController.php @@ -9,15 +9,12 @@ namespace Flarum\Api\Controller; -use Flarum\Foundation\Application; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\AssertPermissionTrait; use Illuminate\Support\Arr; use Illuminate\Support\Str; use Intervention\Image\ImageManager; -use League\Flysystem\Adapter\Local; -use League\Flysystem\Filesystem; -use League\Flysystem\MountManager; +use League\Flysystem\FilesystemInterface; use Psr\Http\Message\ServerRequestInterface; use Tobscure\JsonApi\Document; @@ -31,17 +28,18 @@ class UploadFaviconController extends ShowForumController protected $settings; /** - * @var Application + * @var FilesystemInterface */ - protected $app; + protected $uploadDir; /** * @param SettingsRepositoryInterface $settings + * @param FilesystemInterface $uploadDir */ - public function __construct(SettingsRepositoryInterface $settings, Application $app) + public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir) { $this->settings = $settings; - $this->app = $app; + $this->uploadDir = $uploadDir; } /** @@ -52,36 +50,28 @@ class UploadFaviconController extends ShowForumController $this->assertAdmin($request->getAttribute('actor')); $file = Arr::get($request->getUploadedFiles(), 'favicon'); - - $tmpFile = tempnam($this->app->storagePath().'/tmp', 'favicon'); - $file->moveTo($tmpFile); - $extension = pathinfo($file->getClientFilename(), PATHINFO_EXTENSION); - if ($extension !== 'ico') { + if ($extension === 'ico') { + $image = $file->getStream(); + } else { $manager = new ImageManager; - $encodedImage = $manager->make($tmpFile)->resize(64, 64, function ($constraint) { + $image = $manager->make($file->getStream())->resize(64, 64, function ($constraint) { $constraint->aspectRatio(); $constraint->upsize(); })->encode('png'); - file_put_contents($tmpFile, $encodedImage); $extension = 'png'; } - $mount = new MountManager([ - 'source' => new Filesystem(new Local(pathinfo($tmpFile, PATHINFO_DIRNAME))), - 'target' => new Filesystem(new Local($this->app->publicPath().'/assets')), - ]); - - if (($path = $this->settings->get('favicon_path')) && $mount->has($file = "target://$path")) { - $mount->delete($file); + if (($path = $this->settings->get('favicon_path')) && $this->uploadDir->has($path)) { + $this->uploadDir->delete($path); } $uploadName = 'favicon-'.Str::lower(Str::random(8)).'.'.$extension; - $mount->move('source://'.pathinfo($tmpFile, PATHINFO_BASENAME), "target://$uploadName"); + $this->uploadDir->write($uploadName, $image); $this->settings->set('favicon_path', $uploadName); diff --git a/src/Api/Controller/UploadLogoController.php b/src/Api/Controller/UploadLogoController.php index 554e50734..ce117b496 100644 --- a/src/Api/Controller/UploadLogoController.php +++ b/src/Api/Controller/UploadLogoController.php @@ -9,15 +9,12 @@ namespace Flarum\Api\Controller; -use Flarum\Foundation\Application; use Flarum\Settings\SettingsRepositoryInterface; use Flarum\User\AssertPermissionTrait; use Illuminate\Support\Arr; use Illuminate\Support\Str; use Intervention\Image\ImageManager; -use League\Flysystem\Adapter\Local; -use League\Flysystem\Filesystem; -use League\Flysystem\MountManager; +use League\Flysystem\FilesystemInterface; use Psr\Http\Message\ServerRequestInterface; use Tobscure\JsonApi\Document; @@ -31,17 +28,18 @@ class UploadLogoController extends ShowForumController protected $settings; /** - * @var Application + * @var FilesystemInterface */ - protected $app; + protected $uploadDir; /** * @param SettingsRepositoryInterface $settings + * @param FilesystemInterface $uploadDir */ - public function __construct(SettingsRepositoryInterface $settings, Application $app) + public function __construct(SettingsRepositoryInterface $settings, FilesystemInterface $uploadDir) { $this->settings = $settings; - $this->app = $app; + $this->uploadDir = $uploadDir; } /** @@ -53,28 +51,19 @@ class UploadLogoController extends ShowForumController $file = Arr::get($request->getUploadedFiles(), 'logo'); - $tmpFile = tempnam($this->app->storagePath().'/tmp', 'logo'); - $file->moveTo($tmpFile); - $manager = new ImageManager; - $encodedImage = $manager->make($tmpFile)->heighten(60, function ($constraint) { + $encodedImage = $manager->make($file->getStream())->heighten(60, function ($constraint) { $constraint->upsize(); })->encode('png'); - file_put_contents($tmpFile, $encodedImage); - $mount = new MountManager([ - 'source' => new Filesystem(new Local(pathinfo($tmpFile, PATHINFO_DIRNAME))), - 'target' => new Filesystem(new Local($this->app->publicPath().'/assets')), - ]); - - if (($path = $this->settings->get('logo_path')) && $mount->has($file = "target://$path")) { - $mount->delete($file); + if (($path = $this->settings->get('logo_path')) && $this->uploadDir->has($path)) { + $this->uploadDir->delete($path); } $uploadName = 'logo-'.Str::lower(Str::random(8)).'.png'; - $mount->move('source://'.pathinfo($tmpFile, PATHINFO_BASENAME), "target://$uploadName"); + $this->uploadDir->write($uploadName, $encodedImage); $this->settings->set('logo_path', $uploadName); diff --git a/src/Settings/SettingsServiceProvider.php b/src/Settings/SettingsServiceProvider.php index 852d23e59..2aa7ade40 100644 --- a/src/Settings/SettingsServiceProvider.php +++ b/src/Settings/SettingsServiceProvider.php @@ -9,8 +9,15 @@ namespace Flarum\Settings; +use Flarum\Api\Controller\DeleteFaviconController; +use Flarum\Api\Controller\DeleteLogoController; +use Flarum\Api\Controller\UploadFaviconController; +use Flarum\Api\Controller\UploadLogoController; use Flarum\Foundation\AbstractServiceProvider; +use Illuminate\Contracts\Container\Container; +use Illuminate\Contracts\Filesystem\Factory; use Illuminate\Database\ConnectionInterface; +use League\Flysystem\FilesystemInterface; class SettingsServiceProvider extends AbstractServiceProvider { @@ -28,5 +35,18 @@ class SettingsServiceProvider extends AbstractServiceProvider }); $this->app->alias(SettingsRepositoryInterface::class, 'flarum.settings'); + + $assets = function (Container $app) { + return $app->make(Factory::class)->disk('flarum-assets')->getDriver(); + }; + + $this->app->when([ + DeleteFaviconController::class, + DeleteLogoController::class, + UploadFaviconController::class, + UploadLogoController::class, + ]) + ->needs(FilesystemInterface::class) + ->give($assets); } } diff --git a/src/User/Command/UploadAvatarHandler.php b/src/User/Command/UploadAvatarHandler.php index 1803c4cc5..7700becf1 100644 --- a/src/User/Command/UploadAvatarHandler.php +++ b/src/User/Command/UploadAvatarHandler.php @@ -9,7 +9,6 @@ namespace Flarum\User\Command; -use Flarum\Foundation\Application; use Flarum\Foundation\DispatchEventsTrait; use Flarum\User\AssertPermissionTrait; use Flarum\User\AvatarUploader; @@ -29,11 +28,6 @@ class UploadAvatarHandler */ protected $users; - /** - * @var Application - */ - protected $app; - /** * @var AvatarUploader */ @@ -47,15 +41,13 @@ class UploadAvatarHandler /** * @param Dispatcher $events * @param UserRepository $users - * @param Application $app * @param AvatarUploader $uploader * @param AvatarValidator $validator */ - public function __construct(Dispatcher $events, UserRepository $users, Application $app, AvatarUploader $uploader, AvatarValidator $validator) + public function __construct(Dispatcher $events, UserRepository $users, AvatarUploader $uploader, AvatarValidator $validator) { $this->events = $events; $this->users = $users; - $this->app = $app; $this->uploader = $uploader; $this->validator = $validator; } @@ -76,26 +68,17 @@ class UploadAvatarHandler $this->assertCan($actor, 'edit', $user); } - $file = $command->file; + $this->validator->assertValid(['avatar' => $command->file]); - $tmpFile = tempnam($this->app->storagePath().'/tmp', 'avatar'); - $file->moveTo($tmpFile); + $image = (new ImageManager)->make($command->file->getStream()); - try { - $this->validator->assertValid(['avatar' => $file]); + $this->events->dispatch( + new AvatarSaving($user, $actor, $image) + ); - $image = (new ImageManager)->make($tmpFile); + $this->uploader->upload($user, $image); - $this->events->dispatch( - new AvatarSaving($user, $actor, $image) - ); - - $this->uploader->upload($user, $image); - - $user->save(); - } finally { - @unlink($tmpFile); - } + $user->save(); return $user; }