Proper private file support for images uploaded via EditorImageDialog

This could perhaps be triaged up to major priority.

We simply never added support for private files. You're the first to report/request this AFAIK, which explains how it went unnoticed all this time. Which is why this is IMO a feature request, not a bug.

Related reading: https://www.drupal.org/documentation/modules/file#content-accessing-priv...

#4: where exactly did I say that? I don't see it in comment #100 that you linked to. I'd love to read my own thinking about this from back then :)

You assigned this to yourself, so it sounds like you want to take this on. Let's start with writing a test that shows you can upload files while using the private file system, but you can't download them. That would then make it more clear what else we need.

Aha! Thanks for pointing that out :) (I was looking in the issue comments, that's why I was not able to find it.)

Wow, great first pass! :) Thanks so much!

The steps indeed include JS if you do it manually. But you can just as well do it without JS. You can use File::create([…])->save() to "upload" a file to the private:// stream. Next, you can interact with the form without JS like this test is already doing: \Drupal\Tests\editor\Kernel\EditorImageDialogTest::testEditorImageDialog().

RE: Basically _filter_html_image_secure_process() improperly recognizes URIs of private files as non-local. — yep, that's a known bug. +1 for excluding that from the test, so that it doesn't interfere.

Requested new tests for both — let's see :)

The failure root cause:

PHP Fatal error:  Class 'Drupal\editor\Tests\EditorPrivateFileReferenceFilterTest' not found in /var/www/html/core/scripts/run-tests.sh on line 745

1. +++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
   @@ -0,0 +1,70 @@
   +namespace Drupal\Tests\editor\Kernel;
   

   This is why the patch always results in a fail: the namespace is wrong.

2. +++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
   @@ -0,0 +1,70 @@
   +  // Install standard.profile.
   

   /**
    * {inheritdoc}
    */
   

   Also… let's please not do this — installing the "Standard" install profile is very very time consuming. We should only install the necessary modules, and then manually create the necessary configuration.

3. +++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
   @@ -0,0 +1,70 @@
   +    // Visit /admin/config/content/formats/manage/basic_html.
   +    $edit = [
   +      // Disable "Restrict images to this site".
   +      'filters[filter_html_image_secure][status]' => FALSE,
   +      // Set "File storage" to "Private local files served by Drupal.".
   +      'editor[settings][plugins][drupalimage][image_upload][scheme]' => 'private',
   +      // Remove 'inline-images'.
   +      'editor[settings][plugins][drupalimage][image_upload][directory]' => '',
   +    ];
   +    // Click "Save configuration".
   +    $this->drupalPostForm('/admin/config/content/formats/manage/basic_html', $edit, t('Save configuration'));
   

   This works, but is very slow. There's no reason for us to do this via the UI. We can just create the necessary configuration directly.

4. In fact, why even use WebTestBase? It's very slow. Let's use KernelTestBase. In fact … I think you can just add a new test method to \Drupal\Tests\editor\Kernel\EditorFileReferenceFilterTest :)

Great progress here! :) However, could you provide interdiffs from now on? https://www.drupal.org/documentation/git/interdiff

1. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +  $files = entity_load_multiple_by_properties('file', array('uri' => $uri));
   

   Nit: s/array()/[]/

   More importantly: let's not call a deprecated API function.

2. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +      // Since some database servers sometimes use a case-insensitive comparison
   +      // by default, double check that the filename is an exact match.
   

   Woah!

3. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +  // Temporary files are handled by file_file_download(), so nothing to do here
   +  // about them.
   

   This comment seems to be dangling. Does this belong with the !isset($file)?

4. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +  // Find out which (if any) fields of this type contain the file.
   +  $references = editor_get_file_references($file, EntityStorageInterface::FIELD_LOAD_CURRENT);
   

   "which fields" vs "$references" -> very confusing.

5. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +  // an image preview on a node/add form) in which case, allow download by the
   

   node/add form -> node creation form

6. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +  // Editor.module MUST NOT call $file->access() here (like file_file_download()
   +  // does) as checking the 'download' access to a file entity would end up in
   +  // FileAccessControlHandler->checkAccess() and ->getFileReferences(), which
   +  // calls file_get_file_references() again. This latter one would allow
   +  // downloading files only handled by the file.module, which is exactly not the
   +  // case right here.
   +//  if (!($return = $file->access('download', NULL, TRUE))) {
   +//    return -1;
   +//  }
   

   This is very confusing.

   I think that:

   1. removing the "again"
   2. removing the commented out code

   would help a lot

7. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   + * @param int $age
   + *   (optional) A constant that specifies which references to count. Use
   + *   EntityStorageInterface::FIELD_LOAD_REVISION (the default) to retrieve all
   + *   references within all revisions or
   + *   EntityStorageInterface::FIELD_LOAD_CURRENT to retrieve references only in
   + *   the current revisions of all entities that have references to this file.
   

   This extra complexity only exists here because this was based off of file_get_file_references().

   This is only ever called with FIELD_LOAD_CURRENT. So let's only support that.

8. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +function editor_get_file_references(FileInterface $file, $age = EntityStorageInterface::FIELD_LOAD_REVISION) {
   

   Let's prefix this with an underscore, to signal this is private.

   Let's also add @internal.

   Both of those things ensure we can change this function's implementation and interface over time. This is NOT a public API.

9. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,131 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +    $usage_list = \Drupal::service('file.usage')->listUsage($file);
   +    $file_usage_list = isset($usage_list['editor']) ? $usage_list['editor'] : array();
   

   In fact, why do we need to get all references? We never use it. All we need to know is whether the file that is being downloaded is in fact used, and used by editor module somewhere.

   The reason file module's hook_file_download() calls file_get_file_references() is because it provides the data it needs. It provides much more than is necessary, but it also contains what is necessary. So that's what it uses. But in fact that module could also just look at the file.usage service's entries for the file module.

   This entire function with all its complexity could then go away.

   And instead we'd just have

   $usage_list = \Drupal::service('file.usage')->listUsage($file);
   $is_accessible = !empty($usage_list['editor']);
   

   in editor_file_download().

   *So* much simpler.

   Or what am I missing?

#30: awesome!

RE: cleaning up File module in the same way: we can't do that; there could be contrib modules relying on this code, since the code there is not declared internal, hence it's a public API.

P.S.: upload the test-only patch first, then testbot won't mark this issue needs work :) Silly & annoying small thing you have to know when using d.o.

This now pretty much looks ready, but just needs a bit more test coverage.

1. +++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
   @@ -0,0 +1,70 @@
   +class EditorPrivateFileReferenceFilterTest extends WebTestBase {
   

   We shouldn't add new WebTestBase tests. Please use BrowserTestBase instead – see #2469723: New PHPUnit based classes added for testing: BrowserTestBase and JavascriptTestBase

2. +++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
   @@ -0,0 +1,70 @@
   +    // Do the actual test. The image should be visible for anonymous.
   +    $this->drupalGet($src);
   +    $this->assertResponse(200, 'Image is downloadable as anonymous.');
   

   This is only testing one case — the most common case.

   It's not yet testing the case of a file not yet being permanent, yet uploaded by the current user.

Thanks for your super awesome work here!

Oh and since this is a bugfix, this can go in 8.1!

This looks great! Thanks a lot for your awesome work here! :)

Sorry I did not use the proper tag for D8 Media.

Looks like a retest of this passed fine above :)

Retesting again. Random fail again.

+1 RTBC
Tested with Core image plugin and with Editor File contrib one.
Thank you all for your work :)

PS: private images are still considered as external images by the "Restrict images to this site" filter with this patch. Did someone open a followup issue?

Random fail in ConfigImportAllTest.

1. +++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
   @@ -0,0 +1,87 @@
   +    $this->drupalGet('/node/' . $node->id());
   

   What's the purpose of this get? Nothing is asserted afterwards? If it is necessary for the test it needs a comment.

2. +++ b/core/modules/editor/editor.module
   @@ -468,6 +470,60 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +  // Stop processing if there are no references in order to avoid returning
   +  // headers for files controlled by other modules. Make an exception for
   +  // temporary files where the host entity has not yet been saved (for example,
   +  // an image preview on a node creation form) in which case, allow download by
   +  // the file's owner.
   +  if (empty($usage_list['editor']) && ($file->isPermanent() || $file->getOwnerId() != \Drupal::currentUser()->id())) {
   

   This logic in the if does not seem to match the comment.

   Isn't the matching logic:

   if (empty($usage_list['editor']) && (!$file->isPermanent() && $file->getOwnerId() != \Drupal::currentUser()->id())) { 
   

   If we can make this simpler by breaking it up into separate ifs i'd encourage that.

1. Nice nitpicky catch! I also can't tell what the purpose of that is. AFAICT it can be removed. I bet it was there for debugging.
2. We have the exact same thing in file_file_download():
   

     // Stop processing if there are no references in order to avoid returning
     // headers for files controlled by other modules. Make an exception for
     // temporary files where the host entity has not yet been saved (for example,
     // an image preview on a node/add form) in which case, allow download by the
     // file's owner.
     if (empty($references) && ($file->isPermanent() || $file->getOwnerId() != \Drupal::currentUser()->id())) {
       return;
     }
   

Still NW for point 1.

Oh ok re #51.2 I'm always wary of security logic in ifs that requires a big comments and quite a bit of think to understand how it works.

Just fixed point 1. Back to RTBC, because it's just a nit. Passed locally.

+++ b/core/modules/editor/editor.module
@@ -467,6 +469,60 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
+  // Temporary files are handled by file_file_download(), so nothing to do here
+  // about them.
+  // @see file_file_download()
...
+  // Stop processing if there are no references in order to avoid returning
+  // headers for files controlled by other modules. Make an exception for
+  // temporary files where the host entity has not yet been saved (for example,
+  // an image preview on a node creation form) in which case, allow download by
+  // the file's owner.
+  if (empty($usage_list['editor']) && ($file->isPermanent() || $file->getOwnerId() != \Drupal::currentUser()->id())) {
+    return;
+  }

The first comment seems to be in direct opposition to the later code. If there's nothing to do here about them why do we have... ($file->isPermanent() || $file->getOwnerId() != \Drupal::currentUser()->id()) - if file_file_download() handles this - is this necessary?

The difference is that the temporary file handling (the first section) is not module-specific. The second section (the "stop processing" bit) is module-specific.

  // Temporary files are handled by file_file_download(), so nothing to do here
  // about them.
  // @see file_file_download()

refers to:

  // Find out if a temporary file is still used in the system.
  if ($file->isTemporary()) {
    $usage = \Drupal::service('file.usage')->listUsage($file);
    if (empty($usage) && $file->getOwnerId() != \Drupal::currentUser()->id()) {
      // Deny access to temporary files without usage that are not owned by the
      // same user. This prevents the security issue that a private file that
      // was protected by field permissions becomes available after its usage
      // was removed and before it is actually deleted from the file system.
      // Modules that depend on this behavior should make the file permanent
      // instead.
      return -1;
    }
  }

Which as you can tell does not list anything module-specific.

The second section however does module-specific things: file.module's file_file_download() calls file_get_file_references(), which does:

$file_usage_list = isset($usage_list['file']) ? $usage_list['file'] : array();

Note the key 'file', this corresponds to file module. This means this is only granting download access to files owned by the file module.

The corresponding second section in editor module's editor_file_download() checks not the 'file' key, but the 'editor' key: it grants download access to files owned by the editor module.

  if (empty($usage_list['editor']) && ($file->getOwnerId() != \Drupal::currentUser()->id())) {
    return;
  }

Committed and pushed f2ed11a to 8.3.x and 6e46381 to 8.2.x and 46664f6 to 8.1.x. Thanks!

Fixes on commit:

diff --git a/core/modules/editor/editor.module b/core/modules/editor/editor.module
index fe19bd1..2ecdb8a 100644
--- a/core/modules/editor/editor.module
+++ b/core/modules/editor/editor.module
@@ -16,8 +16,6 @@
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\filter\FilterFormatInterface;
 use Drupal\filter\Plugin\FilterInterface;
-use Drupal\Core\Entity\EntityStorageInterface;
-use Drupal\file\FileInterface;
 
 /**
  * Implements hook_help().
diff --git a/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
index 3fe05ee..b5ff6db 100644
--- a/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
+++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\editor\Tests;
 
 use Drupal\file\Entity\File;
-use Drupal\file\FileInterface;
 use Drupal\simpletest\WebTestBase;
 
 /**
@@ -42,7 +41,7 @@ function testEditorPrivateFileReferenceFilter() {
     // Create a file in the 'private:// ' stream.
     $filename = 'test.png';
     $src = '/system/files/' . $filename;
-    /** @var FileInterface $file */
+
     $file = File::create([
       'uri' => 'private://' . $filename,
       'status' => FILE_STATUS_PERMANENT,

I've reverted this change because after commit I suddenly got worried that this does not implement the correct security behaviour. We if we have a private file referenced in a node should we grant people access to it?

+++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
@@ -0,0 +1,86 @@
+    // Resave the file to be permanent.
+    $file->setPermanent();
+    $file->save();
+

If this is the correct solution. We should test that the anonymous user still does not have access after doing this.

Also this grants access to the file regardless of whether the user has access to the particular entity afaics.

#67:

+++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
@@ -0,0 +1,86 @@
+    // Resave the file to be permanent.
+    $file->setPermanent();
+    $file->save();
+

If this is the correct solution. We should test that the anonymous user still does not have access after doing this.

That code is simulating a call to \Drupal\file\FileUsage\FileUsageBase::add(), which is called by _editor_record_file_usage(), which in turn is called by either editor_entity_insert() (when the image is inserted when the entity is first created) or by editor_entity_update() (when the image is inserted when the entity is updated).

So did you quote the wrong code?

In any case, you're right that we need that test.

#68:

Also this grants access to the file regardless of whether the user has access to the particular entity afaics.

Ugh. You're totally right. And that's what the addition in #7 proved.

This is the problem:

  // Editor.module MUST NOT call $file->access() here (like file_file_download()
  // does) as checking the 'download' access to a file entity would end up in
  // FileAccessControlHandler->checkAccess() and ->getFileReferences(), which
  // calls file_get_file_references(). This latter one would allow downloading
  // files only handled by the file.module, which is exactly not the case right
  // here.

This means we're not at all checking whether the user can access one of the editor-referenced entities before it grants access. I should've questioned it more; this patch has contained that since the very beginning. Sorry for missing that, Alex. The tests seemed right and passed, but thankfully you noticed just in time that this should be testing one more thing.

Also: this entire API really needs an overhaul in Drupal 9, because it's extremely brittle :/

This patch just expands the test coverage. This should fail.

Forgot the interdiff.

And here's the fix. It's 80% copied from \Drupal\file\FileAccessControlHandler::checkAccess(), 10% copied from file_file_download(). Sadly we can't reuse any of the existing code without enormous refactoring, so this feels simpler then. The logic is trivial.

Given that the logic is trivial and 90% copied, I'm going to self-RTBC, so Alex takes another look at it.

Committed 05d832e and pushed to 8.3.x. Thanks!

Leaving at patch to be ported for eventual cherry-pick to 8.2.x once 8.2.0 is released.

diff --git a/core/modules/editor/editor.module b/core/modules/editor/editor.module
index 6c34730..0101779 100644
--- a/core/modules/editor/editor.module
+++ b/core/modules/editor/editor.module
@@ -16,8 +16,6 @@
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\filter\FilterFormatInterface;
 use Drupal\filter\Plugin\FilterInterface;
-use Drupal\Core\Entity\EntityStorageInterface;
-use Drupal\file\FileInterface;
 
 /**
  * Implements hook_help().
diff --git a/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
index 84ef4e0..5f689b3 100644
--- a/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
+++ b/core/modules/editor/src/Tests/EditorPrivateFileReferenceFilterTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\editor\Tests;
 
 use Drupal\file\Entity\File;
-use Drupal\file\FileInterface;
 use Drupal\Tests\BrowserTestBase;
 use Drupal\user\Entity\Role;
 use Drupal\user\RoleInterface;
@@ -47,7 +46,7 @@ function testEditorPrivateFileReferenceFilter() {
     // Create a file in the 'private:// ' stream.
     $filename = 'test.png';
     $src = '/system/files/' . $filename;
-    /** @var FileInterface $file */
+    /** @var \Drupal\file\FileInterface $file */
     $file = File::create([
       'uri' => 'private://' . $filename,
     ]);

Unused uses fixed on commit.

Committed e4aec7c and pushed to 8.2.x. Thanks!

I've applied the patch form #71 to my Drupal 8.2.1 installation. It does'nt work for me, all inline images still get an access denied.
I think there is a problem with the editor_file_download($uri) hook.
The $usage_list contains an array like this: $references[$usage->module][$usage->type][$usage->id] = $usage->count;
The line
$referencing_entities = entity_load_multiple($entity_type, $entity_ids);
expects $entity_ids to be an array of id's, but it isn't, its an associative array like this: [id1=>usage_count1, id2=>usage_count2, ..., idn=>usage_countn,]
A simple way to fix this is to apply array_keys($entity_ids):
$referencing_entities = entity_load_multiple($entity_type, array_keys($entity_ids));
Applying this fix the patch works for me.

@mabuweb: Next time, please create a new issue. Nobody saw your comment because the issue was already closed. Somebody just reported the same bug, and included a patch to fix it: #2831392: Text Editor module uses file.usage service incorrectly.

@Wim Leers #80

Is your link 403 on purpose? I cannot access it - just like #81

@maxilein: That issue is a 403 because it has been unpublished. That issue revealed a security vulnerability, and so it was unpublished in accordance with the Drupal security policies. It was fixed just today with the release of Drupal 8.2.7. See https://www.drupal.org/SA-2017-001. So for now if you need that fix the easiest thing to do is to update to the latest version of Drupal. You could pull the fix out of the Git history of course, but it's policy not to unnecessarily publicize the exact code that contains vulnerabilities.

@quickscetch
Thank you for the clarification. Makes sense now!