Image styles - thumbnails are broken in config page when private file system is used

Working on it now.

This is a legitimate issue. I was able to reproduce with the clear steps provided in the description. The patch provided also solved the issue. However, it contained the hard-coded link to the sample image and some coding standard issues. I have solved both of this in the new patch. Adding interdiff as well. Thanks!

Patch looks good to me.

Would be better having test coverage.

Yep some tests would be great. Also I'm worried about the solution bypassing security features. I think we need to work out why the security features fail rather than bypass them.

I can confirm the problem still exists. Just experienced this.

Based on the request in #7 from @alexpott I did a bit of digging and refactoring to avoid overriding the security checks.

The first security failure inside of of ImageStyleDownloadController is that currently inside of image.admin.inc an itok is not generated for the file, a direct path is generated to the file. ImageStyleDownloadController would expect something like private://path/to/file (private://core/modules/image/sample.png for the default image) I've moved the code into image.admin.inc to generate an itok. I'm not checking for itok suppression/disable since this is only used for the sample pages, even if they are disabled this should not break the validation at it will just be skipped.

The second access failure is caused by the fact that the hook_file_download() inside of the image module only approves files paths that start with 'style/' which in the case of the default image the path will be 'core/modules/image/sample.png' (at least with the default image) and because of this access is denied because no module granted file access to the path. I've added this code into image.module so security checks will now pass.

The next failure isn't a security check fault but a file existence check failure caused when the ImageSyleDownloadController tries to access "private://core/modules/image/sample.png" to generate the derivative which (if you have your server configured securely) will not exist. I kept the code from patch #4 for this

Tests: At the moment I'm not sure how to write a test for this. I think we would want preview link from template_preprocess_image_style_preview() (or from the full rendered page) and than check with a drupalGet() to be sure it returns an image, but I'm not sure how to (reliably) get the link to pass to drupalGet()

I can confirm the problem still exists.

Still experiencing this issue. The proposed patches are applied, but do not work unfortunately.
Thx!

I can confirm now that switching that radio button for Default download method to > Public local files served by the webserver, (/admin/config/media/file-system) does solve the issue... But what are the implications otherwise?

Can anyone explain the difference between these settings and what impact allowing the webserver to handle local files rather than Drupal?

Having this information might allow us to make a more informed decision on how to mitigate the issue most effectively for each case.

There is much documentation on drupal.org and elsewhere about Drupal's public and private file systems. Search for "Drupal private public file system".

I am fully aware of the implications of the Private vs. Public file system. That isn't exactly what I was after... I thought that the issue here was a more specific one related to the Media Styles page and how and where the style demonstrator located the sample image.

I suppose I simply missed the idea that the setting for 'File System > Default download method' was impacting beyond the constraints of this Media Styles issue!

Now I am wondering if this isn't simply a permission setting that is preventing an administration page from accessing the thumbnail and file (/core/modules/image/sample.png) after the style has been applied. or the location if it is different, of the file after it has been processed because I would think it should be in the /temp directory, if the original isn't simply styled using the settings from the specific image/media 'Style name'.

I suppose in other words, I am unclear if the page at: /admin/config/media/image-styles/manage/crop_thumbnail
displays the file (/core/modules/image/sample.png) in it's original form and then with the "Sample modified image" applies the style to the same image, or if it saves the processed file in a temporary location since the very next field in the File system settings is "Delete temporary files after..."

FYI, here is my settings at /admin/config/media/file-system

Public file base URL: https://website.net/sites/default/files
The base URL that will be used for public file URLs. This can be changed in settings.php

Private file system path: /home/account/public_html/../private
An existing local file system path for storing private files. It should be writable by Drupal and not accessible over the web. This must be changed in settings.php

Temporary directory: /tmp
A local file system path where temporary files will be stored. This directory should not be accessible over the web. This must be changed in settings.php.

Default download method:
Public local files served by the webserver.
Private local files served by Drupal. (selected)
This setting is used as the preferred download method. The use of public files is more efficient, but does not provide any access control.

And finally for reference:

Reroll for Drupal 9.5+

fixed issue:

Error messageWarning: Undefined variable $sample_image_uri in Drupal\image\Controller\ImageStyleDownloadController->deliver() (line 184 of core/modules/image/src/Controller/ImageStyleDownloadController.php).

Reroll for Drupal 9.5+ #21

#23 does not appear to be a complete patch, parts of it are already in Drupal Core 9.5 and on first glance it appears to be missing the previous work without an explanation as to why. Hiding the patch to avoid confusion.

Back to 11.x as the mainline, as a bug it’s going to need to be solved in latest version and backported.

Rerolled for 10.1.x.

For the tests.

Fix CCF Errors.

Please also check the tags. Was tagged for tests which are still needed

Issue summary will probably need work too

Hello, I've created tests for the 3127116-27.patch. I would appreciate it if you could review the changes and let me know if everything is in order. Thank you.

Fixed coding standard issue

Ran the tests locally and they failed with

Behat\Mink\Exception\ExpectationException : Current response status code is 403, but 200 expected.

It's recommended to start using MRs as patches are going away.

Looking at your test though and for your first time think you did a pretty good job

$original_path = \Drupal::config('image.settings')->get('preview_image');

Could probably use the local config reference

Some comments throughout would help too for example here

    $preview_file = $style->buildUri($original_path);
    if (!file_exists($preview_file)) {
      $style->createDerivative($original_path, $preview_file);
    }

Also why the need for the file_exist?

Overall though good work!

Moving to NW for the issue summary update I meant to tag before. Filled in some sections but left the rest to TBD, if not applicable to this issue just replace with NA. Helps the review

Issue summary looks good.

Were some code suggestions in #32 also

I've created a merge request and made the suggested code modifications. Could you please review and confirm if everything is in order?

Ran test-only feature

There was 1 error:
1) Drupal\Tests\image\Functional\ImageAdminStylesTest::testPreviewImageShowInPrivateScheme
Behat\Mink\Exception\ExpectationException: Current response status code is 403, but 200 expected.
/builds/issue/drupal-3127116/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3127116/vendor/behat/mink/src/WebAssert.php:130
/builds/issue/drupal-3127116/core/modules/image/tests/src/Functional/ImageAdminStylesTest.php:561
/builds/issue/drupal-3127116/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 8, Assertions: 184, Errors: 1.

Which matches/followed the steps in the issue summary.

Not 100% sure about checking for a sample image but the issue is solved.

I'm triaging RTBC issues. I skimmed the IS and the comments. I did not find any unanswered questions. However, there is an unresolved comment from alexpott.

I added a link to the answers about security to the Issue Summary to help anyone working on this to find that discussion.

I then read the MR and there are comments that should be changed. Because of the existing unresolved comment and my further ones, I am setting this back to Needs work. There isn't much to do though.

1 small nitpicky item. But appears rest of feedback has been resolved. Free to self RTBC after that 1 change.

Committed/pushed to 11.x, cherry-picked to 10.3.x and 10.2.x, thanks!