Image styles fails with absolute path in $settings['file_public_path']

I think the patch is not fully correct. Instead of using the path from $settings['file_public_path'] we should use the path from $settings['file_public_base_url'].

This is necessary for sites that want to run Drupal in a secure fashion with an isolated web root. They have to set

$settings['file_public_path'] = 'web/files';
$settings['file_public_base_url'] = 'https://klau.si/files';

for example. As this is an important security improvement and this breaks image styles on such sites I think this should be major.

Here is a new patch that uses $settings['file_public_base_url'] if set.

I can successfully apply #7 patch to Drupal 8.6.16, and seems working.

Patch from #7 works for me on Drupal 8.9.6, both Media and regular images.

Now that #3036494: Race condition in ImageStyle::createDerivative() is committed can we try to reproduce this?

Attached re-rolled patch.

A note about configuration that works, and what doesn't work.

What does work #1: where the Drupal base URL doesn't end with a subpath, and file_public_base_url ends with a subpath. For example, Drupal installed as https://myhost, and file_public_base_url as https://myhost/files.

What does work #2: where the Drupal base URL ends with a subpath, and file_public_base_url ends with a different subpath. For example, Drupal installed as https://myhost/drupalpath, and file_public_base_url as https://myhost/drupalpathfiles.

What doesn't work: where the Drupal base URL ends with a subpath, and file_public_base_url ends with a subpath of the Drupal base supbath. For example, Drupal installed as https://myhost/drupalpath, and file_public_base_url as https://myhost/drupalpath/files. Here's what seems to be the reason: in this case, the image.style_public route is set up with path /drupalpath/files/styles/..., but since REQUEST_URI also begins /drupalpath/files/styles/... and Drupal's base path ends /drupalpath, Symfony strips the leading /drupalpath element from the REQUEST_URI and looks for a route /files/styles/... instead ... but there isn't one.

And (of course?) if file_public_path points to an absolute path that's outside Drupal, that directory needs to have its .htaccess rewrite to Drupal in the case of a miss.

The existing, generated .htaccess has:

# Turn off all options we don't need.
Options -Indexes -ExecCGI -Includes -MultiViews

# Set the catch-all handler to prevent scripts from being executed.
SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006
<Files *>
  # Override the handler again if we're run later in the evaluation list.
  SetHandler Drupal_Security_Do_Not_Remove_See_SA_2013_003
</Files>

# If we know how to do it safely, disable the PHP engine entirely.
<IfModule mod_php7.c>
  php_flag engine off
</IfModule>

I inserted this (copy/pasted from Drupal's own .htaccess) at the top (where, as per my previous comment, my Drupal is installed at /drupalpath):

RewriteEngine on
# Make sure Authorization HTTP header is available to PHP
# even when running as CGI or FastCGI.
RewriteRule ^ - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_URI} !=/favicon.ico
RewriteRule ^ /drupalpath/index.php [L]

Any problems with this? (I'm not sure if I need the line for authorization, and I'm reasonably sure I don't need the line for favicon.ico.)

A follow-up to something I wrote earlier:

What does work #2: where the Drupal base URL ends with a subpath, and file_public_base_url ends with a different subpath. For example, Drupal installed as https://myhost/drupalpath, and file_public_base_url as https://myhost/drupalpathfiles.

It turns out that although this example works for thumbnail generation, it breaks file download URLs. In fact, in this example, note that the base URL is not "different" enough, it's now a prefix of the file_public_base_url. To make file download URLs work, the example must be changed to, e.g., https://myhost/filesdrupalpath.

See core/lib/Drupal/Component/Utility/UrlHelper.php's function externalIsLocal(), as invoked by core/modules/file/FileUrlGenerator.php's function generate().

Moved to merge request.

Thank you, I took a look at the MR and left a couple of comments, see above. Also, the MR should fixed to point against 10.1.x to match the issue version.

Use 9.5 MR for Drupal 10.0.

Is this testing relative paths that resolve outside of the Drupal webroot? For example, I've been testing a setup where Drupal is installed in a subdirectory on the server, using $settings['file_public_path'] = '../public';. So a Drupal page would be https://my.server/path/to/drupal/web/node/1 and an uploaded image would be https://my.server/path/to/drupal/public/image01.png. That doesn't work with image styles, so I tried setting file_public_base_url to 'https://my.server/path/to/drupal/public'. That has many of the same symptoms described here.

Is adding the clean URL redirect lines to the public file folder .htaccess file going to be a requirement in any case? In order to do things like process the virtual URLs of image styles, for instance.

Reroll the patch

Can MR be updated to 11.x please

have not reviewed for tests yet

is there a MR for Drupal 10.3?

@SocialNicheGuru current MR's diff is working for 10.3.

@smustgrave done

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Should be good now and pass phpcs

Thanks! Appears to still need tests though.

Posting the latest diff as a patch.