Problem/Motivation

It appears that hook_file_download is being called during the file upload process, starting with the 7.29 update. The hook_file_download documentation states that the purpose of hook_file_download is to "Control access to private file downloads and specify HTTP headers". It is never mentioned that it could be called during file uploads.

This became an issue for me because I use a contrib module that allows me to require a user to fill out a form before being able to download a private file (https://www.drupal.org/project/webform_file_gateway). When I upgraded to 7.29, the module began inserting the webform during the file upload process.

It seems to me that contrib modules have the expectation that hook_file_download would only be called when a private file download was attempted, and not during other file processing. It's entirely possible that other contrib modules may experience issues.

Steps to reproduce:
1. Go to Content -> File
2. Click "Add File"
3. Click "Choose File". Select a file to upload. Click "upload"
4. Click "Next" <-- this is where the problematic call to hook_file_download occurs.

Specifically, file_managed_file_value() in form.module:

if (isset($input['fid']) && ($file = file_load($input['fid'])) && file_download_access($file->uri)) {
...

The proposed patch in https://www.drupal.org/node/2305017 changes this line of code slightly, but does not appear to change this behavior.

Proposed resolution

I'm not sure what to suggest. I know this change was done to fix a security issue, but perhaps there is a different way to solve it? Or if not, the documentation for hook_file_download should be updated. Is there a way to alert maintainers of contrib modules that implement hook_file_download that their code needs to adjust to its new usage?

Remaining tasks

TBD

User interface changes

TBD

API changes

TBD

Comments

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Component: file.module » documentation

I agree this should be documented. I'll also add something about it to the 7.29 release notes.

I don't know if there's much else that can be done about it though. Checking hook_file_download() is the only way to determine if a file is allowed to be downloaded, and that's what the core security fix needed to do. Contrib modules (including the Entity API module which is used on half of all Drupal 7 sites) already used it this way before... for example, with the Entity API module and Webform Private File Gateway modules enabled on a Drupal 7.28 site, try this code (assuming the file with ID #1 already exists on the site and that the module is configured to require a webform submission before seeing it):

if (entity_access('view', 'file', file_load(1))) {
  // Do something.
}
else {
  // Do something else.
}

Neither "Do something" will ever happen, since Webform Private File Gateway will redirect to the webform and end the request.

So I think the best solution is for Webform Private File Gateway to change its code to play a bit nicer with other modules... I'll follow up at #2308359: Usage of hook_file_download causes issues with 7.29.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: hook_file_download usage has changed » Document that hook_file_download() can be invoked during content edit file uploading
Version: 7.29 » 7.x-dev

So... I guess what happened in 7.x is that during the processing of an uploaded file from the File module's file field, a new check was added to make sure that the current user has access to see the file. That is what is referenced in this issue report in file_managed_file_value(). and it's part of the security alert at: https://www.drupal.org/SA-CORE-2014-003

The check to make sure the user has access to the file necessarily must call hook_file_download(), because that is how access checking for file download works.

So it seems that what we need to do is add a note to hook_file_download to mention that this check could be run during content editing. The example would be editing content with an Image field: if an Ajax-powered "Upload" button is used to upload the file, after uploading a thumbnail is normally displayed, which is a download operation, and access will need to be checked for that download.

Right?

So do we need to do this in 8.x as well? I don't see a similar line added to that function in 8.x, but presumably in the editing form, a similar access check must be done when the thumbnail is displayed, right?