diff --git a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php index a7b430afc9..ed18a0a6a1 100644 --- a/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php @@ -4,6 +4,8 @@ use Drupal\Component\Utility\Bytes; use Drupal\Component\Utility\Crypt; +use Drupal\Core\Config\Config; +use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Field\FieldDefinitionInterface; use Drupal\Core\Lock\LockBackendInterface; @@ -35,8 +37,8 @@ * This is implemented as a field-level resource for the following reasons: * - Validation for uploaded files is tied to fields (allowed extensions, max * size, etc..). - * - The actual files do not need to be stored in another temporary local, to - * be later moved when they are referenced from a file field. + * - The actual files do not need to be stored in another temporary location, + * to be later moved when they are referenced from a file field. * - Permission to upload a file can be determined by a users field level * create access to the file field. * @@ -118,6 +120,11 @@ class FileUploadResource extends ResourceBase { */ protected $lock; + /** + * @var \Drupal\Core\Config\ImmutableConfig + */ + protected $systemFileConfig; + /** * Constructs a FileUploadResource instance. * @@ -145,8 +152,10 @@ class FileUploadResource extends ResourceBase { * The token replacement instance. * @param \Drupal\Core\Lock\LockBackendInterface $lock * The lock service. + * @param \Drupal\Core\Config\Config $system_file_config + * The system file configuration. */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystem $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, MimeTypeGuesserInterface $mime_type_guesser, Token $token, LockBackendInterface $lock) { + public function __construct(array $configuration, $plugin_id, $plugin_definition, $serializer_formats, LoggerInterface $logger, FileSystem $file_system, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, AccountInterface $current_user, MimeTypeGuesserInterface $mime_type_guesser, Token $token, LockBackendInterface $lock, Config $system_file_config) { parent::__construct($configuration, $plugin_id, $plugin_definition, $serializer_formats, $logger); $this->fileSystem = $file_system; $this->entityTypeManager = $entity_type_manager; @@ -155,6 +164,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition $this->mimeTypeGuesser = $mime_type_guesser; $this->token = $token; $this->lock = $lock; + $this->systemFileConfig = $system_file_config; } /** @@ -173,7 +183,8 @@ public static function create(ContainerInterface $container, array $configuratio $container->get('current_user'), $container->get('file.mime_type.guesser'), $container->get('token'), - $container->get('lock') + $container->get('lock'), + $container->get('config.factory')->get('system.file') ); } @@ -451,19 +462,18 @@ protected function validate(FileInterface $file, FieldDefinitionInterface $field */ protected function prepareFilename($filename, array &$validators) { // Build the list of non-munged extensions if the caller provided them. - $extensions = $validators['file_validate_extensions'][0]; - if (!empty($extensions)) { + if (!empty($validators['file_validate_extensions']) && !empty($validators['file_validate_extensions'][0])) { // Munge the filename to protect against possible malicious extension // hiding within an unknown file type (ie: filename.html.foo). - $filename = file_munge_filename($filename, $extensions); + $filename = file_munge_filename($filename, $validators['file_validate_extensions'][0]); } // Rename potentially executable files, to help prevent exploits (i.e. will // rename filename.php.foo and filename.php to filename.php.foo.txt and // filename.php.txt, respectively). Don't rename if 'allow_insecure_uploads' // evaluates to TRUE. - if (!\Drupal::config('system.file')->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename) && (substr($filename, -4) != '.txt')) { + if (!$this->systemFileConfig->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename) && (substr($filename, -4) != '.txt')) { // The destination filename will also later be used to create the URI. $filename .= '.txt'; diff --git a/core/modules/hal/tests/src/Functional/EntityResource/File/FileUploadHalJsonTestBase.php b/core/modules/hal/tests/src/Functional/EntityResource/File/FileUploadHalJsonTestBase.php index 80060cfae0..58df16bcf2 100644 --- a/core/modules/hal/tests/src/Functional/EntityResource/File/FileUploadHalJsonTestBase.php +++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileUploadHalJsonTestBase.php @@ -5,6 +5,9 @@ use Drupal\Tests\rest\Functional\FileUploadResourceTestBase; use Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait; +/** + * Tests binary data file upload route for HAL JSON. + */ abstract class FileUploadHalJsonTestBase extends FileUploadResourceTestBase { use HalEntityNormalizationTrait; diff --git a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php index dc9f138081..f00e1f4b6d 100644 --- a/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php @@ -337,6 +337,34 @@ public function testFileUploadStrippedFilePath() { // Check the actual file data. It should have been written to the configured // directory, not /foobar/directory/example.txt. $this->assertSame($this->testFileData, file_get_contents('public://foobar/example.txt')); + + + $response = $this->fileRequest($uri, $this->testFileData, ['Content-Disposition' => 'file; filename="../../example_2.txt"']); + $this->assertSame(201, $response->getStatusCode()); + $expected = $this->getExpectedNormalizedEntity(2, 'example_2.txt', TRUE); + $this->assertResponseData($expected, $response); + + // Check the actual file data. It should have been written to the configured + // directory, not /foobar/directory/example.txt. + $this->assertSame($this->testFileData, file_get_contents('public://foobar/example_2.txt')); + $this->assertFalse(file_exists('../../example_2.txt')); + + // Check a path from the root. Extensions have to be empty to allow a file + // with no extension to pass validation. + $this->field->setSetting('file_extensions', '') + ->save(); + $this->refreshTestStateAfterRestConfigChange(); + + $response = $this->fileRequest($uri, $this->testFileData, ['Content-Disposition' => 'file; filename="/etc/passwd"']); + $this->assertSame(201, $response->getStatusCode()); + $expected = $this->getExpectedNormalizedEntity(3, 'passwd', TRUE); + // This mime will be guessed as there is no extension. + $expected['filemime'][0]['value'] = 'application/octet-stream'; + $this->assertResponseData($expected, $response); + + // Check the actual file data. It should have been written to the configured + // directory, not /foobar/directory/example.txt. + $this->assertSame($this->testFileData, file_get_contents('public://foobar/passwd')); } /** @@ -459,13 +487,13 @@ public function testFileUploadMaliciousExtension() { ->save(); $this->refreshTestStateAfterRestConfigChange(); - $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example.php"']); - $expected = $this->getExpectedNormalizedEntity(2, 'example.php.txt', TRUE); + $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_2.php"']); + $expected = $this->getExpectedNormalizedEntity(2, 'example_2.php.txt', TRUE); // Override the expected filesize. $expected['filesize'][0]['value'] = strlen($php_string); $this->assertResponseData($expected, $response); - $this->assertTrue(file_exists('public://foobar/example.php_.txt')); - $this->assertFalse(file_exists('public://foobar/example.php')); + $this->assertTrue(file_exists('public://foobar/example_2.php.txt')); + $this->assertFalse(file_exists('public://foobar/example_2.php')); // Now allow insecure uploads. \Drupal::configFactory() @@ -473,14 +501,38 @@ public function testFileUploadMaliciousExtension() { ->set('allow_insecure_uploads', TRUE) ->save(); - $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example.php"']); - $expected = $this->getExpectedNormalizedEntity(3, 'example.php', TRUE); + $response = $this->fileRequest($uri, $php_string, ['Content-Disposition' => 'filename="example_3.php"']); + $expected = $this->getExpectedNormalizedEntity(3, 'example_3.php', TRUE); // Override the expected filesize. $expected['filesize'][0]['value'] = strlen($php_string); // The file mime should also now be PHP. $expected['filemime'][0]['value'] = 'application/x-httpd-php'; $this->assertResponseData($expected, $response); - $this->assertTrue(file_exists('public://foobar/example.php.txt')); + $this->assertTrue(file_exists('public://foobar/example_3.php')); + } + + /** + * Tests using the file upload POST route no extension configured. + */ + public function testFileUploadNoExtensionSetting() { + $this->initAuthentication(); + + $this->provisionResource([static::$format], static::$auth ? [static::$auth] : [], ['POST']); + + $this->setUpAuthorization('POST'); + + $uri = Url::fromUri('base:' . static::$postUri); + + $this->field->setSetting('file_extensions', '') + ->save(); + $this->refreshTestStateAfterRestConfigChange(); + + // Test using a masked exploit file. + $response = $this->fileRequest($uri, $this->testFileData, ['Content-Disposition' => 'filename="example.txt"']); + $expected = $this->getExpectedNormalizedEntity(1, 'example.txt', TRUE); + + $this->assertResponseData($expected, $response); + $this->assertTrue(file_exists('public://foobar/example.txt')); } /**