Contrib.social

First suppressed image style on a page wrongly suppresses all subsequent image styles

Open on Drupal.org →
Created on 26 May 2023, over 1 year ago
Updated 3 June 2023, over 1 year ago

Problem/Motivation

The first image style rendered on a page that sets itok supression to TRUE then causes the url of any subsequent image style for which insecure derivatives are not allowed (ergo regular image styles) rendered in the same request also be output without an itok.

If the derivative files for these other image styles have not yet been generated (e.g. after creating new content), due to the itok missing fro their url those derivatives will NOT be generated. Previously generated derivative files will still be accessible, which makes this bug hard to spot.

Drupal\image_derivative_token\Entity\ImageStyle::buildUrl() sets an override for 'image.settings.suppress_itok_output' the first time it encounters an itok-supressed image style and the override remain for the remainder of the request and affects subsequent image styles too.

Steps to reproduce

(In my case I used the supressed derivative in a metatag, so the following is theoretical, but should produce the problem)
Create some content type, with two or more image fields displayed with different image styles.

Check the "Allow insecure derivatives" and "Suppress image token output" options for the image style used in the first field.

Do NOT set the "Allow insecure derivatives" option on the image style used in any subsequent fields.

Do NOT use any image styles with the above options enabled as the image field thumbnail/preview style, otherwise the derivative might be generated during upload and the bug might not be visible.

Create new content with both (or more) image fields filled.

After submission the first image derivative should be visible, but the second should be broken and not generated due to the missing itok.

Proposed resolution

Fix the logic for setting and RESTORING 'image.settings.suppress_itok_output' override in Drupal\image_derivative_token\Entity\ImageStyle::buildUrl()

1. save current value
2. set override
3. get $url from parent::buildUrl
4. restore original value
5. return $url

Remaining tasks

Create the generalized logic for setting / unsetting / restoring the override for itok supression

1. save current value - I'm not familiar enough with the Config override system to determine how one can get the current value, which I think is not the same as the orignal value (Config::getOriginal())

4. restore saved value

User interface changes

None

API changes

None

Data model changes

None

🐛 Bug report
Status

Needs review

Version

1.1

Component

Code

Created by

🇭🇺Hungary karol haltenberger

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @karol haltenberger
  • Comment over 1 year ago →
    🇭🇺Hungary karol haltenberger

    I have a basic patch (I will try to create an issue fork) which fixes the problem for the very simple case of ON for styles with the option, OFF otherwise, but a more generic solution should be considered.

    I might come back around eventually and do it, but I'm super busy for the foreseeable future, so don't hold your breath.

    ---

    Nice module BTW - exactly what I needed for generating images of limited size for social share metatags (FB does not allow images over 8MB) - I's even say that it should be in core.

  • Issue was unassigned.
  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇭🇺Hungary karol haltenberger
  • Comment over 1 year ago →
    🇺🇸United States rlhawk Seattle, Washington, United States

    Thanks for this. It makes perfect sense that the configuration override would persist. I think we should move away from overriding in this instance, especially since the method involves replacing the Image Style class. We can alter the URL to remove the itok query parameter by implementing hook_file_url_alter(), which seems like a better way to go.

  • Comment over 1 year ago →
    🇺🇸United States rlhawk Seattle, Washington, United States
  • @rlhawk opened merge request.
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States rlhawk Seattle, Washington, United States

    I've updated the merge request with the proposed change to the way the itok query parameter is suppressed.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024