Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When you enable private file downloads, you always get 'Access Denied'. This is becuse of the logic in file.inc. This patch fixes that.
Comment | File | Size | Author |
---|---|---|---|
#11 | file_2_0.patch | 717 bytes | Souvent22 |
#8 | file.inc_1.patch | 796 bytes | zeltins |
file_2.patch | 717 bytes | Souvent22 | |
Comments
Comment #1
m3avrck CreditAttribution: m3avrck commentedCode looks good to me and does indeed fix 'private' downloads.
Comment #2
Chris Johnson CreditAttribution: Chris Johnson commentedmodule_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:
Prints: NULL
Comment #3
Souvent22 CreditAttribution: Souvent22 commentedChris,
I went off the docs:
http://drupaldocs.org/api/head/function/hook_file_download
Which states:
Although module invoke should not return -1, then how does a node restrict access upon intercepting the hook?
Example:
node 'X' which the user doen'st have access to; returns -1.
drupal_access_denied is returned.
Your last argument:
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.
Comment #4
Souvent22 CreditAttribution: Souvent22 commentedJust 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
Comment #5
Chris Johnson CreditAttribution: Chris Johnson commentedIt 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.
Comment #6
zeltins CreditAttribution: zeltins commentedI 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:
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.
Comment #7
Souvent22 CreditAttribution: Souvent22 commentedZ,
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.
Comment #8
zeltins CreditAttribution: zeltins commentedI 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:
hook_file_download
, we returndrupal_access_denied()
and exit.module_list()
to return an array fromhook_file_download
causes us to callfile_transfer
with those headers. Note thatfile_transfer
callsexit()
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.
Comment #9
Dries CreditAttribution: Dries commentedSouvent22, you get to decide what happens with this. Please follow up and test.
Comment #10
Souvent22 CreditAttribution: Souvent22 commentedZ,
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
Comment #11
Souvent22 CreditAttribution: Souvent22 commentedAttached 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.
Comment #12
Dries CreditAttribution: Dries commentedThanks. Committed. Looking forward to future improvements. If anything, file.inc needs a lot of love!
Comment #13
(not verified) CreditAttribution: commented