Verified Commit 61c592bf authored by Dave Long's avatar Dave Long
Browse files

task: #3563051 Remove unnecessary status messages from file_save_upload() on file rename 

By: lauriii
By: pameeela
By: longwave
By: catch
parent ba110231

core/modules/file/file.module

+3 −9
Original line number Diff line number Diff line
@@ -234,15 +234,9 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL 
        continue;

      }

      $file = $result->getFile();

      // If the filename has been modified, let the user know.

      if ($result->isRenamed()) {

      // Log security renames to help detect potential malicious uploads.

      if ($result->isSecurityRename()) {

          $message = t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]);

        }

        else {

          $message = t('Your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]);

        }

        \Drupal::messenger()->addStatus($message);

        \Drupal::logger('file')->notice('For security reasons, the uploaded file %original_filename has been renamed to %filename.', ['%original_filename' => $result->getOriginalFilename(), '%filename' => $file->getFilename()]);

      }

      $files[$i] = $file;

    }

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

+0 −6
Original line number Diff line number Diff line
@@ -247,7 +247,6 @@ public function testHandleDangerousFile(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->responseContains('For security reasons, your upload has been renamed to <em class="placeholder">' . $this->phpFile->filename . '_.txt</em>');

    $this->assertSession()->pageTextContains('File MIME type is text/plain.');

    $this->assertSession()->pageTextContains("You WIN!");
@@ -263,7 +262,6 @@ public function testHandleDangerousFile(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is {$this->phpFile->filename}");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -322,7 +320,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is {$munged_filename}");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -346,7 +343,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is {$munged_filename}");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -366,7 +362,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is {$this->image->getFilename()}");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -386,7 +381,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("Epic upload FAIL!");



    // Check that the correct hooks were called.

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

+31 −18
Original line number Diff line number Diff line
@@ -5,6 +5,7 @@ 
namespace Drupal\Tests\file\Functional;



use Drupal\Component\Utility\Html;

use Drupal\Core\Database\Database;

use Drupal\Core\File\FileExists;

use Drupal\Core\Url;

use Drupal\file\Entity\File;
@@ -279,7 +280,6 @@ public function testHandleExtension(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->responseContains('For security reasons, your upload has been renamed to <em class="placeholder">' . $this->phpFile->filename . '_.txt</em>');

    $this->assertSession()->pageTextContains('File name is php-2.php_.txt.');

    $this->assertSession()->pageTextContains('File MIME type is text/plain.');

    $this->assertSession()->pageTextContains("You WIN!");
@@ -304,7 +304,6 @@ public function testHandleDangerousFile(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->responseContains('For security reasons, your upload has been renamed to <em class="placeholder">' . $this->phpFile->filename . '_.txt</em>');

    $this->assertSession()->pageTextContains('File name is php-2.php_.txt.');

    $this->assertSession()->pageTextContains('File MIME type is text/plain.');

    $this->assertSession()->pageTextContains("You WIN!");
@@ -321,7 +320,6 @@ public function testHandleDangerousFile(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains('File name is php-2.php.');

    $this->assertSession()->pageTextContains("You WIN!");
@@ -381,6 +379,36 @@ public function testHandleDangerousFile(): void { 
    $this->assertFileHooksCalled(['validate']);

  }



  /**

   * Tests that security renames are logged to watchdog.

   */

  public function testSecurityRenameLogging(): void {

    // Clear the watchdog log to ensure we only see entries from this test.

    Database::getConnection()->delete('watchdog')->execute();



    // Upload a dangerous file that will be renamed for security reasons.

    $edit = [

      'file_test_replace' => FileExists::Replace->name,

      'files[file_test_upload]' => \Drupal::service('file_system')->realpath($this->phpFile->uri),

      'is_image_file' => FALSE,

      'extensions' => 'php txt',

    ];



    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains("You WIN!");



    // Verify that the security rename was logged.

    $query = Database::getConnection()->select('watchdog', 'w')

      ->fields('w', ['message', 'variables'])

      ->condition('type', 'file')

      ->condition('message', '%security reasons%', 'LIKE')

      ->execute();

    $log_entry = $query->fetchObject();

    $this->assertNotNull($log_entry, 'A security rename log entry was created.');

  }



  /**

   * Test dangerous file handling.

   */
@@ -411,7 +439,6 @@ public function testHandleDotFile(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed to test.');

    $this->assertSession()->pageTextContains('File name is test.');

    $this->assertSession()->pageTextContains('You WIN!');
@@ -427,7 +454,6 @@ public function testHandleDotFile(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed to test_0.');

    $this->assertSession()->pageTextContains('File name is test_0.');

    $this->assertSession()->pageTextContains('You WIN!');
@@ -465,7 +491,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is $munged_filename");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -486,7 +511,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is {$this->image->getFilename()}");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -506,7 +530,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is {$this->image->getFilename()}");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -528,7 +551,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is image-test.png_.php_.png");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -547,7 +569,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is image-test.png_.php__0.png");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -568,7 +589,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/upload');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("File name is image-test.png_.cgi_.png_.txt");

    $this->assertSession()->pageTextContains("You WIN!");
@@ -588,7 +608,6 @@ public function testHandleFileMunge(): void { 
    $this->drupalGet('file-test/save_upload_from_form_test');

    $this->submitForm($edit, 'Submit');

    $this->assertSession()->statusCodeEquals(200);

    $this->assertSession()->pageTextNotContains('For security reasons, your upload has been renamed');

    $this->assertSession()->pageTextContains("Epic upload FAIL!");



    // Check that the correct hooks were called.
@@ -809,9 +828,6 @@ public function testSanitization(): void { 
    $this->assertSession()->statusCodeEquals(200);

    // Test that the file name has been transliterated.

    $this->assertSession()->responseContains('File name is TEXT-oe.txt.');

    // Make sure we got a message about the rename.

    $message = 'Your upload has been renamed to <em class="placeholder">TEXT-oe.txt</em>';

    $this->assertSession()->responseContains($message);



    // Generate another file with a name with All The Things(tm) we care about.

    $file = $this->generateFile('S  Pácê--táb#	#--🙈', 64, 5, 'text');
@@ -893,9 +909,6 @@ public function testSanitization(): void { 
    $this->assertSession()->statusCodeEquals(200);

    // Make sure all the sanitization options work as intended.

    $this->assertSession()->responseContains('File name is s-pace-tab-2.txt.');

    // Make sure we got a message about the rename.

    $message = 'Your upload has been renamed to <em class="placeholder">s-pace-tab-2.txt</em>';

    $this->assertSession()->responseContains($message);

  }



}