Don't trigger hook_file_download when no file is requested

I guess this makes sense, but let's add a test for this as well.

Awesome, that test looks like it provides sufficient coverage.

This looks sensible but would a 400 or 404 be more appropriate for the error?

hmm would be good to get a test-only patch to confirm exactly what happens now, but if so maintaining the same behaviour while doing the shortcut seems OK.

I've run the test without the fix.

sudo -u _www ./vendor/bin/phpunit -v -c ./core core/modules/file/tests/src/Functional/FileManagedAccessTest.php --filter testFileAccess
PHPUnit 6.5.13 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.13
Configuration: /Users/alex/dev/sites/drupal8alt.dev/core/phpunit.xml

Testing Drupal\Tests\file\Functional\FileManagedAccessTest
.                                                                   1 / 1 (100%)

Time: 6.71 seconds, Memory: 6.00MB

OK (1 test, 19 assertions)

So the behaviour is unchanged (as expected). I'm not sure we can test for this early return but it is good to have an explicit test of calling system/files with no query string. If we had the ability to use expressions in our routes we could do something like https://symfony.com/doc/current/routing/conditions.html but we don't have that.

+++ b/core/modules/system/src/FileDownloadController.php
@@ -40,6 +40,11 @@ class FileDownloadController extends ControllerBase {
+    if (!$target) {
+      throw new AccessDeniedHttpException();
+    }

This condition feels a bit loose. Given PHP's fun with false equivalence. How about we make this super-explicit and do

    if (!$request->query->has('file')) {
      throw new AccessDeniedHttpException();
    }

prior to the getting.

1. Nice test!

2. +++ b/core/modules/file/tests/src/Functional/FileManagedAccessTest.php
   @@ -78,6 +78,16 @@ public function testFileAccess() {
   +    // There should be no invocations of hook_file_download() with an empty
   +    // file.
   

   It's not an empty file :) - I think we need to be explicit that we mean no file query parameter. However this does pose the question below...

3. +++ b/core/modules/system/src/FileDownloadController.php
   @@ -39,7 +39,12 @@ class FileDownloadController extends ControllerBase {
   +    if (!$request->query->has('file')) {
   +      throw new AccessDeniedHttpException();
   +    }
   

   Hmmm... so now the words "empty file" are making me think about how we should handle URLs that end like ?file=.

   Also reading the whole \Drupal\system\FileDownloadController::download() is instructive.

     public function download(Request $request, $scheme = 'private') {
       $target = $request->query->get('file');
       // Merge remaining path arguments into relative file path.
       $uri = $scheme . '://' . $target;
   
       if (file_stream_wrapper_valid_scheme($scheme) && file_exists($uri)) {
         // Let other modules provide headers and controls access to the file.
         $headers = $this->moduleHandler()->invokeAll('file_download', [$uri]);
   
         foreach ($headers as $result) {
           if ($result == -1) {
             throw new AccessDeniedHttpException();
           }
         }
   
         if (count($headers)) {
           // \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
           // sets response as not cacheable if the Cache-Control header is not
           // already modified. We pass in FALSE for non-private schemes for the
           // $public parameter to make sure we don't change the headers.
           return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
         }
   
         throw new AccessDeniedHttpException();
       }
   
       throw new NotFoundHttpException();
     }
   

   This makes me wonder whether or not the current behaviour of AccessDeniedHttpException is correct. I guess there is no reason to change this. But for me pass no file is very similar to the file_exists() check returning false. OTOH something does exist - a directory - but this should not give access to them so perhaps a 403 is the correct response.

   And thinking even more about this check I wonder if we should be doing this instead if (file_stream_wrapper_valid_scheme($scheme) && is_file($uri)) { because I think that BinaryFileResponse cannot possibly work with directories. This would deal with a $uri like private:// too.

   Perhaps the best change would be

       if (file_stream_wrapper_valid_scheme($scheme) && file_exists($uri)) {
         if (is_dir($uri)) {
           throw new AccessDeniedHttpException();
         }
         // Let other modules provide headers and controls access to the file.
         $headers = $this->moduleHandler()->invokeAll('file_download', [$uri]);
   

   Should it be is_dir() or !is_file() - i.e should we allow symbolic links here... I dunno - maybe a follow up to discuss. Ah looking at the Symfony code eventually \Symfony\Component\HttpFoundation\File\File::__construct() gets called and that does

           if ($checkPath && !is_file($path)) {
               throw new FileNotFoundException($path);
           }
   

   So I think maybe the best fix would be to do:

       if (file_stream_wrapper_valid_scheme($scheme) && is_file($uri)) {
   

   This will change to throw 404s but it will work for empty file query strings and when they point to directories or links neither of which are supported.

> It's not an empty file :) - I think we need to be explicit that we mean no file query parameter.

Yeah, which is why I was confused initially when I saw this issue title :)

+    // Verify access denied if no file is requested.
+    $hook_called_count = \Drupal::state()->get('file_test.hook_file_download_called', 0);
+    $this->drupalGet('/system/files');
+    $this->assertSession()->statusCodeEquals(404);

The comment mentions access denied, but the assertion expects 404.

Patch also needs a reroll, no longer applies against 8.9.x-dev (FileManagedAccessTest was converted to a Kernel test in #3048434: Convert FileManagedAccessTest into a Kernel test, and there is a conflict in FileDownloadController.php). Both seem easy to resolve, so tagging as a novice issue)

Forget to set to NW because of issues mentioned in #19.

Working on it in DrupalCon Amsterdam 2019

YAY! First time mentoring at DrupalCon Amsterdam 2019.

Helping peximo on this issue at DrupalCon Amsterdam 2019.

I'm helping peximo too at DrupalCon Amsterdam 2019

I'm here to help too!

+++ b/core/modules/system/src/FileDownloadController.php
@@ -43,7 +43,7 @@ public function download(Request $request, $scheme = 'private') {
+    if (file_stream_wrapper_valid_scheme($scheme) && is_file($uri)) {

Is it necessary to check the filesize for downloading?

is_file($path) && (filesize($path) > 0)

Helping on this issue as a mentor on DrupalCon Amsterdam 2019

We need a some advice about how to rewrite the test.

We talked with Lendude and concluded we need a new test extending BrowserTestBase because there is an HTTP request.

Since FileManagedAccessTest was converted to a Kernel test but we need an HTTP request in this case, I've added a new Browser test. Also I've changed the comment and rerolled the patch.

Thanks @peximo, looks good!

+++ b/core/modules/file/tests/src/Functional/FileManagedDownloadTest.php
@@ -0,0 +1,24 @@
+    $hook_called_count = \Drupal::state()->get('file_test.hook_file_download_called', 0);

Since your test now runs in isolation and for every test a new environment is set up, there is no reason to store the previous value of file_test.hook_file_download_called, You can just assert that at the end of the test that the hook_file_download count === 0.

Can you update the test and provide an interdiff (see Creating an interdiff)?

Thanks @mr.baileys, updated

Seems good to me! I applied the patch and everything was fine also testing with xdebug to make sure that the hook was not called without file argument.
Also the test is green for me.

The patch could not be applied in #34, so still some work to be done.

Missing added file.

retagging

Here's the interdiff of 34->38

I think we are almost there!

+++ b/core/modules/file/tests/src/Functional/FileManagedDownloadTest.php
@@ -0,0 +1,23 @@
+    $this->assertEquals(\Drupal::state()->get('file_test.hook_file_download_called', 0), 0, 'hook_file_download() was triggered but should not have been.');

The message in an assertion needs to be a statement that you expect to always be true, so instead of "... was triggered but should not have been", the assertion message should be something like "hook_file_download() was not triggered."

Updated the message.

Looks good, and the tests pass.

@peximo, just one more question since the test has changed quite a bit since #12: could you upload a "test only" patch? If that one fails, and the combined patch succeeds (as it did in #43), it proves that the test is correct. If the test-only patch succeeds, we know it's not correctly testing the issue.

See #44 the TEST-ONLY.patch needs to fail. So I've set it to needs review.

As mentioned earlier, looks good.

+++ b/core/modules/file/tests/file_test/file_test.module
@@ -150,6 +150,10 @@ function file_test_file_validate(File $file) {
+  $calls = \Drupal::state()->get('file_test.hook_file_download_called', 0) + 1;

The state is set to 0 + 1 if empty, or the number of calls + 1 if already called.

+++ b/core/modules/file/tests/src/Functional/FileManagedDownloadTest.php
@@ -0,0 +1,23 @@
+    $this->assertEquals(\Drupal::state()->get('file_test.hook_file_download_called', 0), 0, 'hook_file_download() was triggered but should not have been.');

Check the state Equals still 0.

I think this can be set to RTBC?

+++ b/core/modules/file/tests/file_test/file_test.module
@@ -150,6 +150,10 @@ function file_test_file_validate(File $file) {
+  // Tracks the number of times this hook is called.
+  $calls = \Drupal::state()->get('file_test.hook_file_download_called', 0) + 1;
+  \Drupal::state()->set('file_test.hook_file_download_called', $calls);

+++ b/core/modules/file/tests/src/Functional/FileManagedDownloadTest.php
@@ -0,0 +1,23 @@
+    $this->assertEquals(\Drupal::state()->get('file_test.hook_file_download_called', 0), 0, 'hook_file_download() was not triggered.');

All the complexity of have this as a count is not needed for the test. It should be a flag ie TRUE or FALSE. Also we have no test that the flag/count ever gets set so this test is very vulnerable to become a false positive - testing for the absence of doing something is always tricky.

It is always helpful if negative tests like this are placed alongside positive tests.

We can leverage existing file test plumbing to test this and add the test to core/modules/file/tests/src/Functional/DownloadTest.php

The code looks good, the patch applied clearly, during manual testing the result was as expected. Thanks!

@init90, Thanks for the review. Can you please apply a screenshot for the manual testing results.

Rerolled from #51

@neslee-canil-pinto thanks for your message. Actually, I don't think that screenshots here are needed. One difference that we have, without patch when we don't specify a file we will get 403-page status and potential errors from contrib/custom code which don't check in hook_file_download if the file really exists. In the same case with the patch, we will get 404-page status and no potential errors, because hook_file_download won't be executed.

@jungle thanks for the reroll. I don't know why but the patch from #51 applies clearly in my machine with the last update in 8.9.x branch.

Credited everyone who worked with @peximo on this at Drupalcon.

Committed 24e1692 and pushed to 9.1.x. Thanks!

diff --git a/core/modules/file/tests/src/Functional/DownloadTest.php b/core/modules/file/tests/src/Functional/DownloadTest.php
index 6765f704cf..01f2e21207 100644
--- a/core/modules/file/tests/src/Functional/DownloadTest.php
+++ b/core/modules/file/tests/src/Functional/DownloadTest.php
@@ -95,7 +95,7 @@ protected function doPrivateFileTransferTest() {
     $url = file_create_url('private://' . $this->randomMachineName());
     $response = $http_client->head($url, ['http_errors' => FALSE]);
     $this->assertSame(404, $response->getStatusCode(), 'Correctly returned 404 response for a non-existent file.');
-    // Ensure hook_file_download is fired correctly.
+    // Assert that hook_file_download is not called.
     $this->assertEquals([], \Drupal::state()->get('file_test.results')['download']);
 
     // Try requesting the private file url without a file specified.

I improved the test text on commit. Going to ping release managers about backporting this to 9.0.x and 8.9.x

Discussed with @catch and we agreed to cherry-pick this to 8.9.x and 9.0.x

Is there a version of this code for drupal 7?