Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
When downloading an already generated derivative of a private file, the original image is delivered instead of the derivative. This is proved by the "test only" patch.
Proposed resolution
Fix the call to private files download controller in \Drupal\image\Controller\ImageStyleDownloadController::deliver().
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | drupal-private_image_styles-2702227-10.patch | 3.04 KB | alexpott |
| #13 | interdiff.txt | 1.28 KB | claudiu.cristea |
| #13 | 2702227-13.patch | 2.75 KB | claudiu.cristea |
| #13 | 2702227-13-test-only.patch | 1.96 KB | claudiu.cristea |
Comments
Comment #2
i15 commentedUp
Comment #3
i15 commentedUp
Comment #4
cilefen commentedComment #5
i15 commentedup
Comment #6
i15 commentedComment #7
marco.giaco commenteddeliver method in ImageStyleDownloadController class
is returning
parent::download($request, $scheme)
in case the file-system is private and the derivative image exists.
This is always returning the original image.
This invoke file_download on original image and continue returning the derivative.
Comment #8
duneblI confirm that #7 is doing the job!!
Thanks for it
Comment #9
jeroen.b commentedHere's my patch from #2776759: Image styles on private files returns original file on subsequent requests (I didn't find this issue, sorry), which includes a test.
I'm also not sure #7 was correct, it seems to skip access checking for new derivatives, which normally would be done in
parent::download().I will first attach the patch only.
Comment #10
jeroen.b commentedHere's the patch with fix.
Sorry, name of previous patch was wrong, it should be test-only, not patch-only.
Comment #12
jeroen.b commentedComment #13
claudiu.cristeaTentatively set this as Critical. IMO is critical because allows access to data (original images) that some sites might consider forbidden..
I considered an alternative approach that passes the control to private file controller as soon as possible, like the current code from HEAD.
Comment #15
jeroen.b commented@claudiu.cristea That also works. I don't think it's a good design decision to make the private download code different from the public download code though.
Also, are you sure this is a security risk? I think you can always download the original image if you have access to the image style.
Comment #16
claudiu.cristeaWith the core settings there's no security risk but it might be on sites where custom code denies the access to original. Imagine a commerce that sells hight resolution posters. There you can see derivatives but you cannot download the original. Event there, if the access is denied via hook_file_download() there should be no problem.
Both solutions are keeping the
if ($scheme == 'private') {...}so why not deliver the image earlier? I think this was intended also by the original buggy code. But I'm not insisting on that solution.Comment #17
jeroen.b commentedAh yeah, you are correct about that.
Yes but my patch only does the actual access check inside the if, after it just goes into the normal code. I don't really have a good reason for it other than it's just confusing. I'm all for returning early, but then it should be done for both public and private files.
Comment #18
pwolanin commentedClarity and consistency in the code are important to avoid later regressions.
Comment #19
zaporylieIMO #13 looks good enough to be at least reviewed by core committers if not committed right away. It definitely solves the problem, question is if it follows best Drupal practices. I'm not 100% convinced by modifications to request on the controller level but maybe that's the best we can do in situation like this.
Comment #20
alexpottPersonally I think the patch in #10 is correct because if you look at the patch in #13 it calls
parent::download()which does pretty much the same thing. I think it way more consistent to not call the parent. We should file follow-ups to stop doing unnecessary work in the rest of the method - we do an unnecessary cryptographic call even if the derivate exists. And changing the request does not look great.Re-uploading #10 so it is the latest patch.
Comment #22
zaporylieI can confirm that #10 also fixes this critical issue and I agree it feels better this way. I found two more bugs related to this issue and created follow-ups, major #2786735: Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem and minor #2786763: Edit incorrect headers for image derivative if the "Convert" effect is in use, both are postponed for now.
Comment #23
zaporylieComment #24
claudiu.cristeaI agree with @jeroen.b comment from #15 and @alexpott comment from #20. So this is RTBC.
I also added the followup requested in #20: #2786855: Don't compute the lock name if not necessary when generating image derivatives.
Comment #25
alexpottCommitted and pushed bba2fda to 8.3.x and 8316205 to 8.2.x and a6e1c22 to 8.1.x. Thanks!