From 79d006640be9a404a620c9414cbe4e7b38cb26a5 Mon Sep 17 00:00:00 2001
From: Lee Rowlands <lee.rowlands@previousnext.com.au>
Date: Thu, 11 Apr 2024 15:42:35 +1000
Subject: [PATCH] Issue #3437623 by alexpott, kim.pepper, sakthi_dev:
\Drupal\file\Upload\FileUploadHandler::handleFileUpload() should alway check
that the uploaded file is valid
---
core/modules/file/file.module | 9 --
core/modules/file/file.services.yml | 6 +-
.../file/src/Upload/FileUploadHandler.php | 97 +++++++++----------
.../file/src/Upload/FormUploadedFile.php | 29 +++---
.../file/src/Upload/UploadedFileInterface.php | 16 +++
.../src/Validation/UploadedFileValidator.php | 56 -----------
.../UploadedFileValidatorInterface.php | 29 ------
.../src/Kernel/FileUploadHandlerTest.php | 5 +-
...> UploadedFileConstraintValidatorTest.php} | 38 ++++----
9 files changed, 94 insertions(+), 191 deletions(-)
delete mode 100644 core/modules/file/src/Validation/UploadedFileValidator.php
delete mode 100644 core/modules/file/src/Validation/UploadedFileValidatorInterface.php
rename core/modules/file/tests/src/Kernel/Validation/{UploadedFileValidatorTest.php => UploadedFileConstraintValidatorTest.php} (78%)
diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index 555b406e2441..bc52dbe021a0 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -653,18 +653,9 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
/** @var \Drupal\Core\Render\RendererInterface $renderer */
$renderer = \Drupal::service('renderer');
$files = [];
- /** @var \Drupal\file\Validation\UploadedFileValidatorInterface $uploaded_file_validator */
- $uploaded_file_validator = \Drupal::service('file.uploaded_file_validator');
/** @var \Symfony\Component\HttpFoundation\File\UploadedFile $uploaded_file */
foreach ($uploaded_files as $i => $uploaded_file) {
try {
- $violations = $uploaded_file_validator->validate($uploaded_file);
- if (count($violations) > 0) {
- // We only get one violation for uploaded files.
- \Drupal::messenger()->addError($violations->get(0)->getMessage());
- $files[$i] = FALSE;
- continue;
- }
$form_uploaded_file = new FormUploadedFile($uploaded_file);
$result = $file_upload_handler->handleFileUpload($form_uploaded_file, $validators, $destination, $fileExists, FALSE);
if ($result->hasViolations()) {
diff --git a/core/modules/file/file.services.yml b/core/modules/file/file.services.yml
index 6ac6765e07e3..a8acef6e27e1 100644
--- a/core/modules/file/file.services.yml
+++ b/core/modules/file/file.services.yml
@@ -11,7 +11,7 @@ services:
- { name: backend_overridable }
file.upload_handler:
class: Drupal\file\Upload\FileUploadHandler
- arguments: ['@file_system', '@entity_type.manager', '@stream_wrapper_manager', '@event_dispatcher', '@file.mime_type.guesser', '@current_user', '@request_stack', '@file.repository', '@file.validator', '@lock']
+ arguments: ['@file_system', '@entity_type.manager', '@stream_wrapper_manager', '@event_dispatcher', '@file.mime_type.guesser', '@current_user', '@request_stack', '@file.repository', '@file.validator', '@lock', '@validation.basic_recursive_validator_factory']
Drupal\file\Upload\FileUploadHandler: '@file.upload_handler'
file.repository:
class: Drupal\file\FileRepository
@@ -28,10 +28,6 @@ services:
class: Drupal\file\Validation\FileValidator
arguments: ['@file.recursive_validator', '@validation.constraint', '@event_dispatcher', '@module_handler']
Drupal\file\Validation\FileValidatorInterface: '@file.validator'
- file.uploaded_file_validator:
- class: Drupal\file\Validation\UploadedFileValidator
- arguments: ['@validation.basic_recursive_validator_factory']
- Drupal\file\Validation\UploadedFileValidatorInterface: '@file.uploaded_file_validator'
file.input_stream_file_writer:
class: Drupal\file\Upload\InputStreamFileWriter
arguments: ['@file_system']
diff --git a/core/modules/file/src/Upload/FileUploadHandler.php b/core/modules/file/src/Upload/FileUploadHandler.php
index 716d1869beb1..2512e7ceb147 100644
--- a/core/modules/file/src/Upload/FileUploadHandler.php
+++ b/core/modules/file/src/Upload/FileUploadHandler.php
@@ -14,6 +14,7 @@
use Drupal\Core\Lock\LockBackendInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface;
+use Drupal\Core\Validation\BasicRecursiveValidatorFactory;
use Drupal\file\Entity\File;
use Drupal\file\FileInterface;
use Drupal\file\FileRepositoryInterface;
@@ -103,30 +104,6 @@ class FileUploadHandler {
*/
protected FileValidatorInterface $fileValidator;
- /**
- * Constructs a FileUploadHandler object.
- *
- * @param \Drupal\Core\File\FileSystemInterface $fileSystem
- * The file system service.
- * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
- * The entity type manager.
- * @param \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $streamWrapperManager
- * The stream wrapper manager.
- * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $eventDispatcher
- * The event dispatcher.
- * @param \Symfony\Component\Mime\MimeTypeGuesserInterface $mimeTypeGuesser
- * The MIME type guesser.
- * @param \Drupal\Core\Session\AccountInterface $currentUser
- * The current user.
- * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
- * The request stack.
- * @param \Drupal\file\FileRepositoryInterface|null $fileRepository
- * The file repository.
- * @param \Drupal\file\Validation\FileValidatorInterface|null $file_validator
- * The file validator.
- * @param \Drupal\Core\Lock\LockBackendInterface|null $lock
- * The lock.
- */
public function __construct(
FileSystemInterface $fileSystem,
EntityTypeManagerInterface $entityTypeManager,
@@ -138,6 +115,7 @@ public function __construct(
FileRepositoryInterface $fileRepository = NULL,
FileValidatorInterface $file_validator = NULL,
protected ?LockBackendInterface $lock = NULL,
+ protected ?BasicRecursiveValidatorFactory $validatorFactory = NULL,
) {
$this->fileSystem = $fileSystem;
$this->entityTypeManager = $entityTypeManager;
@@ -160,6 +138,10 @@ public function __construct(
@trigger_error('Calling ' . __METHOD__ . '() without the $lock argument is deprecated in drupal:10.3.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/3389017', E_USER_DEPRECATED);
$this->lock = \Drupal::service('lock');
}
+ if (!$validatorFactory) {
+ @trigger_error('Calling ' . __METHOD__ . '() without the $validatorFactory argument is deprecated in drupal:10.3.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/3375456', E_USER_DEPRECATED);
+ $this->validatorFactory = \Drupal::service('validation.basic_recursive_validator_factory');
+ }
}
/**
@@ -197,45 +179,56 @@ public function handleFileUpload(UploadedFileInterface $uploadedFile, array $val
// @phpstan-ignore-next-line
$fileExists = FileExists::fromLegacyInt($fileExists, __METHOD__);
}
- $originalName = $uploadedFile->getClientOriginalName();
- // @phpstan-ignore-next-line
- if ($throw && !$uploadedFile->isValid()) {
+ $result = new FileUploadResult();
+
+ if ($throw) {
@trigger_error('Calling ' . __METHOD__ . '() with the $throw argument as TRUE is deprecated in drupal:10.3.0 and will be removed in drupal:11.0.0. Use \Drupal\file\Upload\FileUploadResult::getViolations() instead. See https://www.drupal.org/node/3375456', E_USER_DEPRECATED);
// @phpstan-ignore-next-line
- switch ($uploadedFile->getError()) {
- case \UPLOAD_ERR_INI_SIZE:
- // @phpstan-ignore-next-line
- throw new IniSizeFileException($uploadedFile->getErrorMessage());
+ if (!$uploadedFile->isValid()) {
+ // @phpstan-ignore-next-line
+ switch ($uploadedFile->getError()) {
+ case \UPLOAD_ERR_INI_SIZE:
+ // @phpstan-ignore-next-line
+ throw new IniSizeFileException($uploadedFile->getErrorMessage());
- case \UPLOAD_ERR_FORM_SIZE:
- // @phpstan-ignore-next-line
- throw new FormSizeFileException($uploadedFile->getErrorMessage());
+ case \UPLOAD_ERR_FORM_SIZE:
+ // @phpstan-ignore-next-line
+ throw new FormSizeFileException($uploadedFile->getErrorMessage());
- case \UPLOAD_ERR_PARTIAL:
- // @phpstan-ignore-next-line
- throw new PartialFileException($uploadedFile->getErrorMessage());
+ case \UPLOAD_ERR_PARTIAL:
+ // @phpstan-ignore-next-line
+ throw new PartialFileException($uploadedFile->getErrorMessage());
- case \UPLOAD_ERR_NO_FILE:
- // @phpstan-ignore-next-line
- throw new NoFileException($uploadedFile->getErrorMessage());
+ case \UPLOAD_ERR_NO_FILE:
+ // @phpstan-ignore-next-line
+ throw new NoFileException($uploadedFile->getErrorMessage());
- case \UPLOAD_ERR_CANT_WRITE:
- // @phpstan-ignore-next-line
- throw new CannotWriteFileException($uploadedFile->getErrorMessage());
+ case \UPLOAD_ERR_CANT_WRITE:
+ // @phpstan-ignore-next-line
+ throw new CannotWriteFileException($uploadedFile->getErrorMessage());
- case \UPLOAD_ERR_NO_TMP_DIR:
- // @phpstan-ignore-next-line
- throw new NoTmpDirFileException($uploadedFile->getErrorMessage());
+ case \UPLOAD_ERR_NO_TMP_DIR:
+ // @phpstan-ignore-next-line
+ throw new NoTmpDirFileException($uploadedFile->getErrorMessage());
- case \UPLOAD_ERR_EXTENSION:
- // @phpstan-ignore-next-line
- throw new ExtensionFileException($uploadedFile->getErrorMessage());
+ case \UPLOAD_ERR_EXTENSION:
+ // @phpstan-ignore-next-line
+ throw new ExtensionFileException($uploadedFile->getErrorMessage());
+ }
+ // @phpstan-ignore-next-line
+ throw new FileException($uploadedFile->getErrorMessage());
+ }
+ }
+ else {
+ $violations = $uploadedFile->validate($this->validatorFactory->createValidator());
+ if (count($violations) > 0) {
+ $result->addViolations($violations);
+ return $result;
}
- // @phpstan-ignore-next-line
- throw new FileException($uploadedFile->getErrorMessage());
}
+ $originalName = $uploadedFile->getClientOriginalName();
$extensions = $this->handleExtensionValidation($validators);
// Assert that the destination contains a valid stream.
@@ -288,8 +281,6 @@ public function handleFileUpload(UploadedFileInterface $uploadedFile, array $val
// Add in our check of the file name length.
$validators['FileNameLength'] = [];
- $result = new FileUploadResult();
-
// Call the validation functions specified by this function's caller.
$violations = $this->fileValidator->validate($file, $validators);
if (count($violations) > 0) {
diff --git a/core/modules/file/src/Upload/FormUploadedFile.php b/core/modules/file/src/Upload/FormUploadedFile.php
index 7ae45b7721ee..9e7747dd893f 100644
--- a/core/modules/file/src/Upload/FormUploadedFile.php
+++ b/core/modules/file/src/Upload/FormUploadedFile.php
@@ -2,7 +2,10 @@
namespace Drupal\file\Upload;
+use Drupal\file\Validation\Constraint\UploadedFileConstraint;
use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Symfony\Component\Validator\ConstraintViolationListInterface;
+use Symfony\Component\Validator\Validator\ValidatorInterface;
/**
* Provides a bridge to Symfony UploadedFile.
@@ -35,12 +38,6 @@ public function getClientOriginalName(): string {
/**
* {@inheritdoc}
- *
- * @deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use
- * \Drupal\file\Validation\UploadedFileValidatorInterface::validate()
- * instead.
- *
- * @see https://www.drupal.org/node/3375456
*/
public function isValid(): bool {
@trigger_error(__METHOD__ . '() is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use \Drupal\file\Validation\UploadedFileValidatorInterface::validate() instead. See https://www.drupal.org/node/3375456', E_USER_DEPRECATED);
@@ -49,12 +46,6 @@ public function isValid(): bool {
/**
* {@inheritdoc}
- *
- * @deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use
- * \Drupal\file\Validation\UploadedFileValidatorInterface::validate()
- * instead.
- *
- * @see https://www.drupal.org/node/3375456
*/
public function getErrorMessage(): string {
@trigger_error(__METHOD__ . '() is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use \Drupal\file\Validation\UploadedFileValidatorInterface::validate() instead. See https://www.drupal.org/node/3375456', E_USER_DEPRECATED);
@@ -63,12 +54,6 @@ public function getErrorMessage(): string {
/**
* {@inheritdoc}
- *
- * @deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use
- * \Drupal\file\Validation\UploadedFileValidatorInterface::validate()
- * instead.
- *
- * @see https://www.drupal.org/node/3375456
*/
public function getError(): int {
@trigger_error(__METHOD__ . '() is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use \Drupal\file\Validation\UploadedFileValidatorInterface::validate() instead. See https://www.drupal.org/node/3375456', E_USER_DEPRECATED);
@@ -103,4 +88,12 @@ public function getFilename(): string {
return $this->uploadedFile->getFilename();
}
+ /**
+ * {@inheritdoc}
+ */
+ public function validate(ValidatorInterface $validator, array $options = []): ConstraintViolationListInterface {
+ $constraint = new UploadedFileConstraint($options);
+ return $validator->validate($this->uploadedFile, $constraint);
+ }
+
}
diff --git a/core/modules/file/src/Upload/UploadedFileInterface.php b/core/modules/file/src/Upload/UploadedFileInterface.php
index c601e0e9a604..8a25ce70aec0 100644
--- a/core/modules/file/src/Upload/UploadedFileInterface.php
+++ b/core/modules/file/src/Upload/UploadedFileInterface.php
@@ -2,6 +2,9 @@
namespace Drupal\file\Upload;
+use Symfony\Component\Validator\ConstraintViolationListInterface;
+use Symfony\Component\Validator\Validator\ValidatorInterface;
+
/**
* Provides an interface for uploaded files.
*/
@@ -102,4 +105,17 @@ public function getPathname(): string;
*/
public function getFilename(): string;
+ /**
+ * Validates the uploaded file information.
+ *
+ * @param \Symfony\Component\Validator\Validator\ValidatorInterface $validator
+ * A validator object.
+ * @param array $options
+ * Options to pass to a constraint.
+ *
+ * @return \Symfony\Component\Validator\ConstraintViolationListInterface
+ * The list of violations.
+ */
+ public function validate(ValidatorInterface $validator, array $options = []): ConstraintViolationListInterface;
+
}
diff --git a/core/modules/file/src/Validation/UploadedFileValidator.php b/core/modules/file/src/Validation/UploadedFileValidator.php
deleted file mode 100644
index 984b06f9878f..000000000000
--- a/core/modules/file/src/Validation/UploadedFileValidator.php
+++ /dev/null
@@ -1,56 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Drupal\file\Validation;
-
-use Drupal\Core\Validation\BasicRecursiveValidatorFactory;
-use Drupal\file\Validation\Constraint\UploadedFileConstraint;
-use Symfony\Component\HttpFoundation\File\UploadedFile;
-use Symfony\Component\Validator\ConstraintViolationListInterface;
-use Symfony\Component\Validator\Validator\ValidatorInterface;
-
-/**
- * Validator for uploaded files.
- */
-class UploadedFileValidator implements UploadedFileValidatorInterface {
-
- /**
- * The symfony validator.
- *
- * @var \Symfony\Component\Validator\Validator\ValidatorInterface
- */
- protected ValidatorInterface $validator;
-
- /**
- * Creates a new UploadedFileValidator.
- *
- * @param \Drupal\Core\Validation\BasicRecursiveValidatorFactory $validatorFactory
- * The validator factory.
- */
- public function __construct(
- protected readonly BasicRecursiveValidatorFactory $validatorFactory,
- ) {}
-
- /**
- * {@inheritdoc}
- */
- public function validate(UploadedFile $uploadedFile, array $options = []): ConstraintViolationListInterface {
- $constraint = new UploadedFileConstraint($options);
- return $this->getValidator()->validate($uploadedFile, $constraint);
- }
-
- /**
- * Get the Symfony validator instance.
- *
- * @return \Symfony\Component\Validator\Validator\ValidatorInterface
- * The Symfony validator.
- */
- protected function getValidator(): ValidatorInterface {
- if (!isset($this->validator)) {
- $this->validator = $this->validatorFactory->createValidator();
- }
- return $this->validator;
- }
-
-}
diff --git a/core/modules/file/src/Validation/UploadedFileValidatorInterface.php b/core/modules/file/src/Validation/UploadedFileValidatorInterface.php
deleted file mode 100644
index 1e745cf68089..000000000000
--- a/core/modules/file/src/Validation/UploadedFileValidatorInterface.php
+++ /dev/null
@@ -1,29 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Drupal\file\Validation;
-
-use Symfony\Component\HttpFoundation\File\UploadedFile;
-use Symfony\Component\Validator\ConstraintViolationListInterface;
-
-/**
- * Validator for uploaded files.
- */
-interface UploadedFileValidatorInterface {
-
- /**
- * Validates an uploaded file.
- *
- * @param \Symfony\Component\HttpFoundation\File\UploadedFile $uploadedFile
- * The uploaded file.
- * @param array $options
- * An array of options accepted by
- * \Drupal\file\Validation\Constraint\UploadedFileConstraint.
- *
- * @return \Symfony\Component\Validator\ConstraintViolationListInterface
- * The constraint violation list.
- */
- public function validate(UploadedFile $uploadedFile, array $options = []): ConstraintViolationListInterface;
-
-}
diff --git a/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php b/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php
index 81bec1bb3c67..656fdb6d005b 100644
--- a/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php
+++ b/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php
@@ -10,6 +10,7 @@
use Drupal\file\Upload\UploadedFileInterface;
use Drupal\KernelTests\KernelTestBase;
use Symfony\Component\Mime\MimeTypeGuesserInterface;
+use Symfony\Component\Validator\ConstraintViolationList;
/**
* Tests the file upload handler.
@@ -92,12 +93,14 @@ public function testLockAcquireException(): void {
$this->container->get('request_stack'),
$this->container->get('file.repository'),
$this->container->get('file.validator'),
- $lock
+ $lock,
+ $this->container->get('validation.basic_recursive_validator_factory'),
);
$file_name = $this->randomMachineName();
$file_info = $this->createMock(UploadedFileInterface::class);
$file_info->expects($this->once())->method('getClientOriginalName')->willReturn($file_name);
+ $file_info->expects($this->once())->method('validate')->willReturn(new ConstraintViolationList());
$this->expectException(LockAcquiringException::class);
$this->expectExceptionMessage(sprintf('File "temporary://%s" is already locked for writing.', $file_name));
diff --git a/core/modules/file/tests/src/Kernel/Validation/UploadedFileValidatorTest.php b/core/modules/file/tests/src/Kernel/Validation/UploadedFileConstraintValidatorTest.php
similarity index 78%
rename from core/modules/file/tests/src/Kernel/Validation/UploadedFileValidatorTest.php
rename to core/modules/file/tests/src/Kernel/Validation/UploadedFileConstraintValidatorTest.php
index 45b1dbb7271a..fbd51dc3b143 100644
--- a/core/modules/file/tests/src/Kernel/Validation/UploadedFileValidatorTest.php
+++ b/core/modules/file/tests/src/Kernel/Validation/UploadedFileConstraintValidatorTest.php
@@ -5,30 +5,24 @@
namespace Drupal\Tests\file\Kernel\Validation;
use Drupal\Core\StringTranslation\TranslatableMarkup;
-use Drupal\file\Validation\UploadedFileValidator;
+use Drupal\file\Upload\FormUploadedFile;
use Drupal\KernelTests\KernelTestBase;
use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Symfony\Component\Validator\Validator\ValidatorInterface;
/**
* Tests the uploaded file validator.
*
- * @coversDefaultClass \Drupal\file\Validation\UploadedFileValidator
+ * @coversDefaultClass \Drupal\file\Validation\Constraint\UploadedFileConstraintValidator
* @group file
*/
-class UploadedFileValidatorTest extends KernelTestBase {
+class UploadedFileConstraintValidatorTest extends KernelTestBase {
/**
* {@inheritdoc}
*/
protected static $modules = ['file'];
- /**
- * The validator under test.
- *
- * @var \Drupal\file\Validation\UploadedFileValidator
- */
- protected UploadedFileValidator $validator;
-
/**
* The file name.
*
@@ -50,16 +44,20 @@ class UploadedFileValidatorTest extends KernelTestBase {
*/
protected int $maxSize = 4194304;
+ /**
+ * A validator.
+ *
+ * @var \Symfony\Component\Validator\Validator\ValidatorInterface
+ */
+ private ValidatorInterface $validator;
+
/**
* {@inheritdoc}
*/
protected function setUp(): void {
parent::setUp();
$fileSystem = $this->container->get('file_system');
- /** @var \Drupal\file\Validation\UploadedFileValidator $validator */
- $this->validator = new UploadedFileValidator(
- $this->container->get('validation.basic_recursive_validator_factory'),
- );
+ $this->validator = $this->container->get('validation.basic_recursive_validator_factory')->createValidator();
$this->filename = $this->randomMachineName() . '.txt';
$this->path = 'temporary://' . $this->filename;
@@ -70,12 +68,12 @@ protected function setUp(): void {
* @covers ::validate
*/
public function testValidateSuccess(): void {
- $uploadedFile = new UploadedFile(
+ $uploadedFile = new FormUploadedFile(new UploadedFile(
path: $this->path,
originalName: $this->filename,
test: TRUE,
- );
- $violations = $this->validator->validate($uploadedFile);
+ ));
+ $violations = $uploadedFile->validate($this->validator);
$this->assertCount(0, $violations);
}
@@ -84,13 +82,13 @@ public function testValidateSuccess(): void {
* @dataProvider validateProvider
*/
public function testValidateFail(int $errorCode, string $message): void {
- $uploadedFile = new UploadedFile(
+ $uploadedFile = new FormUploadedFile(new UploadedFile(
path: $this->path,
originalName: $this->filename,
error: $errorCode,
test: TRUE,
- );
- $violations = $this->validator->validate($uploadedFile, [
+ ));
+ $violations = $uploadedFile->validate($this->validator, [
'maxSize' => $this->maxSize,
]);
$this->assertCount(1, $violations);
--
GitLab