Issue #3312198 by catch, alexpott, longwave, Nixou, Spokje, darvanen:...

Issue #3312198 by catch, alexpott, longwave, Nixou, Spokje, darvanen: Regression concerning the cache of private files

core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php

@@ -3,6 +3,7 @@
 namespace Drupal\Core\EventSubscriber;
 
 use Drupal\Component\Utility\Html;
+use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
 use Drupal\Core\Config\ConfigCrudEvent;
 use Drupal\Core\Config\ConfigEvents;
@@ -91,6 +92,14 @@ public function on404(ExceptionEvent $event) {
       if ($fast_paths && preg_match($fast_paths, $request->getPathInfo())) {
         $fast_404_html = strtr($config->get('fast_404.html'), ['@path' => Html::escape($request->getUri())]);
         $response = new HtmlResponse($fast_404_html, Response::HTTP_NOT_FOUND);
+        // Some routes such as system.files conditionally throw a
+        // NotFoundHttpException depending on URL parameters instead of just the
+        // route and route parameters, so add the URL cache context to account
+        // for this.
+        $cacheable_metadata = new CacheableMetadata();
+        $cacheable_metadata->setCacheContexts(['url']);
+        $cacheable_metadata->addCacheTags(['4xx-response']);
+        $response->addCacheableDependency($cacheable_metadata);
         $event->setResponse($response);
       }
     }

core/modules/file/src/Entity/File.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\file\Entity;
 
+use Drupal\Core\Cache\Cache;
 use Drupal\Core\Entity\ContentEntityBase;
 use Drupal\Core\Entity\EntityChangedTrait;
 use Drupal\Core\Entity\EntityStorageInterface;
@@ -268,4 +269,20 @@ public static function getDefaultEntityOwner() {
     return NULL;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function invalidateTagsOnSave($update) {
+    $tags = $this->getListCacheTagsToInvalidate();
+    // Always invalidate the 404 or 403 response cache because while files do
+    // not have a canonical URL as such, they may be served via routes such as
+    // private files.
+    // Creating or updating an entity may change a cached 403 or 404 response.
+    $tags = Cache::mergeTags($tags, ['4xx-response']);
+    if ($update) {
+      $tags = Cache::mergeTags($tags, $this->getCacheTagsToInvalidate());
+    }
+    Cache::invalidateTags($tags);
+  }
+
 }

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

@@ -82,7 +82,7 @@ protected function doPrivateFileTransferTest() {
 
     // Create a file.
     $contents = $this->randomMachineName(8);
-    $file = $this->createFile(NULL, $contents, 'private');
+    $file = $this->createFile($contents . '.txt', $contents, 'private');
     // Created private files without usage are by default not accessible
     // for a user different from the owner, but createFile always uses uid 1
     // as the owner of the files. Therefore make it permanent to allow access
@@ -108,19 +108,33 @@ protected function doPrivateFileTransferTest() {
     $this->assertSame($contents, $this->getSession()->getPage()->getContent(), 'Contents of the file are correct.');
     $http_client = $this->getHttpClient();
 
-    // Deny access to all downloads via a -1 header.
-    file_test_set_return('download', -1);
-    $response = $http_client->head($url, ['http_errors' => FALSE]);
-    $this->assertSame(403, $response->getStatusCode(), 'Correctly denied access to a file when file_test sets the header to -1.');
-
     // Try non-existent file.
     file_test_reset();
-    $url = $this->fileUrlGenerator->generateAbsoluteString('private://' . $this->randomMachineName());
-    $response = $http_client->head($url, ['http_errors' => FALSE]);
+    $not_found_url = $this->fileUrlGenerator->generateAbsoluteString('private://' . $this->randomMachineName() . '.txt');
+    $response = $http_client->head($not_found_url, ['http_errors' => FALSE]);
     $this->assertSame(404, $response->getStatusCode(), 'Correctly returned 404 response for a non-existent file.');
     // Assert that hook_file_download is not called.
     $this->assertEquals([], \Drupal::state()->get('file_test.results')['download']);
 
+    // Having tried a non-existent file, try the original file again to ensure
+    // it's returned instead of a 404 response.
+    // Set file_test access header to allow the download.
+    file_test_reset();
+    file_test_set_return('download', ['x-foo' => 'Bar']);
+    $this->drupalGet($url);
+    // Verify that header is set by file_test module on private download.
+    $this->assertSession()->responseHeaderEquals('x-foo', 'Bar');
+    // Verify that page cache is disabled on private file download.
+    $this->assertSession()->responseHeaderDoesNotExist('x-drupal-cache');
+    $this->assertSession()->statusCodeEquals(200);
+    // Test that the file transferred correctly.
+    $this->assertSame($contents, $this->getSession()->getPage()->getContent(), 'Contents of the file are correct.');
+
+    // Deny access to all downloads via a -1 header.
+    file_test_set_return('download', -1);
+    $response = $http_client->head($url, ['http_errors' => FALSE]);
+    $this->assertSame(403, $response->getStatusCode(), 'Correctly denied access to a file when file_test sets the header to -1.');
+
     // Try requesting the private file url without a file specified.
     file_test_reset();
     $this->drupalGet('/system/files');

core/tests/Drupal/FunctionalTests/EventSubscriber/Fast404Test.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\FunctionalTests\EventSubscriber;
 
+use Drupal\file\Entity\File;
 use Drupal\Tests\BrowserTestBase;
 
 /**
@@ -18,6 +19,11 @@ class Fast404Test extends BrowserTestBase {
    */
   protected $defaultTheme = 'stark';
 
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['file'];
+
   /**
    * Tests the fast 404 functionality.
    */
@@ -69,4 +75,33 @@ public function testFast404(): void {
     $this->assertSession()->responseHeaderContains('X-Generator', 'Drupal');
   }
 
+  /**
+   * Tests the fast 404 functionality.
+   */
+  public function testFast404PrivateFiles(): void {
+    $admin = $this->createUser([], NULL, TRUE);
+    $this->drupalLogin($admin);
+
+    $file_url = 'system/files/test/private-file-test.txt';
+    $this->drupalGet($file_url);
+    $this->assertSession()->statusCodeEquals(404);
+    $this->drupalGet($file_url);
+    $this->assertSession()->statusCodeEquals(404);
+
+    // Create a private file for testing accessible by the admin user.
+    \Drupal::service('file_system')->mkdir($this->privateFilesDirectory . '/test');
+    $filepath = 'private://test/private-file-test.txt';
+    $contents = "file_put_contents() doesn't seem to appreciate empty strings so let's put in some data.";
+    file_put_contents($filepath, $contents);
+    $file = File::create([
+      'uri' => $filepath,
+      'uid' => $admin->id(),
+    ]);
+    $file->save();
+
+    $this->drupalGet($file_url);
+    $this->assertSession()->statusCodeEquals(200);
+    $this->assertSession()->pageTextContains($contents);
+  }
+
 }