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
Loading
Loading
Loading
Loading
+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();

+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.
   */
+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.
   *
+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.
   *