image_style_deliver can create invalid headers

It works for me.

The patch didn't apply automatically for Drupal 7.26 so I'll try to trigger re-test here, however after I applied it manually in the core image.module, the issue with 'array to string conversion' notices I had with File Entity is gone now. Thank you for the patch.

image_style_deliver_shouldnt_use_module_invoke_all-0.patch queued for re-testing.

I will rtbc again, I have this live on some sites, and it's fixing the issue.

This would need to go into Drupal 8 first.

It could also use a code comment explaining why it's done this way, and I'm wondering if there are other places in the codebase that invoke hook_file_download() that have the same problem?

The error that brought me here was:

Notice: Array to string conversion in drupal_send_headers() (line 1221 of /var/www/drupal-7.27/includes/bootstrap.inc).

For future googlers.

Patch #6 solved the problem

D7:

TLDR; this is an over explanation of the fact that I should have read the issue summary, it would have saved me time.

I had a private file being rendered as a download in a custom entity.
following hooks were called:

file_entity_file_download
image_file_download

Both returned the same array.

$result = [
	'Content-Type' => "image/jpeg",
	'Content-Length' => "15140",
	'Cache-Control ' => "private",
];

Within module_invoke_all($hook) we are doing a array_merge_recursive

$return = [
	'Content-Type' => "image/jpeg",
	'Content-Length' => "15140",
	'Cache-Control ' => "private",
];
$result = [
	'Content-Type' => "image/jpeg",
	'Content-Length' => "15140",
	'Cache-Control ' => "private",
];

$result = array_merge_recursive($result, $return);
var_dump($result);

Results:

array(3) {
  'Content-Type' =>
  array(2) {
    [0] =>
    string(10) "image/jpeg"
    [1] =>
    string(10) "image/jpeg"
  }
  'Content-Length' =>
  array(2) {
    [0] =>
    string(5) "15140"
    [1] =>
    string(5) "15140"
  }
  'Cache-Control ' =>
  array(2) {
    [0] =>
    string(7) "private"
    [1] =>
    string(7) "private"
  }
}

Ok all that being said, maybe a test is needed to show the problem.

Hi, this error is still happening in 7.8. Will the patch be added to core fairly soon, I'm not keen on patching a core module.

Hi,

I am having the same issue.

Patch fixed it for me.

When can we expect to see this commited?

Thanks,

I just noticed this was introduced with the latest core update to 7.31.

I can see from the doing a diff that the code this patch adds was in core but got removed?

I just noticed this was introduced with the latest core update to 7.31.

Also present in core 7.34.

Patch in #6 worked for me.

Same issue on 7.35.
Applied #6 and works great too.

Thanks!

I've added a patch for D7.36

Marking this Needs Review to get #16 tested in latest D7. But then I know somebody will just change it to 8.0.x again. But we need this patch in Open Atrium to fix the send_header errors we are seeing.

If we want it fixed in D7, it should at least be fixed in 7.x-dev.

OK, let's get this in. I really need this for a handful of sites.

Probably needs tests. So here is a test-only patch and patch with added test.

Oops. Made this in a repo where drupal was not root folder :)

Hm, I guess the testbot does not consider notices as errors in tests. Either that or it does not consider "Array to string conversion" as a notice (should be added in php 5.4, right?)

For the record, I checked if this bug exists in d8, it does not.

Not sure how to adjust the test to force it to fail on the testbot, through. image_style_deliver calls drupal_exit, so that does not work. Anyway, leaving as needs review for now.

This patch works for me in 7.38

If it works and you think the patch code looks ok, feel free to set the status to "Reviewed & tested by the community" to try to get this commited. :)

About the tests, I did a little digging around, and the only idea I found is this.

Maybe that would work for making tests fail, even if it's ugly.

Hm, really strange that this test-only patch is not failing. Tested it on different platforms too. Trying once more...

The test will not fail because notices do not trigger fails in tests. For a failing test, you need to intercept the notice, and trigger a fail. To intercept the notice, you can use a custom error handler, and in there you do a string match to see if the error message is "Array to string conversion". If the string matches, you trigger a fail, otherwise at the end of the test you trigger a pass.

You can find a link to an example in #29.

On my computer, across 3 versions of php and in both GUI and CLI versions of running the tests, and across 2 platforms (both on my Mac and in a virtual ubuntu machine) they are always being marked as failing, when there is a notice. According to someone on IRC, this should also be the case for the testbots.

I'll see if I can make it fail in another way though. And worst case, look at that link. Seems a little elaborate, though :)

How about this, then...

Hm, maybe try to log in the user to make sure it is not a caching issue. And also assert server response. This is so strange...

The tests in #34 failed because the login failed, $this->admin_user is not existing.

What if we throw an exception in drupal_add_http_header() if the $name is not a string, as the function assumes that the $name argument is a string? That would make the tests a lot easier to write, and would make possibly bad code in contrib easier to identify.

Yeah, i saw that, went a bit fast there. :)

So, I finally got this to not fail for me too. And throwing an exception would not fix it, because the test just never triggered the notice (hence it was not passing an array as a header value). Hopefully this will get the test failing.

Taking another stab in the dark here. Maybe the test image is getting cached somehow, since it re-uses the same file as another test.

OK, let's try something else. This will not be the actual test diff, of course, but this makes the test also fail for me. Let's hope the same is the case for the testbot

OK, so this is literally obligated to fail :)

Seriously? Just uploading this in pure spite, to test the testbot.

OK, then I guess all my effort has always been wasted :). The testbot does not recognise a notice at all, so I guess we have to go with an exception? This one is not really a test-only patch, and I am a bit uncertain if it is a good idea to add this (one could consider this a breaking change, in some weird way).

OK then, we allow numbers as well, I guess.

Ah, so I finally am pretty sure I have figured it out.

On php 5.3, the notice will not be generated, but the string will just become "Array". I was convinced these tests run on 5.4 (since that is what it says on the testbot project page), but I assume it has to be 5.3.

Attached is a patch that fails on both 5.3 and above. So we can finally go back to the simple solution and not throw an exception, probably. Less disruptive this way.

Well, thats a lot of back and forth.

Anyway, to summarize:

Last comment, when I thought I had made a test that would fail on php 5.3, I only switched CLI php version, so the web server was still running 5.4. So that was why it was failing locally for me.

So it seems it is impossible to get this to fail on php 5.3. So I'll just reupload the patch from #21. This test would have failed if the test was running on 5.4 >=.

image_style_deliver can end up calling drupal_add_header with an array, but then a couple of lines later, file_transfer also adds the content-type header, thus overriding the array with a string again. So no way to cleanly test this as far as I can see.

Anyway, this patch still fixes the issue (which appearently only is an issue on 5.4 >=), so please test and review.

+++ b/modules/image/image.module
@@ -833,7 +833,19 @@ function image_style_deliver($style, $scheme) {
+          // Throw away the headers received so far.

This comment should explain why the headers are discarded not that they ARE discarded.

+++ b/modules/image/image.module
@@ -833,7 +833,19 @@ function image_style_deliver($style, $scheme) {
+          $headers = array();
...
       if (in_array(-1, $headers) || empty($headers)) {
         return MENU_ACCESS_DENIED;

Does discarding the headers for a -1 result break the access denied later on? -1 will never appear in the array and thus MENU_ACCESS_DENIED will never be returned?

This comment should explain why the headers are discarded not that they ARE discarded.

This is a copy-paste from the file_dowload function, which has the exact same code. We throw away the headers, because someone has denied access, and thus no headers should be sent. It does not directly relate to throwing away headers to avoid duplicate headers.

Does discarding the headers for a -1 result break the access denied later on? -1 will never appear in the array and thus MENU_ACCESS_DENIED will never be returned?

No. When a -1 is encountered, the headers are "emptied" (as explained above), the loop is "stopped", and the next if will check for an empty array of headers, which will in turn return MENU_ACCESS_DENIED. Actually I think we can remove the "if (in_array(-1, $headers)" part, because headers will always be empty when there is a -1 result. I'll just try that here, to see if all tests still pass.

Can we RTBC this?

@mstrelan if you've reviewed and tested it, then sure you can.

The tests only patch in #47 really should be failing if it's testing the problem we are solving and this is really a bug in D7. So i'd keep it say this needs-work for that.

I'm re-posting those so we can get the new CI testbot to try it out with different versions of PHP.

This is not failing because the test is run on the old CI with php 5.3 where this does not generate a notice. Do you know when we can start to test d7 on the new testbot? Then this would fail easily and prove the bug.

I fired up two ad-hoc tests for this issue to test #47 and #50.

The tests only results are (will be) here:
https://dispatcher.drupalci.org/job/DCI_XML/161/

And the patch is here:
https://dispatcher.drupalci.org/job/DCI_XML/162/

In #56 the test-only patch is red with:

Error Message

exception: [Notice] Line 1236 of includes/bootstrap.inc:
Array to string conversion

and #50 fixes it, so re-uploading that.

Anyone else care to give this a final review?

Also had this problem, #57 fixed it.
Drupal 7.41, PHP 5.6.14.

See my comment in #8... I think all of that still applies?

And related to that, for Drupal 7 couldn't we just call file_download_headers() here rather than duplicating its code? (Drupal 8 doesn't have that function, although probably it should be added as part of this issue so it can be reused in the Drupal 8 patch also.)

As stated in #25 I have tested this on D8 and the bug does not exist there. Granted, this might as well be stated in the IS, it is not so easy to just find randomly in one of the comments.

To be 100% sure, I just tested it again today. But don't take my word for it, here is a test-only patch that should pass on D8. :)

Regarding the other points in #8 and #59:
- We could absolutely comment it better. And use file_download_headers. Definitely. I'll write a new patch with that. (after this d8 patch).
- There is only one other call to module_invoke_all('file_download') in the codebase, and it is also in image.module. But the problem would not apply here, as it does not try to add these headers afterwards. The call is here: https://api.drupal.org/api/drupal/modules!image!image.module/function/im...

Back to 7.x

For d7, with more comments and using file_download_headers

Did not do an interdiff since the patch is so small (and the change so apparent) :)

Ah, it looks like Drupal 8 never uses array_merge_recursive() when invoking any hook, that's why.

-      $headers = module_invoke_all('file_download', $image_uri);
-      if (in_array(-1, $headers) || empty($headers)) {
+      // We do not call module_invoke_all('file_download') here because that
+      // calls array_merge_recursive which in turn can make some headers end up
+      // as an array if more than one module returns the same header.
+      // @see https://www.drupal.org/node/1891228
+      $headers = file_download_headers($image_uri);

So since we're calling file_download_headers() I don't think we need this code comment - the reasoning is already documented inside that function and we don't need to explain why we're using the correct API :) (Sorry for the confusion - in my comment from long ago I was probably thinking we needed a comment here because file_download_headers didn't even exist then so calling it wasn't an option.)

There is only one other call to module_invoke_all('file_download') in the codebase, and it is also in image.module. But the problem would not apply here, as it does not try to add these headers afterwards.

Agreed. That one should probably be using file_download_headers() anyway, but isn't needed to fix a bug so doesn't need to be part of this issue.

I agree. So here is one with no comments :)

Yup, that looks good to me, and the patch works as expected.

*Bump*

I would _really_ like to see this issue not becoming legacy with Drupal 7. Can we have the patch in the very next core release pretty soon, please? Really annoying thing if you work with a lot of private file image styles.

Can I assist in accelerating the solution? Please let me know. Thanks!

accidentally changed issue state (re-rolling)

Yup, the plan was always to get it into the next release - I just left it for a while in case there were additional reviews.

Committed to 7.x - thanks!

Removing backport tag since it turned out this did not need a Drupal 8 fix. If someone wants to forward-port the tests to Drupal 8 I guess it would be nice to have a followup issue for that - it could be created and then linked back here.

Thanks @David_Rothstein. I've created a stub issue for those D8 forward port tests #2660564: Forward port tests for image_style_deliver can create invalid headers

Great, thanks from here, too!