"Restrict images to this site" blocks image style derivatives

This means that the entity embed module cannot display images for view modes that are using an image style.

This isn't terribly elegant, but what about adding

// Check for query string and remove it if present.
$parse_query_string = substr($local_image_path, 0, strpos($local_image_path, '?'));
if (!empty($parse_query_string)) {
  $local_image_path = $parse_query_string;
}

to _filter_html_image_secure_process() to strip the query string before calling @getimagesize($local_image_path) ?

This worked in my situation.

Patch attached

When trying to reference an image from a Javascript, I am running into 403 forbiddens, I'm assuming because that image style is not referenced from a view in a 'hard coded' way and it's not giving a good token. Posting on this thread because this is a prominent search result for it. Forcing through the image reference, preventing the forbidden, would be nice.
Ah see also: https://www.drupal.org/node/1923814

... and via David Rothstein here #1934498: Allow the image style 'itok' token to be suppressed in image derivative URLs:
The particular configuration setting added to Drupal 8 in this issue can be enabled via $config['image.settings']['suppress_itok_output'] = TRUE in settings.php followed by a cache clear. However, images in Drupal should not be broken just because there are tokens in the path.... So instead of suppressing the tokens and lowering security, it would make more sense to debug what is breaking them.

Closed #2772617: Handle files uploaded to the private storage as internal as a duplicate.

Also closed #2794513: FilterHtmlImageSecure considers private:// files insecure as a duplicate.

Inheriting issue tags from #2772617: Handle files uploaded to the private storage as internal.

The patch in #2772617: Handle files uploaded to the private storage as internal looks more proper to me - should it be added to this issue, too?

Per #12 and #2772617-10: Handle files uploaded to the private storage as internal I'm bringing the patch I wrote here.

Does the private file patch actually address the issue with image style derivatives though? Those are in the public files directory generally, and the problem is due to the query string token appended to the image path.

You changed the description to eliminate the actual issue this was created to report.

I've tried to create a filter that replaces inline images with their style derivatives and face the same problem: "getimagesize($local_image_path)" fails due to the "itok" parameter in the path to the image; as a results the images are considered invalid.

I tried the patch from #3. It worked for some images, but some were still removed. Tested using Entity Embed in CKeditor.

Combining patches.

#3 image_restrict_image_styles-2528214-3.patch
Image style derivatives.

#13 2528214-13.patch
Private images.

Note this is also blocking contrib now:

This still needs tests for sure.

Due to the variable naming in this patch it could occur that the alter hook triggered in the _filter_html_image_secure_process function was passed an object that implemented ImageInterface instead of a DOMElement which causes errors when filter_filter_secure_image_alter tries to call setAttribute.

We've been unable to find the exact replication steps but if you follow the code you can see where it happens:

Line 799 and 780 do the following.

      $image = \Drupal::service('image.factory')->get($private_image_path);
      if ($image->getFileSize()) {
        // The image has the right path. Erroneous images are dealt with below.
        continue;
      }

Before this the $image variable contained a DOMElement instance. If the $image->getFileSize() returns TRUE then everything is okay as the alter hook on line 825 will not get invoked.

\Drupal::moduleHandler()->alter('filter_secure_image', $image);

However, if the getFileSize() call returns NULL or 0 (both valid values according to the ImageFactory::get documentation) then the continue is not invoked and the alter hook is now called with an \Drupal\Core\Image\Image object instance.

The attached patch renames the loaded image object so that the DOMElement remains intact for the alter hook.

#24 works for 8.4.5, but I don't set it to RTBC since testing needs to be done in 8.5.x

Patch #24 does not work for 8.5.3 for embedded images with image styles.

Like Wim said in #22, this needs tests.

A tip when using entity embed, place "Display embedded entities" after "Restrict images to this site" in the "Filter processing order" for your text format. That way the Restrict images will run before embedding the entity (the image), and thus not affecting it.

Added tests. No interdiff because the only changes are in the test_only patch.

I'm continuing to work on the 8.5.x patch because that is the version I require, but I will re-roll for 8.6.x too once the patch is stable.

Correct the test_only patch (I have a suspicion that this might pass when it should fail, but I can't run functional tests locally).

#32 was full of mistakes, so let's pretend that never happened!

Patch in #34 no longer applies. Re-rolled.

Ignore the patch in #36 that was rolled against 8.6.x.

This still not working properly. When I used file which has whitespace in name (e.g. file name.png) image ends up as removed. When renaming file to filename.png and re-uploaded, it showed up as expected. URL in first case contains %20 and probably this is causing issue? I did't really looked in code, sorry. Most probably some small fix is needed.

I added simple str_replace(). Probably not best solution for all cases (special char cases maybe, e.g. š, ņ, ē), but currently fix issue for me.

@biguzis
Thanks for the patch! I was using #24 which at least solved wsod, but still has the same problem as you describe and your patch works for that.
Like you say I also think there are better solutions than the str replace. Doesn't Drupal offers a sanitize function that will strip it the correct way?

Patch in #40 not longer applies in Drupal 8.6.x.

Re-rolled.

Added a new patch as an addition to #40, where I altered the str_replace by urldecode, this should offer a more wide range instead of just the %20

Note that because of the call to getimagesize() none of these patches work in case when the derivative doesn't exist at the time when the filter is applied.
I.e.:
- "public://my-image.jpg" exists.
- "/sites/default/files/styles/thumbnail/public/my-image.jpg" doesn't.

Marking this as "needs work" based on #49.

none of these patches work in case when the derivative doesn't exist at the time when the filter is applied.

Exactly. This is still a problem.

- "/sites/default/files/styles/thumbnail/public/my-image.jpg" doesn't.

This is the case that we run into in the Entity Embed contrib module, see #2752253-8: Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default.

@marcoscano pointed out while discussing this in #2752253: Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default that it actually doesn't make sense for this filter to be filtering <img src=…> that wasn't entered by the content author: if that HTML is imported from other content on the site (transcluded, embedded …), then it shouldn't be ignoring (i.e. not securing) the transcluded HTML.

This is a way we could achieve that.

I'm adding a patch based on #47, because it does not work with private paths for installations under a directory, ie http://example.com/drupal/folder as mentioned in the definition of base_path().

The only difference is that I removed the hard-coded leading slash of $private_path and replaced it with $base_path.

I just ran into this issue and read through the queue. Here is a swing at ignoring embedded media by way of inspecting the data attributes on the <img> tag. This has resolved our use case of using the wysiwyg to place private images on a content type.

@devkinetic: How are you embedding Media? Because #2940029: Add an input filter to display embedded Media entities and https://www.drupal.org/project/entity_embed both specifically ensure that their filters run after filter_html_image_secure, meaning that they are not adversely impacted by this.

So, I think you're referring specifically to images as inserted by EditorImageDialog? Those are not "media" as in Media entities.

(I know, Drupal's terminology is confusing here!)

@Wim Yes we are placing private files, and the getimagesize() failes on the /inline-images/ route.

Alright 👍

While #56 would work for those image URLs, it still won't solve the general problem. The general problem was being fixed in earlier patch iterations.

(My patch in #53 should also be ignored, it was also addressing a different, related problem, that now no longer is a problem.)

Ok so like 90% of the issues people report with this issue is that it fails when attempting to validate that the image actually exists. Has no one asked why the filter is even doing this check? I think the filter should just verify that the path to the image is local and that's it!

The description of the filter:

Disallows usage of <img> tag sources that are not hosted on this site by replacing them with a placeholder image.

I vote just removing the file exists check altogether.

As a site owner, I just want to ensure that images don't have sources from other websites. Full stop.

There is a bit of gray area… Does this filter show the red X on images missing from existence? Or does this filter show the red X when an image is referenced from an external location?

These are really two totally different cases, but as a site owner, I would really want the same red X functionally for both cases. I’d also want to ensure that I’m getting proper 404 logging for both cases, so they can be tracked down and fixed.

The filter description would be more like, “Replace <img> tags with a placeholder image when missing or externally hosted.”

The code would have to be modified to handle both cases individually, but treat them the same.

- missing public files
- missing private files
- externally hosted

I think that this could be solved by adding settings for the filter. Have a checkbox "Verify that image exists". We would have it checked by default for backwards compatibility. But turning it off would allow people who want to link to image style derivs and private files to still use the other portion of the filter.

If I get some agreement from others, I can update the issue summary and title to reflect that change.

We would still want to verify that a private file exists. As well as for image styles, we’d want to verify that the source image exists to build the derivative. If that checkbox handled this too, I’d agree.

Here's a re-rolled version of patch 55 (which itself was a tiny adjustment to patch 47). Sorry that excludes any of the other progress/thoughts since comment 47. This is just for anyone else like me that was using patch 47 on a site that needed upgrading.

Reroll for 10.1

In case anyone else runs into a similar problem - a site was showing a symptom like this, only the problem turned out to be a custom URL processor that was changing all image paths of a certain type from "public" to "private", regardless of where the source image was. After far too much time was spent digging through the problem, removing the few lines of custom code fixed it and the images loaded again. X-|

I've converted the patch from #71 to a MR to comply with current Drupal processes. Tests are successful and test-only changes show that the test will fail without the fix.

Have not reviewed yet but the issue summary appears to be incomplete,

Appears to be missing steps to reproduce section too

Adapted the issue description and added steps to reproduce.

Couple of comments on the MR.