Incorrect image URL using an image style should return a 404 instead of a 403

This was causing major performance issues for a very large site we launched. An external app vender had stripped the query parameters from image requests, thus causing a huge amount of 403s. These were all hitting origin and made up for 70% of the origin traffic.

I reviewed the thread which introduced this feature from the security patch, and from what I can tell a 403 was chosen because a menu access was refactored from returning false. My guess is since the original solution using menu access would return a 403, the same value was kept in the refactoring.

Does anyone have any reasons this should not be a 404?

I would guess the rationale for a 403 is that your request to the server is trying to do something (generate an image) but the system is choosing not to allow you to do it (e.g. because you have provided an invalid token)...?

In any case, is this issue valid for Drupal 8 also? If so, it would need to go there first, then backported.

I think we should follow the path set by the just committed #927138: Fail image generation with 404 instead of 500, when source file is missing

No, it's separate. It addresses the actual image missing, where as this addresses the wrong token.

I confirmed this issue still exists in Drupal 8. Oddly, the test case appears to already test for a 404- I don't know how it was passing in the first place.

Here's a patch against 8.0.x-dev, functionally identical to the 7.x version in #4.

Umm... I have no idea why tests are repeatedly failing. Each time it's with a different error- none of them seem related to the patch.

Edit: tests are passing now.

The patch 'drupal-2211429-13.patch' in comment #13 is not applied and gives error.
Attaching snapshot for referenc

Agreed @Truptti - a lot can change in 9 months. You up for trying to re-roll this patch?

Rerolled the patch, and it worked for me..Rest on the Test Bot

On applying patch, i am getting warnings
warning: core/modules/image/src/Controller/ImageStyleDownloadController.php has type 100644, expected 100755
warning: core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php has type 100644, expected 100755

root@ubuntu:/var/www/d8# git apply -v Incorrect-url-image-2211429-22-8.patch
Checking patch core/modules/image/src/Controller/ImageStyleDownloadController.php...
warning: core/modules/image/src/Controller/ImageStyleDownloadController.php has type 100644, expected 100755
Checking patch core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php...
warning: core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php has type 100644, expected 100755
Applied patch core/modules/image/src/Controller/ImageStyleDownloadController.php cleanly.
Applied patch core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php cleanly.

Patch attached. Please review.

Did a quick test of #26. Seems to work as intended.

Patch #26 has been tested and working fine

Also (manually) tested #26 and can confirm that it seems to work fine. :-) Since automated tests also appear to be in place, look correct (and pass), I think this can be marked RTBC. It would be nice to see this committed before it needs another re-roll. ;-)

Discussed this with @alexpott a bit, we disagreed on the solution somewhat, so trying to document my take on the 404 vs. 403 here. Alex's point of view was that we should 403 if the file could be generated but we're refusing, and 404 when it's not there (this is not our current behaviour when we have a bad itok and also a missing file).

There are several scenarios. The first value in parenthesis is the current behaviour, the second is what the patch changes it to (if there's a change).

1. Valid itok + derivative on disk (200)
2. Invalid/no itok + derivative on disk + source image (200)
3. Invalid/no itok + derivative on disk + no source image (200)
4. Valid itok + source image (200)
5. Valid itok + no source image (404)
6. Invalid itok + source image (403) (404) - we check the itok before checking file_exists() and return early
7. Invalid itok + no source image (403) (404)

For me, given that any other request with a correct itok will result in a valid image on disk that's then accessible, we should not treat the itok check as an access check as such, but just determining whether something (the generated file) is going to be returned by that location or not, which only needs to happen when it's not already there.

The only reason to stick to a 403 semantically here would be if we weren't using on-disk derivatives at all but were access checking the files each time, but that's not what the itok is for - it's only a protection against DDOS.

Given that links with and without the itok are fairly common, the existence of the file on disk may or may not be determined by the request at all - it's simply there or it isn't, and if it isn't only then does the itok matter. So a 404 seems fine.

Given #32 I think that returning a 404 is reasonable.

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -109,7 +112,7 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
-      throw new AccessDeniedHttpException();
+      throw new NotFoundHttpException();

I think some commentary why we return a 404 here for invalid itok's. Something like at this point we don't know if the source file exists on the server and in order to minimise disk activity we don't bother to check. Because we already know that we're going to return an error response and we chose a 404 because it is cacheable. Or something like that. Or borrow some of the words from #32

Okay. I'll get a patch together with the needed commentary for review. I think this is a good way to avoid regression (even more appropriate than just updating the tests to our current behavior). I'll include a reference to this issue.

Hopefully this fits the bill and we can just get this committed after 3 years. ;-)

@akashkrishnan What?

Rerolled against 8.6.x as it no longer applied.

Updated the issue summary and wrote an initial draft for a change record as I think this would be helpful to advertise for this change when it lands.

In the change record, I'd suggest:

upon trying to access an invalid image token derivative or an invalid image token with no source image available

...should instead be:

upon trying to access an image derivative with an invalid token, whether or not the source image is available

(I think the same text would be changed in both the before and after section.)

I'd perhaps also change "load balancer" for something like "caching reverse proxy" or just "caching proxy".

That makes sense! Diff here https://www.drupal.org/node/2962688/revisions/view/10931428/10931971

Quickly discussed the issue with @mcdruid and he suggested an improved comment and few other CR changes.

Core committers: please make sure to credit him, thanks!

I think the patch looks good, tests pass, and the CR makes sense. RTBC!

👍

Adding "Security improvements" tag, as this is about enhancing resilience to (D)DoS.

Crediting @catch, @cashwilliams and @mcdruid as per comments and reviews.

diff --git a/core/modules/image/src/Controller/ImageStyleDownloadController.php b/core/modules/image/src/Controller/ImageStyleDownloadController.php
index 7be5f870fd..3d4ee1cc81 100644
--- a/core/modules/image/src/Controller/ImageStyleDownloadController.php
+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -111,7 +111,6 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
       // image token is for DDoS protection rather than access checking. 404s
       // are more likely to be cached (e.g. at a proxy) which enhances
       // protection from DDoS.
-      // @see https://www.drupal.org/node/2211429
       throw new NotFoundHttpException();
     }
 

Removed @see on commit because git history gives you this. The @see to an issue nid is pointless.

Backported to 8.5.x since this has caused issues on real sites.

Thanks! Published the CR.