When you enable private file downloads, you always get 'Access Denied'. This is becuse of the logic in file.inc. This patch fixes that.

CommentFileSizeAuthor
#11 file_2_0.patch717 bytesSouvent22
#8 file.inc_1.patch796 byteszeltins
file_2.patch717 bytesSouvent22
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m3avrck’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good to me and does indeed fix 'private' downloads.

Chris Johnson’s picture

Status: Reviewed & tested by the community » Needs work

module_invoke() should never return -1.*

module_invoke(..., 'file_download', ...) returns an array when there is a valid download, or NULL when the called hook_file_download() does not have an explicit return.

This patch makes it look like an empty array is returned for private downloads. Is that true?

It also looks like the check for -1 will never be true, because that should not be a possible value. So permission will never be denied for that reason. Is this true?

*This is because module_invoke() returns whatever value is returned by the called hook. For example:

function hook_file_download($file) {
  // do some stuff
}
$foo = hook_file_download('baz.txt');
var_dump($foo);

Prints: NULL

Souvent22’s picture

Chris,

I went off the docs:

http://drupaldocs.org/api/head/function/hook_file_download

Which states:

If the user does not have permission to access the file, return -1. If the user has permission, return an array with the appropriate headers.

Although module invoke should not return -1, then how does a node restrict access upon intercepting the hook?

Example:

  1. module_invoke fires. It has to run through 8 modules.
  2. modules 1-4 do not implement the hook, therefor, empty (nothing, not an empty array) is returned.
  3. module 5 implements the hook, and runs checks the file. Sees that this file is tied to
    node 'X' which the user doen'st have access to; returns -1.
  4. drupal_access_denied is returned.

Your last argument:

*This is because module_invoke() returns whatever value is returned by the called hook.

Is true, however, this module invocation is restricted to 'file_download' hook, thus we know the return value according to the documentation of the hook.

Let me know if this answers your questions.

Souvent22’s picture

Just to clarify:
It's not module_invoke returning -1...it's the hook returning -1, and module_invoke returns whatever hook it's firing/running.

http://drupaldocs.org/api/head/function/module_invoke

Chris Johnson’s picture

Status: Needs work » Reviewed & tested by the community

It looks like you are right. Whoever designed the hook_file_download API apparently picked -1 as the way to indicate restrict access, even though that's not a very clean interface.

Sorry for the confusion. I had checked all of core and found no modules that returned -1 and so confused the return value when trying to optimizie the usage of is_array() throughout core.

I think the patch is correct as is, and that I was mistaken. Setting the status to ready to be committed.

zeltins’s picture

I don't think this patch is quite right (although it does work most of the time). Before discussing the correct solution, I think we need to agree on the desired behavior. Should a single module be able to deny access to a file, causing the system to ignore all other modules? I would think to be consistent with the rest of the drupal access control system, the answer is no. The system should assume access denied and any module can override that to allow. Our case here is a little more complicated since the module that gets to handle the file will be the first one in the modules array that returns an array result, and this may or may not be the desired behavior, but changing this would probably be more trouble than it is worth.

So, assuming that returning -1 is not necessarily an instruction to deny access, but more of a no op where the default behavior is to deny access, the following code might be a better solution:

Index: C:/projects/Personal/websites/nicholaszsmith.com/html/main/includes/file.inc
===================================================================
--- C:/projects/Personal/websites/nicholaszsmith.com/html/main/includes/file.inc	(revision 30)
+++ C:/projects/Personal/websites/nicholaszsmith.com/html/main/includes/file.inc	(working copy)
@@ -488,13 +488,14 @@
     $list = module_list();
     foreach ($list as $module) {
       $headers = module_invoke($module, 'file_download', $file);
-      if (empty($headers)) {
-        drupal_access_denied();
-      }
-      elseif (is_array($headers)) {
+      if (is_array($headers)) {
+        // Note that file_transfer calls exit(), so we do not have to stop the loop.
         file_transfer($file, $headers);
       }
     }
+    // All modules have had a chance to handle the file, but none have.
+    drupal_access_denied();
+    exit();
   }
   drupal_not_found();
 }

One issue with the first patch is that drupal_access_denied() does not call exit(), so if a module in the array returned -1, and then another module tried to allow access, an error occured. Even if it did call exit(), the system would behave in a way contrary to my above assumption. As a side question, does anyone know why drupal_access_denied() doesn't call exit() at the end? Just curious. I hope this has been helpful.

Souvent22’s picture

Status: Reviewed & tested by the community » Needs review

Z,

I see your point. This also brings up another issue:

Who do who believe?

Meaning, the hook says it returns an array of headers to be sent.....
but, we can't send multiple headers at will.

Exmaple: I make a module that checks to see if the user is using a mozilla or an IE browser if it sees the MIME is application/pdf...if it's Moz., the MIME application/octect-stream needs to be sent instead.
So, if upload.module has already returned an array of headers, and i return an array of headers....that will conflict, it isn't pretty, and isn't standard.

I'm just going to use this patch internally for while cause I need it...however, I think I shall set this to review to get some more comments and discussion on it.

zeltins’s picture

FileSize
796 bytes

I agree, it is not clear how the function should behave when multiple modules want to return headers for the same file. As a result, I think this should be left to a future version of Drupal, and we should concentrate on getting private downloads working for 4.7. I have attached my patch, and it behaves as follows:

  • If no module returns an array from hook_file_download, we return drupal_access_denied() and exit.
  • The first module in the list returned by module_list() to return an array from hook_file_download causes us to call file_transfer with those headers. Note that file_transfer calls exit() at the end of the method, so this is also an exit point.

I think this should be safe to apply and will fix the issue referenced by this bug.

We can either leave this bug open or add a new feature request to 'fix' the download security to be divorced from the generation of headers for a file. I don't really have a good plan for handling the headers (almost no matter what, it comes down to an arbitrary ordering of modules resulting in a first or last wins situation for each header), but I will keep thinking about it. Let me know what you think.

Dries’s picture

Souvent22, you get to decide what happens with this. Please follow up and test.

Souvent22’s picture

Z,

I think there needs to be a fundemental change to the hook itself. I think the correct thing to do would be to bring file.inc in sync with the current hook, so that everyone is on the same page. Then discuss a change to the hook for the next release, or as an update. I agree with you patch to a certain degree, however it does deny access to other modules from even having a chance to view the file becuase of the random nature in which the array is filled and it's order in the cue. I think that with the first patch at least lets each module have a crack at the file. Further more, if no module returns an array or -1 (meaning no module handles it), then by default drupal_not_found is returned. Which in some ways is better than access denied, thus making any novice hacker or 'bot-hacker', look elsewhere thinking the file doesn't exist rather than just not being able to get to it.

I think me and you Z should start another thread and discuss a better alternative to this hook. My off the top thinking is that we should be passing around an array of headers and the file path so that each module can have a chance to manipulate the headers, view the currnet headers and the file, and be able to deny access by returning -1, and empty array, or some other arbitrary value.

- Souvent

Souvent22’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
717 bytes

Attached is the original patch, re-rolled from head that brings file.inc in sync with the hook hook_file_download as per drupaldocs.org ( http://www.drupaldocs.org/hook_file_download ). This fixes file.inc. It also allows for the base case that if no modules handle the hook, access_denied is returned/run.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks. Committed. Looking forward to future improvements. If anything, file.inc needs a lot of love!

Anonymous’s picture

Status: Fixed » Closed (fixed)