When generating an Image style derivative of a remote_stream_wrapper wrapped file with a ".jpeg" extension, and an image style that sets the extension to ".jpg", both extensions end up in the requested path (".jpeg.jpg"). This ends up producing an access denied error by RemoteImageStyleDownloadController::deliver because the file path in the request is not what it is locally where it ends in ".jpeg" and not ".jpeg.jpg". Where I think this goes wrong is in ImageStyle::buildUri (not the core class but the module's own class). The extension that the image style expects is added here. Removing that call and just using the path directly does not cause the issue to appear as the filename is left as-is.
Steps to reproduce:
1. Create a managed file with a URI scheme supported by this module. We create image files linking to other Drupal sites manually for reasons, so a row in file_managed ends up looking like this:
"1", "90150000-6b00-4700-a700-c2fb1ce2c000", "en", "pexels-photo.jpeg", "http://some.url/sites/default/files/2019-07/pexels-photo.jpeg", "image/jpeg", "4000", "1", "1563500000", "1563500000"
2. Render the file as an image with an image style that changes the extension. We use Imagick module's Convert filter to force writing JPEG files, which sets the extension to ".jpg".
Omitting the call to addExtension in ImageStyle::buildUri seems to resolve the issue. It would be a very simple patch, but it is so simple that something tells me that I'm missing something...
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3068898-12.patch | 1.73 KB | aimevp |
| #5 | 3068898-5.patch | 1.7 KB | antongp |
| #3 | remote_stream_wrapper-image_derivative_managed_file_check-3068898-3.patch | 1.61 KB | Coops_ |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
amaisano commentedI am also seeing this issue. Drupal 8.8.x
Comment #3
Coops_The proposed resolution has a problem that derivative images will have the wrong extension.
Attached is a patch which takes a different approach. It strips off the extra extension and checks whether that URI exists as the path for a managed file. This is very similar to how core checks for the existence of source images within the ImageStyleDownloadController::deliver method https://git.drupalcode.org/project/drupal/-/blob/9.0.x/core/modules/imag...
Comment #4
Coops_Comment #5
antongp commentedWith Drupal core >= 9.1.x you'll probably need to patch Drupal itself: #3223321: ImageStyleDownloadController::deliver() when checks for source image existence for a style converting extension uses derivative scheme instead of source
Attached updated patch reflecting latest
\Drupal\image\Controller\ImageStyleDownloadController::deliver()code. I haven't tested it on Drupal 8.9.x though...Comment #6
pstewart commentedI've just added a new patch to #3210163 which incorporates the fix from #5 implemented in a trait so that at can be used in multiple controller classes (which that other issue needs as it needs to subclass controllers from webp related modules), so that patch will also fix this issue too if committed.
Comment #7
agoradesign commented#5 works for me in combination with the mentioned core patch
Comment #8
gurbakshish commented#5 works fine for me, Thanks
Comment #9
dpagini commentedPatch form #5 is also working for me... I'm going to set this to RTBC.
I see a lot of talk here and in the linked issues about supporting multiple WEBP modules. Isn't that functionality in core now? I know I'm using the core webp "image_convert" plugin, and this patch is fixing it for me, and is relatively straight forward as well, that's why I chose to use this patch over some of the other open issues.
Comment #12
aimevpRe-rolled the patch for 2.x version of the module where it is working as well once applied.
*edit* So sorry: didn't notice the MR which makes my patch obsolete...
Comment #13
ransomweaver commentedI have rolled the code for this issue in to issue #3210163 with MR 22: https://git.drupalcode.org/project/remote_stream_wrapper/-/merge_request...
Comment #14
richardbporter commentedPatch #12 resolved the issue with remote_stream_wrapper 2.1.1 and Drupal 11.2.9
Comment #15
dpagini commentedRTBC for a year and a half now... is there any module maintainer who can chime in here? Is this approach something that can be contributed to this project? Or are there any issues that need to be addressed?
Comment #16
byrond commentedThe MR is failing one test. It looks like the patch is messing with the MIME type guessing:
Drupal\Tests\remote_stream_wrapper\Kernel\HttpMimeTypeGuesserTest::testHttpMimeTypeGuessing with data set #6 ('//www.drupal.org/test.unknown', 'html/head', GuzzleHttp\Psr7\Response Object (...), GuzzleHttp\Psr7\Response Object (...)) Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'html/head' +'application/octet-stream' /builds/issue/remote_stream_wrapper-3068898/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95 /builds/issue/remote_stream_wrapper-3068898/tests/src/Kernel/HttpMimeTypeGuesserTest.php:68 /builds/issue/remote_stream_wrapper-3068898/vendor/phpunit/phpunit/src/Framework/TestResult.php:729The feedback I have about the approach may address that. Since we have the image styles object, we can use it to determine whether a "Convert" effect is used and only then remove that specific extension (instead of indiscriminately removing any extension during the check). This new behavior will also likely need a test.
We need this patch, so I'll see what I can do to implement the change I described and see if that fixes the failing test as well.
Comment #17
byrond commentedI created a new MR with the refactored solution on the issue fork, but it isn't showing up here yet.
https://git.drupalcode.org/project/remote_stream_wrapper/-/merge_request...
Comment #18
byrond commentedSetting to Needs Review for the updated approach.
Comment #19
nagy.balint commentedComment #21
nagy.balint commentedI improved it a bit, and added tests.