Unverified Commit e1037701 authored by Alex Pott's avatar Alex Pott
Browse files

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
parent e35d0860

core/modules/file/file.module

+25 −0
Original line number Diff line number Diff line
@@ -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

+29 −0
Original line number Diff line number Diff line
@@ -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

+28 −0
Original line number Diff line number Diff line
@@ -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

+31 −0
Original line number Diff line number Diff line
@@ -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.

   *