Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, image-style-404.patch, failed testing.

catch’s picture

Makes sense to me, you can hit random style paths and generate 500s at the moment.

douggreen’s picture

FileSize
998 bytes

rolling from drupal root this time

cyberswat’s picture

Status: Needs work » Needs review
marcingy’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
FileSize
833 bytes

Reroll and move to d8 as it needs to be fixed there first.

rasmusluckow’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, fail-image-generation-with-404-927138-5.patch, failed testing.

rasmusluckow’s picture

Status: Needs work » Needs review
FileSize
795 bytes

I have rerolled the patch for D8 and simplified it a bit.
I have manually testet that I get a 404 if the source file does not exist, thus quitting the script early.

h4rrydog’s picture

Assigned: Unassigned » h4rrydog
Albert Volkman’s picture

Assigned: h4rrydog » Unassigned
Status: Needs review » Needs work

Thanks for the updated patch, @rasmusluckow! This patch indeed properly returns a 404 instead of a 500. However, one small nitpick. There's an extra space in your closing bracket. Please re-roll the patch with that fix and I believe this should be RTBC.

+++ b/core/modules/image/image.moduleundefined
@@ -745,6 +745,12 @@ function image_style_deliver($style, $scheme) {
+     return new Response(t('Error generating image, missing source file.'), 404);
+   }
+
   // Don't start generating the image if the derivative already exists or if
   // generation is in progress in another thread.
   $lock_name = 'image_style_deliver:' . $style['name'] . ':' . drupal_hash_base64($image_uri);
h4rrydog’s picture

Assigned: Unassigned » h4rrydog
Status: Needs work » Needs review

Also manually confirmed that the patch works, resulting in a 404 instead of a 500. This was tested on Ubuntu with nginx / php-fpm.

h4rrydog’s picture

Status: Needs review » Needs work

Crosspost. Updating status.

rasmusluckow’s picture

Assigned: h4rrydog » Unassigned
Status: Needs work » Needs review
FileSize
893 bytes
792 bytes

Fixed indentation.

h4rrydog’s picture

Status: Needs review » Reviewed & tested by the community

Cool. Indent fixed.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. Thank you.

webchick’s picture

Status: Patch (to be ported) » Needs review
FileSize
772 bytes

Here's a re-roll I did on stage at Drupalcon Munich. I hope it works. :)

bas.hr’s picture

webchick’s picture

Status: Needs review » Needs work
+++ b/modules/image/image.moduleundefined
@@ -797,6 +797,12 @@ function image_style_deliver($style, $scheme) {
+    return new Response(t('Error generating image, missing source file.'), 404);

I'm an idiot. This is Symfony code. :)

Albert Volkman’s picture

Status: Needs work » Fixed

Marking this complete since imagecache isn't core in D6.

webchick’s picture

Status: Fixed » Needs work

It is in D7 though. :)

Albert Volkman’s picture

Ah, my fault. I thought it had been resolved in the other issue.

droplet’s picture

Status: Needs work » Needs review
FileSize
708 bytes
h4rrydog’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
115.19 KB

This patch works for me in D7, properly returning a 404 (after patch) instead of a 500 (before patch).

screenshot.png

Albert Volkman’s picture

Would it not be better to use drupal_not_found()?

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Oops. Yep, I think Albert is right. Take a look at http://api.drupal.org/api/drupal/includes%21common.inc/function/calls/dr... for some examples.

handrus’s picture

FileSize
540 bytes

Patch using drupal_not_found().
Thanks for the suggestion and guidance.

handrus’s picture

Status: Needs work » Needs review
FileSize
540 bytes

Changing status and re-submiting patch

handrus’s picture

As @caiovlp suggested I changed exit; for drupal_exit().
Thanks for reviewing this Caio!

caiovlp’s picture

Status: Needs review » Reviewed & tested by the community

Patch #28 works fine and looks good.

manarth’s picture

Status: Reviewed & tested by the community » Needs work

The patch doesn't differentiate between a missing source-file, and an error preventing the image-style applying (such as corruption in the source image).

This is an important distinction, because if the image is missing, 404 is semantically correct, whilst if the image exists but cannot be converted because of a server error, 500 is more appropriate.

In #1226512: Image module reports missing images as error 500 (server error) instead of 404 (not found). we used an is_file($image_uri) check to serve 404 only when the source-image is missing - http://drupal.org/files/source_image_exists-1226512-19.patch

manarth’s picture

Issue tags: +Needs tests

NB: That file_exists behaviour is used in D8 (commit from #15 / diff)

manarth’s picture

Status: Needs work » Needs review
FileSize
1.44 KB

This patch checks if the source-image exists, and only returns 404 if the source image is missing.
I've also added a test for that.

manarth’s picture

This patch adds the same watchdog() message that's used in D8.

David_Rothstein’s picture

+    drupal_not_found();
+    drupal_exit();

Shouldn't this be return MENU_NOT_FOUND? (see http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_not_found/7)

Does the test need to go in Drupal 8 too? (Also, minor issues with it: "source-image" => "source image" and "existant" => "existent")

manarth’s picture

Good points, I've updated the patch to use MENU_NOT_FOUND, corrected source-image/existant, and rephrased the assertion message slightly.

I've also created a patch to add the test to D8.

Status: Needs review » Needs work

The last submitted patch, image404_drupalexit-927138-35.d8.patch, failed testing.

manarth’s picture

Status: Needs work » Needs review
David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev

That looks pretty good to me, although is there a reason for the triple slash in the tests ("public:///foo.png")? Let's see if the tests pass for Drupal 8 anyway.

In the watchdog message, it looks like it's using the wrong variable (derivative file rather than the original image file), given the way the sentence is worded? Actually both would be useful in this log message, but the original image file is most useful since that's what's missing. Looks like the committed Drupal 8 patch had the same problem though...

David_Rothstein’s picture

manarth’s picture

The triple-slash? That's me spending too much time writing provisioning scripts where the triple-slash is required: puppet:///modules/foo :-D

For the watchdog message, if we're going to change it, we should change it for both D7 and D8, so it's consistent.

How about this?

watchdog('image', 'Source image at %source_image_path not found using derivative path %derivative_path.',  array('%source_image_path' => $image_uri, '%derivative_path' => $derivative_uri));

Status: Needs review » Needs work

The last submitted patch, image404_drupalexit-927138-35.d8.patch, failed testing.

David_Rothstein’s picture

Ah, it's been a while since I've used Puppet, but that makes sense :)

The watchdog message looks good although instead of "using derivative path" I might say something more like "while trying to generate derivative image at"... I think it makes it more clear what's going on.

manarth’s picture

Here's the D8 patch with the test, and the changed watchdog message.

Here's the message as it appears in watchdog now:

Source image at public://foo.png not found while trying to generate derivative image at public://styles/foob/public/foo.png.

I've also corrected a mistake I made in the test, I originally had:

+    $non_existent_uri = 'public://foo.png';
+    $generated_uri = image_style_path($this->style_name, $non_existent_uri);
+    $generated_url = image_style_url($this->style_name, $generated_uri);

That second line shouldn't be there…instead we simply generate the URL with this:

+    $non_existent_uri = 'public://foo.png';
+    $generated_url = image_style_url($this->style_name, $non_existent_uri);
manarth’s picture

Status: Needs work » Needs review
manarth’s picture

Version: 8.x-dev » 7.x-dev
FileSize
1.61 KB

And the D7 patch.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community
FileSize
1.71 KB

The Drupal 8 patch in #43 looks ready to go to me.

(I'm just fixing an instance of "Test" => "Tests" in the function documentation in the attached.)

alexpott’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests

Committed f816c89 and pushed to 8.x. Thanks!

handrus’s picture

Status: Needs review » Reviewed & tested by the community

What about the backport to D7?
The patch look good to me.

aviindub’s picture

I just deployed the d7 patch in production. i can't vouch for the .test file, but the .module part of the patch is working as intended.

Taz’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
FileSize
1.7 KB

Re-rolled the above patch to use a lighter 404, as per the rest of the image.module

Surely there's no need to cause a full 404 page render for failed images and put a hit on the server.

It's very easy to DDos a Drupal site under the image styles path without this patch, thus I am raising the priority of this.

Moving back to 'needs review' as this is a new patch for the testbot & community.

David_Rothstein’s picture

How is this a particular DDos risk? If we return a normal 404 I don't see how it's different from any other path on a Drupal site that returns a 404...

I can see the logic for wanting to return a fast 404 anyway (same reason as in drupal_fast_404()) but to do that we'd presumably want to respect some of the settings in the drupal_fast_404() function, plus it would need to go in Drupal 8 first. So perhaps we should get the original patch in first, and discuss that in a followup.

catch’s picture

Some load balancer configurations will take a web server out of rotation if it returns a certain number of 500s. Returning a full 404 page isn't any worse than anything else agreed.

cdenneen’s picture

I know it's been a few months since latest comment. Can anyone tell me if this patch is complete? Will it make it into 7.x?
Is it safe to use: https://drupal.org/files/927138-50-lighter404.patch

mgifford’s picture

50: 927138-50-lighter404.patch queued for re-testing.

mikeytown2’s picture

Issue summary: View changes

Did a search and This is the only issue I found semi-related to this one that I just created #2267639: Image styles do not get created if the original file contains a plus sign or is double url encoded

JKingsnorth’s picture

Just to say, this would be a great change but could we have a more useful 'on-screen' error message too, rather than just 'Error generating image.'

How about:

'Unable to generate the image because the source file does not exist.'

Note that this only applies to the currently still under review Drupal 7 patch. The Drupal 8 patch already does this.

[We've just been scratching our heads over this error for ages, checking server configuration and .htaccess files for the cause. But it was the simple problem that the images didn't exist on the server. A more useful error message would have saved us a lot of time (as would taking a proper copy of our /files/ directory)]

JKingsnorth’s picture

FileSize
1.72 KB

The attached patch makes this small amendment (better on-screen error message) to the patch in #50

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

@John, so you just added ", missing source file" to print t('Error generating image, missing source file.');

Simple enough change. Still a big change from #45 though. Would probably be simpler to just have #45 RTBC and open up the variation in #57 as a new issue as it would also need to be dealt with in D8.

We still need a follow-up if we want to stop "full 404 page render for failed images". As @David_Rothstein suggests, if there is a real concern with DDos with the code in D8.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: 927138-57-lighter404.patch, failed testing.

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Reviewed & tested by the community
JKingsnorth’s picture

Fair enough, just thought I'd get it out there =]

mgifford’s picture

That's totally fine.

FluxSauce’s picture

#57 works for me on Drupal 7.32.

David_Rothstein’s picture

It seems like there's a bit more consensus on #45 for now, so let's go with that one (we can deal with further refinements as a followup - especially since as I mentioned above this really seems like it should integrate with the drupal_fast_404() settings if it's going to do a fast 404). Will retest to make sure it still passes tests.

Although it is true that #57 is closer to what went in to Drupal 8. The on-screen "Error generating image, missing source file" message doesn't make lots of sense to me though; that is a very technical message to potentially show an end user. The technical message (with much more detail than that) is already in watchdog.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

  • David_Rothstein committed c8a26f2 on 7.x
    Issue #927138 by manarth, handrus, rasmusluckow, douggreen, .John, Taz,...

Status: Fixed » Closed (fixed)

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