I guess it's not a bug, since it happened after https://drupal.org/node/2124241, but I don't get it, at all.

I upload an image to a custom entity (not a node). Then I want to reuse that image in a node, via filefield sources. It won't let me, because "You do not have permission to use the selected file.", because no module creates headers (what???) for this public file that I just uploaded.

1. It's a public file. Why is there any access check?
2. I just upload this file. Do I seriously have to upload a copy?
3. Why does it work if I upload it to a node first?

I don't get the security flaw... The point of public files is that they're public, right? Anyone can see, download, use. And anyone can! Anyone can download the image and reupload it!

I've tried to follow the file access trail, but it's insane. Filefield sources > Image > File > System? Somewhere a hard -1 is returned, so there's no way to publish my PUBLIC file.

What's going on?

For now I've added this to the top of filefield_sources_file_access():

  if (file_uri_scheme($uri) == 'public') {
    return TRUE;
  }

Why is that bad?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DrColossos’s picture

Category: Support request » Bug report
Priority: Normal » Major

We ran into the very same issue and got stock with a similar resolution. We couldn't deploy the 1.9 version since it was not possible to upload files anymore due to the "fix" from security.

I saw that we always got the mentioned error message because the new code decides to block the upload from the file.

I consider this a bug since there is no mention of a setting or allike that needs to be changed in order to make the new patch work

leewillis77’s picture

We're hitting this as well. In our case, the issue is that the core "image" module returns -1 since the URI (in our case) doesn't start with "styles/".

I'm not sure if whether this is a bug on the permissions checking in the filefield_sources module, or image?

Alan D.’s picture

Version: 7.x-1.9 » 7.x-1.x-dev
Component: General » Source: Auto-complete
Status: Active » Needs review
FileSize
765 bytes

Appears to be limited to Taxonomy terms. Users, comments, nodes all appear ok.

Steps to replicate.

  1. Add an image field to a term
  2. Create a new term and upload an image
  3. Create an image field on a node and enable FileField Sources reference existing on this
  4. Attempt to upload the term image

So... using the node example; hook_file_download_access() checks the entity permission of the file and grants permission if the user can view the node. aka access is now coupled to both the file AND the entity it is attached to.

Terms are not covered by core; a hurried oversight I guess from the security issues and there **should** be a corresponding core issue about this...

Anyway, rather than waiting a hundred years for core to fix this, simple workaround for terms, use user_access('access content'), the permission used for taxonomy/term/% menu callback.

@rudiedirkx
Clone this function in your module (changing the entity type of course) to provide file access.

Alan D.’s picture

Actually, probably worth some doco about this in the readme?

Alan D.’s picture

Title: File access check seems all wrong » Missing file fields entity grants for terms and custom entities.
rudiedirkx’s picture

The host entity is irrelevant. A public file is public. Everyone can access it. It's not FFS's job to check access by any method, certainly not by using download access. It's definitely not core's bug, it's FFS's. That's why this is new since the ridiculous 'security issue'.

My access hack works fine in all cases, since all public files are always all public.

What if you've uploaded a public file via File Entity, it's not attached to any entity, it's permanent and it's not used anywhere? No access to use it in a node? You have to reupload the same file? That can't be right.

rudiedirkx’s picture

Title: Missing file fields entity grants for terms and custom entities. » File access check seems all wrong

So that's why the title is that title.

Alan D.’s picture

@rudiedirkx
You are going against the flow that core has gone, so imho, this is a battle that you will not win convincing module maintainers to reverse cores security team decision.

So I would consider this a Feature request for a bypass permission to allow users to skip access checks is what you are after?

public files are always all public

If the URL is known, yes, but the security decision is to block access to prevent the URL being discovered, so in reality no.

public file via File Entity

There should be a corresponding record in the managed_file table and it should be handled or there is an issue in the corresponding modules.

Workarounds, one for File entities and a couple for FTP uploads:

I actually resolved this via a custom module for Terms and could easily allow any file entity based files as simple as adding

function MYMODULE_file_download_access($field, $entity_type, $entity) {
  // File fields attached to taxonomy items are not granted this permission.
  if ($entity_type == 'taxonomy_term') {
    return user_access('access content');
  }
  if ($entity_type == 'file') {
    // You have the field to check the schema if required, or just grant all.
    // File entity module now has a complete access api to hook into if you required this.
    // And would probably remove the need to do this...
    return TRUE;
  }
}

If there are zero references (i.e. FTP), then file_file_download() does nothing, you need to return some headers somewhere to initiate this yourself. A bit painful, but something like this:

function MYMODULE_file_download($uri) {
  // Only targets public files.
  if (file_uri_scheme($uri) != 'public') {
    return;
  }
  // Get the file record based on the URI. If not in the database just return.
  $files = file_load_multiple(array(), array('uri' => $uri));
  if (count($files)) {
    foreach ($files as $item) {
      // Since some database servers sometimes use a case-insensitive comparison
      // by default, double check that the filename is an exact match.
      if ($item->uri === $uri) {
        $file = $item;
        break;
      }
    }
  }
  // Leave it to the file module to handle referenced files.
  if (isset($file) && file_get_file_references($file)) {
    return;
  }

  // Not within Drupals file_managed table, make a fake record.
  if (!isset($file)) {
    $file = (object) array(
      'filename' => 'xxx',
      'filemime' => 'xxx',
      'filesize' => 'xxx',
    );
  }
  // Return the headers via file_get_content_headers().
  $headers = file_get_content_headers($file);
  return $headers;
}
leewillis77’s picture

Hi Alan,

This issue certainly isn't limited to taxonomy terms - our use case is nodes only. Implementing hook_file_download_access in a module also won't help since (As mentioned in my earlier comment), the image module is specifically blocking access, so even if our custom module provided headers, they would be trumped by "Image" returning -1.

Alan D.’s picture

Pain will come from either of these two source roots:

FileField Sources module - filefield_sources_file_access()
includes/file.inc - file_download()

Both correctly triggers hook_file_download()

Image module - image_file_download() only potentially blocks styles\...
Image module - image_file_download() invokes file_file_download()
File module - file_file_download() will only block a download if file references exist and access to the entity is blocked....

afaick, nothing permits a ftp'ed file by the simple fact that nothing is registered to do this and nothing returns any http headers to permit the download:

function file_download() {
....
    if (count($headers)) {
      file_transfer($uri, $headers);
    }
    drupal_access_denied();
}

Or

function filefield_sources_file_access() {
  ...
  return !empty($headers);
}

Placing a different spin on things:

Would you expect access to the body field on an unpublished node? Same diff, the file is a field on an entity that has no access (or there is another bug that I haven't seen clear steps to replicate)

In saying that, the grants get on permissions is permissive, simply implement hook_file_download_access() to bypass other entity based restrictions, any positive result will allow the reference.

However, I do see this as a potentially valid feature request ;)

Integrating with File entity modules permission 'bypass file access':

    'bypass file access' => array(
      'title' => t('Bypass file access control'),
      'description' => t('View, edit and delete all files regardless of permission restrictions.'),
      'restrict access' => TRUE,
    ),

simple as:

-        if (!filefield_sources_file_access($file->uri)) {
+        $access = filefield_sources_file_access($file->uri) || user_access('bypass file access');
+        if (!$access) {
           form_error($element, t('You do not have permission to use the selected file.'));
         }

Or a bit more complex if you need to support individual stream wrappers.

Anyways, nice learning experience on the new file handling in core.

I personally think that
a) handling terms is a works as designed / duplicate of core issue now.
b) ftp'ed files, possible feature request in this queue [redundant to this discussion anyway, as you can not reference these from the widget, but if you were writing your own...]
c) integration with the File entity module, a task for either module. Patch attached is a clone of the code snippet above. The File Entity file access api still needs some love, thus I skipped integrating properly.

leewillis77’s picture

Image module - image_file_download() only potentially blocks styles\...

This is the exact problem we're hitting.

From image.module (Comments mine)

if (strpos($path, 'styles/') === 0) {
  // Do other checking and return if file can be accessed
}
return -1; // This causes the error for us.

Specifically - in our case, the URI being checked doesn't start with "styles/"

Alan D.’s picture

Looks like your version is behind core:

http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/im...

But that will not help as file_file_download() will still block it unless you use a workaround.

Without hacking contrib modules or core, this should bypass the restrictions.

Tested using unhacked Drupal 7.26, File Field Sources 7.x-1.9. Remember to flush your cache after adding this.

/**
 * Implements hook_file_download_access().
 */
function MODULENAME_file_download_access($field, $entity_type, $entity) {
  // Taxonomy term entities have no implemention of hook_file_download_access().
  // See https://drupal.org/node/1327224
  if ($entity_type == 'taxonomy_term') {
     return user_access('access content');
  }
  // Allows access to any referenced file that uses the public schema.
  if (!empty($field['uri']) && ($schema = file_uri_scheme($field['uri']))) {
    if ($schema == 'public') {
      return TRUE;
    }
  }
}
GiorgosK’s picture

I am affected too
perhaps one should look at how they found similar solution
https://drupal.org/node/2149407

rudiedirkx’s picture

That was exactly my 'solution' hack. Public files are public.

grossmann’s picture

Status: Needs review » Reviewed & tested by the community

#11 fixes my issue with term files.
I am not sure if there are some deeper changes discussed as it seems there are different paradigms.

But for now this fixes this and this issue is now 8 months old so I set this to reviewed and tested.

MatthijsB’s picture

Component: Source: Auto-complete » General
Status: Reviewed & tested by the community » Needs work

Actually, the fix in #11 only solves term issues. But what about this use-case:
- say you have a Content Type with a File Field, using private storage and with the IMCE browser enabled
- create a new node of this type
- Use the Open File Browser link next to the File Field, to start IMCE
- Browse to the desired folder within the private file storage
- Upload a file, using the Upload button inside IMCE
- Select the newly uploaded file and click Select in IMCE.
- IMCE inserts the filename into the File Field and closes.
--> The same error "You do not have permission to use the selected file." appears, and the File Field is empty again.
This even happens if you perform all these steps as superuser.

Using IMCE is not the only use-case where permissions are still broken. As mentioned in #11, any file which is uploaded via FTP will also receive the same treatment: the file is not (yet) owned by any Drupal file download handler, so the same error message will appear and access will be disallowed.

And more about User 1 (or any other user with the special 'administrator' role)? Shouldn't he also be allowed to select any file into a File Field, whether it is a public or a private file?

Concluding, the following items are not fixed with #11, and are all caused by https://www.drupal.org/node/2124241:
- IMCE upload new files
- FTP upload of new files
- User-1 trying to access private files

rudiedirkx’s picture

@MatthijsB Drupal's 'logic' is this: you (anyone, including admins) can only access and use files that are attached to an accessible entity. For instance, files attached to files won't EVER work, because the file module doesn't have decent view access checks for itself. Only files attached to nodes are accessible decently, because nodes have a decent access check. I think it's a ridiculous system of checking access: every indivdually uploaded file is useless, but that's how it works in D7 =(

MatthijsB’s picture

After some further investigation, it appears that a file which is uploaded via IMCE is registered in Drupal's database after all.
However, this issue leads to the 'access denied' (as mentioned above) for every private file (even the ones which you own, and have uploaded yourself).

This comment proposes a working workaround (if combined with the remarks from the comment below, about the missing global $user.

The other two issues (FTP / User-1 + private files) are not influenced by this workaround, and will remain unsolved.

Leeteq’s picture

This is not as simple as "public files are public":

When choosing the "private files" option, one is not only changing to "access control" the file, but changing the very method by which the files can be accessed and delivered. The "Private files" method limits the files to be served directly "by Drupal", and are not possible to access through a (file-specific, direct) URL, regardless of if the user in question has access to it or not.

There are many situations and types of sites (configurations) where we want to have BOTH access control AND the possibility to use direct URLs to files, still limited to what the access control permits. Therefore, not all sites give "public" access to files just because the URL method is available, so that logic should not be used as the basis for this solution. When the Core developers added that security check, it was this scenario that they also addressed, and that is the "direction" of core, which a solution to this problem also should adhere to.

rudiedirkx’s picture

They intended separately uploaded files (not in a node) to be useless? With File Entity you can upload files, but never use them. That seems wrong.

What should I as developer do to make my public files public? I, the developer, must have a way without hacking core or FFS. What is it? I want some authenticated users to upload files, and other users to use them.

GiorgosK’s picture

patch from #11 does not apply cleanly
some lines have changes but once you apply by hand it works

GiorgosK’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.49 KB

#11 does not apply cleanly

Rearrnaged the patch and this applies fine to the current version (7.x-1.10)
RTBC because this patch does not add anything more
and its been tested already by a few people

grossmann’s picture

+1 for RTBC

quicksketch’s picture

Thanks everyone for their input here. #23 is great but I think the comments about allowing all public files is also valid. Let's do both and allow access to all public files all the time, and allow access to all private files if the user has the "bypass file access" permission from File Entity module.

Here's a patch that does the combined approach. I've committed it to the 7.x branch and it will be in 7.x-1.11.

  • quicksketch committed be33c76 on 7.x-1.x
    Issue #2176133 by Alan D., GiorgosK, quicksketch: Allow public files to...
quicksketch’s picture

Title: File access check seems all wrong » Allow all public files to be reused and/or skip access check if File Entity permission is granted
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.