From 6236d36180cf7ae446b6d4847861998a649bf830 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Sat, 2 Mar 2024 11:58:23 +0000 Subject: [PATCH] Issue #3389016 by kim.pepper, xjm, larowlan: Add file upload lock handling to FileUploadHandler --- .../Core/Lock/LockAcquiringException.php | 11 + core/modules/file/file.module | 5 + core/modules/file/file.services.yml | 2 +- .../file/src/Upload/FileUploadHandler.php | 229 +++++++++++------- .../src/Kernel/FileUploadHandlerTest.php | 33 +++ 5 files changed, 193 insertions(+), 87 deletions(-) create mode 100644 core/lib/Drupal/Core/Lock/LockAcquiringException.php diff --git a/core/lib/Drupal/Core/Lock/LockAcquiringException.php b/core/lib/Drupal/Core/Lock/LockAcquiringException.php new file mode 100644 index 00000000000..91e7129d493 --- /dev/null +++ b/core/lib/Drupal/Core/Lock/LockAcquiringException.php @@ -0,0 +1,11 @@ +addError(t('The file %filename could not be uploaded because the name is invalid.', ['%filename' => $uploaded_file->getClientOriginalName()])); $files[$i] = FALSE; } + catch (LockAcquiringException $e) { + \Drupal::messenger()->addError(t('File already locked for writing.')); + $files[$i] = FALSE; + } } // Add files to the cache. diff --git a/core/modules/file/file.services.yml b/core/modules/file/file.services.yml index 9b56858a1a0..6ac6765e07e 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'] + arguments: ['@file_system', '@entity_type.manager', '@stream_wrapper_manager', '@event_dispatcher', '@file.mime_type.guesser', '@current_user', '@request_stack', '@file.repository', '@file.validator', '@lock'] Drupal\file\Upload\FileUploadHandler: '@file.upload_handler' file.repository: class: Drupal\file\FileRepository diff --git a/core/modules/file/src/Upload/FileUploadHandler.php b/core/modules/file/src/Upload/FileUploadHandler.php index 0f8d52ff37e..1e4b832b6eb 100644 --- a/core/modules/file/src/Upload/FileUploadHandler.php +++ b/core/modules/file/src/Upload/FileUploadHandler.php @@ -2,12 +2,15 @@ namespace Drupal\file\Upload; +use Drupal\Component\Utility\Crypt; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\File\Event\FileUploadSanitizeNameEvent; use Drupal\Core\File\Exception\FileExistsException; use Drupal\Core\File\Exception\FileWriteException; use Drupal\Core\File\Exception\InvalidStreamWrapperException; use Drupal\Core\File\FileSystemInterface; +use Drupal\Core\Lock\LockAcquiringException; +use Drupal\Core\Lock\LockBackendInterface; use Drupal\Core\Session\AccountInterface; use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface; use Drupal\file\Entity\File; @@ -120,8 +123,21 @@ class FileUploadHandler { * 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, StreamWrapperManagerInterface $streamWrapperManager, EventDispatcherInterface $eventDispatcher, MimeTypeGuesserInterface $mimeTypeGuesser, AccountInterface $currentUser, RequestStack $requestStack, FileRepositoryInterface $fileRepository = NULL, FileValidatorInterface $file_validator = NULL) { + public function __construct( + FileSystemInterface $fileSystem, + EntityTypeManagerInterface $entityTypeManager, + StreamWrapperManagerInterface $streamWrapperManager, + EventDispatcherInterface $eventDispatcher, + MimeTypeGuesserInterface $mimeTypeGuesser, + AccountInterface $currentUser, + RequestStack $requestStack, + FileRepositoryInterface $fileRepository = NULL, + FileValidatorInterface $file_validator = NULL, + protected ?LockBackendInterface $lock = NULL, + ) { $this->fileSystem = $fileSystem; $this->entityTypeManager = $entityTypeManager; $this->streamWrapperManager = $streamWrapperManager; @@ -139,6 +155,10 @@ class FileUploadHandler { $file_validator = \Drupal::service('file.validator'); } $this->fileValidator = $file_validator; + if (!$this->lock) { + @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'); + } } /** @@ -170,6 +190,8 @@ class FileUploadHandler { * Thrown when a file system error occurs and $throws is TRUE. * @throws \Drupal\file\Upload\FileValidationException * Thrown when file validation fails and $throws is TRUE. + * @throws \Drupal\Core\Lock\LockAcquiringException + * Thrown when a lock cannot be acquired. */ public function handleFileUpload(UploadedFileInterface $uploadedFile, array $validators = [], string $destination = 'temporary://', int $replace = FileSystemInterface::EXISTS_REPLACE, bool $throw = TRUE): FileUploadResult { $originalName = $uploadedFile->getClientOriginalName(); @@ -236,96 +258,124 @@ class FileUploadHandler { throw new FileExistsException(sprintf('Destination file "%s" exists', $destinationFilename)); } - $file = File::create([ - 'uid' => $this->currentUser->id(), - 'status' => 0, - 'uri' => $uploadedFile->getRealPath(), - ]); + // Lock based on the prepared file URI. + $lock_id = $this->generateLockId($destinationFilename); - // This will be replaced later with a filename based on the destination. - $file->setFilename($filename); - $file->setMimeType($mimeType); - $file->setSize($uploadedFile->getSize()); - - // 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) { - $result->addViolations($violations); - return $result; - } - - if ($throw) { - $errors = []; - foreach ($violations as $violation) { - $errors[] = $violation->getMessage(); + try { + if (!$this->lock->acquire($lock_id)) { + throw new LockAcquiringException( + sprintf( + 'File "%s" is already locked for writing.', + $destinationFilename + ) + ); } - if (!empty($errors)) { - throw new FileValidationException('File validation failed', $filename, $errors); + + $file = File::create([ + 'uid' => $this->currentUser->id(), + 'status' => 0, + 'uri' => $uploadedFile->getRealPath(), + ]); + + // This will be replaced later with a filename based on the destination. + $file->setFilename($filename); + $file->setMimeType($mimeType); + $file->setSize($uploadedFile->getSize()); + + // 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) { + $result->addViolations($violations); + + return $result; + } + + if ($throw) { + $errors = []; + foreach ($violations as $violation) { + $errors[] = $violation->getMessage(); + } + if (!empty($errors)) { + throw new FileValidationException( + 'File validation failed', + $filename, + $errors + ); + } + } + + $file->setFileUri($destinationFilename); + + if (!$this->moveUploadedFile($uploadedFile, $file->getFileUri())) { + throw new FileWriteException( + 'File upload error. Could not move uploaded file.' + ); + } + + // Update the filename with any changes as a result of security or + // renaming due to an existing file. + $file->setFilename($this->fileSystem->basename($file->getFileUri())); + + if ($replace === FileSystemInterface::EXISTS_REPLACE) { + $existingFile = $this->fileRepository->loadByUri($file->getFileUri()); + if ($existingFile) { + $file->fid = $existingFile->id(); + $file->setOriginalId($existingFile->id()); + } + } + + $result->setOriginalFilename($originalName) + ->setSanitizedFilename($filename) + ->setFile($file); + + // If the filename has been modified, let the user know. + if ($event->isSecurityRename()) { + $result->setSecurityRename(); + } + + // Set the permissions on the new file. + $this->fileSystem->chmod($file->getFileUri()); + + // We can now validate the file object itself before it's saved. + $violations = $file->validate(); + if ($throw) { + foreach ($violations as $violation) { + $errors[] = $violation->getMessage(); + } + if (!empty($errors)) { + throw new FileValidationException( + 'File validation failed', + $filename, + $errors + ); + } + } + if (count($violations) > 0) { + $result->addViolations($violations); + + return $result; + } + + // If we made it this far it's safe to record this file in the database. + $file->save(); + + // Allow an anonymous user who creates a non-public file to see it. See + // \Drupal\file\FileAccessControlHandler::checkAccess(). + if ($this->currentUser->isAnonymous() && $destinationScheme !== 'public') { + $session = $this->requestStack->getCurrentRequest()->getSession(); + $allowed_temp_files = $session->get('anonymous_allowed_file_ids', []); + $allowed_temp_files[$file->id()] = $file->id(); + $session->set('anonymous_allowed_file_ids', $allowed_temp_files); } } - - $file->setFileUri($destinationFilename); - - if (!$this->moveUploadedFile($uploadedFile, $file->getFileUri())) { - throw new FileWriteException('File upload error. Could not move uploaded file.'); + finally { + $this->lock->release($lock_id); } - - // Update the filename with any changes as a result of security or renaming - // due to an existing file. - $file->setFilename($this->fileSystem->basename($file->getFileUri())); - - if ($replace === FileSystemInterface::EXISTS_REPLACE) { - $existingFile = $this->fileRepository->loadByUri($file->getFileUri()); - if ($existingFile) { - $file->fid = $existingFile->id(); - $file->setOriginalId($existingFile->id()); - } - } - - $result->setOriginalFilename($originalName) - ->setSanitizedFilename($filename) - ->setFile($file); - - // If the filename has been modified, let the user know. - if ($event->isSecurityRename()) { - $result->setSecurityRename(); - } - - // Set the permissions on the new file. - $this->fileSystem->chmod($file->getFileUri()); - - // We can now validate the file object itself before it's saved. - $violations = $file->validate(); - if ($throw) { - foreach ($violations as $violation) { - $errors[] = $violation->getMessage(); - } - if (!empty($errors)) { - throw new FileValidationException('File validation failed', $filename, $errors); - } - } - if (count($violations) > 0) { - $result->addViolations($violations); - return $result; - } - - // If we made it this far it's safe to record this file in the database. - $file->save(); - - // Allow an anonymous user who creates a non-public file to see it. See - // \Drupal\file\FileAccessControlHandler::checkAccess(). - if ($this->currentUser->isAnonymous() && $destinationScheme !== 'public') { - $session = $this->requestStack->getCurrentRequest()->getSession(); - $allowed_temp_files = $session->get('anonymous_allowed_file_ids', []); - $allowed_temp_files[$file->id()] = $file->id(); - $session->set('anonymous_allowed_file_ids', $allowed_temp_files); - } - return $result; } @@ -412,4 +462,11 @@ class FileUploadHandler { return $this->fileRepository->loadByUri($uri); } + /** + * Generates a lock ID based on the file URI. + */ + protected static function generateLockId(string $fileUri): string { + return 'file:upload:' . Crypt::hashBase64($fileUri); + } + } diff --git a/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php b/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php index bdc5ccbcfdb..5410552c9c2 100644 --- a/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php +++ b/core/modules/file/tests/src/Kernel/FileUploadHandlerTest.php @@ -2,6 +2,8 @@ namespace Drupal\Tests\file\Kernel; +use Drupal\Core\Lock\LockAcquiringException; +use Drupal\Core\Lock\LockBackendInterface; use Drupal\file\Upload\FileUploadHandler; use Drupal\file\Upload\UploadedFileInterface; use Drupal\KernelTests\KernelTestBase; @@ -70,4 +72,35 @@ class FileUploadHandlerTest extends KernelTestBase { $this->assertEquals(['txt'], $subscriber->getAllowedExtensions()); } + /** + * Test the lock acquire exception. + */ + public function testLockAcquireException(): void { + + $lock = $this->createMock(LockBackendInterface::class); + $lock->expects($this->once())->method('acquire')->willReturn(FALSE); + + $fileUploadHandler = new FileUploadHandler( + $this->container->get('file_system'), + $this->container->get('entity_type.manager'), + $this->container->get('stream_wrapper_manager'), + $this->container->get('event_dispatcher'), + $this->container->get('file.mime_type.guesser'), + $this->container->get('current_user'), + $this->container->get('request_stack'), + $this->container->get('file.repository'), + $this->container->get('file.validator'), + $lock + ); + + $file_name = $this->randomMachineName(); + $file_info = $this->createMock(UploadedFileInterface::class); + $file_info->expects($this->once())->method('getClientOriginalName')->willReturn($file_name); + + $this->expectException(LockAcquiringException::class); + $this->expectExceptionMessage(sprintf('File "temporary://%s" is already locked for writing.', $file_name)); + + $fileUploadHandler->handleFileUpload(uploadedFile: $file_info, throw: FALSE); + } + }