Issue #2869855 by mcdruid, Krzysztof Domański, wturrell, rjg, manarth,...

Issue #2869855 by mcdruid, Krzysztof Domański, wturrell, rjg, manarth, dpagini, Wim Leers, catch, xjm, cilefen: Race condition in file_save_upload causes data loss

core/modules/file/file.module

@@ -995,6 +995,9 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   $values['filemime'] = \Drupal::service('file.mime_type.guesser')->guess($values['filename']);
   $file = File::create($values);
 
+  // Ensure that the file object is validated.
+  $file->setValidationRequired(TRUE);
+
   $extensions = '';
   if (isset($validators['file_validate_extensions'])) {
     if (isset($validators['file_validate_extensions'][0])) {
@@ -1124,6 +1127,28 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   // due to an existing file.
   $file->setFilename(\Drupal::service('file_system')->basename($file->destination));
 
+  // We can now validate the file object itself before it's saved.
+  $violations = $file->validate();
+  foreach ($violations as $violation) {
+    $errors[] = $violation->getMessage();
+  }
+  if (!empty($errors)) {
+    $message = [
+      'error' => [
+        '#markup' => t('The specified file %name could not be uploaded.', ['%name' => $file->getFilename()]),
+      ],
+      'item_list' => [
+        '#theme' => 'item_list',
+        '#items' => $errors,
+      ],
+    ];
+    // @todo Add support for render arrays in
+    // \Drupal\Core\Messenger\MessengerInterface::addMessage()?
+    // @see https://www.drupal.org/node/2505497.
+    \Drupal::messenger()->addError(\Drupal::service('renderer')->renderPlain($message));
+    return FALSE;
+  }
+
   // If we made it this far it's safe to record this file in the database.
   $file->save();
 

core/modules/file/tests/src/Functional/SaveUploadTest.php

@@ -134,6 +134,35 @@ public function testNormal() {
     $this->assertTrue(is_file('temporary://' . $dir . '/' . trim(\Drupal::service('file_system')->basename($image3_realpath))));
   }
 
+  /**
+   * Test uploading a duplicate file.
+   */
+  public function testDuplicate() {
+    // It should not be possible to create two managed files with the same URI.
+    $image1 = current($this->drupalGetTestFiles('image'));
+    $edit = ['files[file_test_upload]' => \Drupal::service('file_system')->realpath($image1->uri)];
+    $this->drupalPostForm('file-test/upload', $edit, t('Submit'));
+    $max_fid_after = (int) \Drupal::entityQueryAggregate('file')->aggregate('fid', 'max')->execute()[0]['fid_max'];
+    $file1 = File::load($max_fid_after);
+
+    // Simulate a race condition where two files are uploaded at almost the same
+    // time, by removing the first uploaded file from disk (leaving the entry in
+    // the file_managed table) before trying to upload another file with the
+    // same name.
+    unlink(\Drupal::service('file_system')->realpath($file1->getFileUri()));
+
+    $image2 = $image1;
+    $edit = ['files[file_test_upload]' => \Drupal::service('file_system')->realpath($image2->uri)];
+    $this->drupalPostForm('file-test/upload', $edit, t('Submit'));
+    // Received a 200 response for posted test file.
+    $this->assertResponse(200);
+    $message = t('The file %file already exists. Enter a unique file URI.', ['%file' => $file1->getFileUri()]);
+    $this->assertRaw($message);
+    $max_fid_before_duplicate = $max_fid_after;
+    $max_fid_after = (int) \Drupal::entityQueryAggregate('file')->aggregate('fid', 'max')->execute()[0]['fid_max'];
+    $this->assertEqual($max_fid_before_duplicate, $max_fid_after, 'A new managed file was not created.');
+  }
+
   /**
    * Test extension handling.
    */

core/modules/jsonapi/tests/src/Functional/FileUploadTest.php

@@ -440,6 +440,34 @@ public function testPostFileUploadDuplicateFile() {
     $this->assertSame($this->testFileData, file_get_contents('public://foobar/example_0.txt'));
   }
 
+  /**
+   * Tests using the file upload POST route twice, simulating a race condition.
+   *
+   * A validation error should occur when the filenames are not unique.
+   */
+  public function testPostFileUploadDuplicateFileRaceCondition() {
+    $this->setUpAuthorization('POST');
+    $this->config('jsonapi.settings')->set('read_only', FALSE)->save(TRUE);
+
+    $uri = Url::fromUri('base:' . static::$postUri);
+
+    // This request will have the default 'application/octet-stream' content
+    // type header.
+    $response = $this->fileRequest($uri, $this->testFileData);
+
+    $this->assertSame(201, $response->getStatusCode());
+
+    // Simulate a race condition where two files are uploaded at almost the same
+    // time, by removing the first uploaded file from disk (leaving the entry in
+    // the file_managed table) before trying to upload another file with the
+    // same name.
+    unlink(\Drupal::service('file_system')->realpath('public://foobar/example.txt'));
+
+    // Make the same request again. The upload should fail validation.
+    $response = $this->fileRequest($uri, $this->testFileData);
+    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: file validation failed.\nThe file public://foobar/example.txt already exists. Enter a unique file URI."), $uri, $response);
+  }
+
   /**
    * Tests using the file upload route with any path prefixes being stripped.
    *

core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php

@@ -310,6 +310,37 @@ public function testPostFileUploadDuplicateFile() {
     $this->assertSame($this->testFileData, file_get_contents('public://foobar/example_0.txt'));
   }
 
+  /**
+   * Tests using the file upload POST route twice, simulating a race condition.
+   *
+   * A validation error should occur when the filenames are not unique.
+   */
+  public function testPostFileUploadDuplicateFileRaceCondition() {
+    $this->initAuthentication();
+
+    $this->provisionResource([static::$format], static::$auth ? [static::$auth] : [], ['POST']);
+
+    $this->setUpAuthorization('POST');
+
+    $uri = Url::fromUri('base:' . static::$postUri);
+
+    // This request will have the default 'application/octet-stream' content
+    // type header.
+    $response = $this->fileRequest($uri, $this->testFileData);
+
+    $this->assertSame(201, $response->getStatusCode());
+
+    // Simulate a race condition where two files are uploaded at almost the same
+    // time, by removing the first uploaded file from disk (leaving the entry in
+    // the file_managed table) before trying to upload another file with the
+    // same name.
+    unlink(\Drupal::service('file_system')->realpath('public://foobar/example.txt'));
+
+    // Make the same request again. The upload should fail validation.
+    $response = $this->fileRequest($uri, $this->testFileData);
+    $this->assertResourceErrorResponse(422, PlainTextOutput::renderFromHtml("Unprocessable Entity: validation failed.\nuri: The file public://foobar/example.txt already exists. Enter a unique file URI.\n"), $response);
+  }
+
   /**
    * Tests using the file upload route with any path prefixes being stripped.
    *