There are actually two different problems, both can easily be fixed.
- Files that are served in the original size are broken because file_file_download() does return the headers twice, this causes invalid HTTP Headers like this: "Content-Disposition: Array". Fix is easy, just needs an additional empty($references) check.
- The other problem is that image styles don't respect private file access permissions and are always displayed, because there is a bug in image_file_download(). That function does not remove the scheme that is added in front of the path. Therefore, it generates an invalid uri (private://private/some-file.jpg), file.module/image.module don't feel responsible for that file and don't deny access. This patch does fix that by calling array_shift($args) again. While discussing this, we also came up with a way that will make the whole generation thing work for other file schemes than public/private, DamZ is going to create a separate issue about that.
- Another related issue is that I was wondering if file_download() should manually call the hook implementations and use array_merge() to avoid multi-dimensional arrays. That would also allow modules to override headers defined by other modules. This would avoid bugs like this between contrib modules and is another reason why array_merge_recursive() has it all wrong ;)
- It is also quite ugly that file_file_download() is used as an API function and a hook implementation and even uses an additional argument for that. I planned to clean that up but I'm not sure anymore because that seems to be a common pattern between image.module and file.module.
I guess the existing tests should be improved to cover these two cases. But it's to late (4am) for writing tests now :)
Comment | File | Size | Author |
---|---|---|---|
#14 | 898036-14-private-images-broken.patch | 6.33 KB | ksenzee |
#12 | 898036-12-private-images-broken.patch | 6.32 KB | ksenzee |
#8 | fix_private_images2_quick_reroll.patch | 5.87 KB | aspilicious |
#7 | fix_private_images2.patch | 5.48 KB | franz |
#3 | fix_private_images2.patch | 5.49 KB | Berdir |
Comments
Comment #1
webchickTagging. Bleh.
Comment #2
BenK CreditAttribution: BenK commentedKeeping track of this thread....
Comment #3
BerdirUff. Doesn't even require that much code, but getting the tests working was quite a challenge :)
New patch contains:
- Refactored the existing formatter tests a bit to test them both with public and private scheme
- Tests that access is denied without access content permission on nodes for both original file and image styles
- Tests that the headers are sent correctly in private mode. I actually had to do that because when accessing with simpletest, the image was still sent even if the headers were messed up
- Also implemented the suggested change in file_download(). That should avoid similiar errors with other modules and make the solution a bit more flexible.
I verified that the tests fail without the changes...
Comment #4
BerdirHas tests now :)
Comment #5
tstoecklerif $result is an array, why do we need to typecast?
Powered by Dreditor.
Comment #6
BerdirUps. I first added the type cast and then re-used the is_array() check from module_invoke_all(). I won't have time for a re-roll, anyone up for it? This is super-easy :)
Comment #7
franzdone =]
Comment #8
aspilicious CreditAttribution: aspilicious commentedSuper-easy? Sounds like a job for aspilicious...
Putting his laser eyes on the cast ==> cast is gone!
Comment #9
aspilicious CreditAttribution: aspilicious commentedComment #10
aspilicious CreditAttribution: aspilicious commentedAAAAAH superhero franz destroyed it first....
Comment #11
franz=] There's nothing like super-easy-super-heros...
Comment #12
ksenzeeThis looks good to me. I made a few changes to comments, and one change to a test assertion message, but no substantive changes. I'll try to get another review instead of marking it RTBC myself.
Comment #13
tstoecklerReally tiny nitpick: Can we not abbreviate 'anon', please?
Still marking RTBC. I had already reviewed the code changes and now I reviewed the tests. I must say that I only looked at the patch and not the surrounding code, but the tests are sound and they pass and Berdir did a pretty good job of explaining the changes.
The nitpick can be fixed in a small follow-up or prior to commit. Or simply not, it's not that big a deal.
Powered by Dreditor.
Comment #14
ksenzeeReroll changing "anon" to "anonymous" - no other changes.
Comment #15
webchickSorry, this has been on my radar all week but just got a chance to get back to it now.
Looks good. Really happy to see expanded test coverage for this. Private files have broken numerous times this release in horrible and frightening ways. Hopefully we're at the end of that now.
Committed to HEAD.
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson commentedI believe this also fixes #181036: cannot download from https using IE.
Comment #18
Bevan CreditAttribution: Bevan commentedThis issue removes code introduced in this patch, since that code introduces other bugs and the bug originally reported here is not reproducible when said code is removed:
#1414990: Orphaned private files can not be accessed