Private images broken [#898036]

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
#14
#12	898036-12-private-images-broken.patch	6.32 KB	ksenzee
#12
#8	fix_private_images2_quick_reroll.patch	5.87 KB	aspilicious
#8
#7	fix_private_images2.patch	5.48 KB	franz
#7
#3	fix_private_images2.patch	5.49 KB	Berdir
#3
	fix_private_images.patch	1.17 KB	Berdir

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 August 2010 at 02:42

Issue tags:

+Needs tests

Tagging. Bleh.

Comment #2

BenK CreditAttribution: BenK commented 31 August 2010 at 03:30

Keeping track of this thread....

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir commented 31 August 2010 at 13:05

File	Size
fix_private_images2.patch	5.49 KB

Uff. 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

Berdir

German

Switzerland

CreditAttribution: Berdir commented 31 August 2010 at 21:11

Issue tags:

-Needs tests

Has tests now :)

Comment #5

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 1 September 2010 at 09:29

+++ includes/file.inc
@@ -1800,9 +1800,19 @@ function file_download() {
+      if (isset($result) && is_array($result)) {
+        $headers = array_merge($headers, (array)$result);

if $result is an array, why do we need to typecast?

Powered by Dreditor.

Comment #6

Berdir

German

Switzerland

CreditAttribution: Berdir commented 1 September 2010 at 11:03

Status:

Needs review

» Needs work

Ups. 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

franz

any

Montréal

CreditAttribution: franz commented 1 September 2010 at 12:11

Status:

Needs work

» Needs review

File	Size
fix_private_images2.patch	5.48 KB

done =]

Comment #8

aspilicious CreditAttribution: aspilicious commented 1 September 2010 at 12:12

Status:

Needs review

» Needs work

File	Size
fix_private_images2_quick_reroll.patch	5.87 KB

Super-easy? Sounds like a job for aspilicious...
Putting his laser eyes on the cast ==> cast is gone!

Comment #9

aspilicious CreditAttribution: aspilicious commented 1 September 2010 at 12:14

Status:

Needs work

» Needs review

Comment #10

aspilicious CreditAttribution: aspilicious commented 1 September 2010 at 12:15

AAAAAH superhero franz destroyed it first....

Comment #11

franz

any

Montréal

CreditAttribution: franz commented 1 September 2010 at 12:23

=] There's nothing like super-easy-super-heros...

Comment #12

ksenzee

she/her

Seattle area

CreditAttribution: ksenzee commented 1 September 2010 at 19:20

File	Size
898036-12-private-images-broken.patch	6.32 KB

This 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

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 2 September 2010 at 02:37

Status:

Needs review

» Reviewed & tested by the community

+++ modules/image/image.test
@@ -621,12 +621,27 @@ class ImageFieldDisplayTestCase extends ImageFieldTestCase {
+    // Remove access content permission from anon users.

Really 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

ksenzee

she/her

Seattle area

CreditAttribution: ksenzee commented 2 September 2010 at 13:47

File	Size
898036-14-private-images-broken.patch	6.33 KB

Reroll changing "anon" to "anonymous" - no other changes.

Comment #15

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 11 September 2010 at 05:06

Status:

Reviewed & tested by the community

» Fixed

Sorry, 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 commented 17 September 2010 at 03:11

I believe this also fixes #181036: cannot download from https using IE.

Comment #17

1 October 2010 at 03:20

Status:

Fixed

» Closed (fixed)

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

Comment #18

Bevan CreditAttribution: Bevan commented 30 January 2012 at 03:16

This 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