kamsar / Dianoga

An automatic image optimizer for the Sitecore media library.
Other
104 stars 45 forks source link

Dianoga does not optimize next gen image formats with resized JSS media URLs #129

Open andrewlansdowne opened 1 year ago

andrewlansdowne commented 1 year ago

Version of Dianoga

6.1.0

Environment description

Sitecore 10.2 XP0 running locally with Headless Services 20.0.2 module installed, running a jss next.js headless site. Ensure you have some entries in the JSS media resizing whitelist (allowedMediaParams) e.g. mw=640.

What configs you have enabled

Dianoga.NextGenFormats.CDN.config Dianoga.Strategy.GetMediaStreamSync.config z.01.Dianoga.NextGenFormats.WebP.config z.02.Dianoga.NextGenFormats.Avif.config

Reproducible steps (1... 2... 3...) that cause the issue

  1. Upload a large JPEG that clearly requires optimisation anywhere in the media library
  2. Ensure that Dianoga is working with CDN (extension support) by testing in a normal media URL e.g. https://site.local/-/media/filename.ashx?extension=avif and https://site.local/-/media/filename.ashx?extension=webp
  3. Ensure that JSS media resizing is working by testing in a jssmedia URL e.g. https://site.local/-/media/filename.ashx?mw=640 - this should get both resized AND optimized into either avif or webp based on Accept headers
  4. Observe that JSS media resized with the extension querystring (required for CDN support) are NOT optimized into avif or webp when combined with resizing - https://site.local/-/media/filename.ashx?mw=640&extension=avif or https://site.local/-/media/filename.ashx?mw=640&extension=webp

What you expected to see, versus what you actually saw

I see an optimized resized jpeg, and in the Dianoga logs it says it optimized as a jpeg. I expect to see an optimized resized webp or avif file depending on the extension querystring.

Relevant logs

No errors, just the expected info logs saying the image was resized into jpeg. For example: 24172 00:22:37 INFO Dianoga: optimized /site/filename.jpg [original size: 1736039 bytes] [final size: 341112 bytes] [saved 1394927 bytes / 80.35%] [Optimized in 685ms] [Extension jpg]


Hi there, I've raised this issue to document the details of this issue as I am actively investigating and believe I have a fix for this which if it works I will submit a PR or at the least document it below. Thanks, Andy

andrewlansdowne commented 1 year ago

OK I think I've figured this out,

It's because the JSS image resizing whitelist blocks parsing of the querystring if ALL the querystring parameters from "protectedMediaQueryParameters" are not listed in the allowedMediaParams list (apart from rev which is ignored).

"extension" is added to the "protectedMediaQueryParameters" by Dianoga's webp config file and it seems to be required for the different images to be served/cached independently. So can't remove this.

Since it is an exact match of the combination of all the parameters, you can't realistically add all combinations of mw/mh/extension (perhaps you could if you only want webp since I think the comma used in extension=avif,webp would be an issue because allowedMediaParams uses commas as a separate)

I have therefore created a override of this class Sitecore.JavaScriptServices.Media.Pipelines.RestrictMediaRequest.ConfigurationWhitelistProcessor which ignores valid values of "extension" when checking against the resize whitelist. It still blocks if an arbitrary string is passed, which would enable the DOS attack vector the feature is intended to block.

public class ConfigurationWhitelistProcessor : Sitecore.JavaScriptServices.Media.Pipelines.RestrictMediaRequest.ConfigurationWhitelistProcessor
    {
        public ConfigurationWhitelistProcessor(BaseMediaManager mediaManager, WhitelistConfiguration whitelistConfiguration, BaseLog log, BaseSettings settings) : base(mediaManager, whitelistConfiguration, log, settings)
        {
        }

        /// <summary>
        /// Extension is added to the protectedMediaQueryParameters needed for each variation to be cached seperately.
        /// But it should be ignored by the JSS image resize whitelist since it isn't listed in allowedMediaParams
        /// as that would require every combination of mw/mh/extension to be added.
        /// </summary>
        /// <param name="queryString"></param>
        /// <returns></returns>
        protected override IDictionary<string, string> GetProtectedParams(NameValueCollection queryString)
        {
            var p = base.GetProtectedParams(queryString);
            if (p.ContainsKey("extension") && IsValidExtension(p["extension"]))
            {
                p.Remove("extension");
            }
            return p;
        }

        string[] validExtensions = new string[] { "webp", "avif" };

        private bool IsValidExtension(string extension)
        {
            if (!string.IsNullOrEmpty(extension))
            {
                var splitExt = extension.Split(',');
                if (splitExt.All(s => validExtensions.Contains(s)))
                {
                    return true;
                }
            }
            return false;
        }
    }

Ideally the list of valid extensions would come from the Dianoga enabled next-gen configs but I haven't figured out how to do that yet. The dianogaGetSupportedFormats isn't suitable as-is because you have to pass in the Accept header which defeats the point as I'd have to hardcode that so for now I'm just hardcoding the common next-gen extensions, that we are using.

This pipeline has to be replaced via config include:

<configuration xmlns:patch="http://www.sitecore.net/xmlconfig/" xmlns:role="http://www.sitecore.net/xmlconfig/role/" xmlns:set="http://www.sitecore.net/xmlconfig/set/">
  <sitecore>
    <pipelines>
      <group groupName="javaScriptServices">
        <pipelines>
          <restrictMediaRequest>
            <processor type="Sitecore.JavaScriptServices.Media.Pipelines.RestrictMediaRequest.ConfigurationWhitelistProcessor, Sitecore.JavaScriptServices.Media">
              <patch:delete />
            </processor>
            <processor type="Your.Optimisation.Media.ConfigurationWhitelistProcessor, Your.Optimisation" resolve="true" />
          </restrictMediaRequest>
        </pipelines>
      </group>
    </pipelines>
  </sitecore>
</configuration>
andrewlansdowne commented 1 year ago

Worth noting for completeness that if you want next-gen optimization of images when NOT passing any resize querystring then you need to override Sitecore.JavaScriptServices.Media.MediaRequestHandler and call Dianoga's AddCustomOptions like

using Dianoga.NextGenFormats;
using Sitecore.Resources.Media;
using System.Web;

public class JssMediaRequestHandler : Sitecore.JavaScriptServices.Media.MediaRequestHandler
    {
        protected override bool DoProcessRequest(HttpContext context, MediaRequest request, Sitecore.Resources.Media.Media media)
        {
            request.AddCustomOptions(new HttpContextWrapper(context));

            return base.DoProcessRequest(context, request, media);
        }
    }

Although thinking about it, if you are using CDN maybe don't want extensionless media requests to get converted to next-gen formats... Also I now need to figure out why the nextjs frontend isn't appending the extension querystring, probably another whitelist used by JSS media provider.

markgibbons25 commented 1 year ago

Do you have SXA installed? If so I'd suggest use the SXA MediaRequestHandler

andrewlansdowne commented 1 year ago

Hi, no I don’t. But thanks for the tip.

Sent from Outlook for iOShttps://aka.ms/o0ukef


From: Mark Gibbons @.> Sent: Wednesday, June 28, 2023 2:59:14 AM To: kamsar/Dianoga @.> Cc: Andrew Lansdowne @.>; Author @.> Subject: Re: [kamsar/Dianoga] Dianoga does not optimize next gen image formats with resized JSS media URLs (Issue #129)

Do you have SXA installed? If so I'd suggest use the SXA MediaRequestHandler

— Reply to this email directly, view it on GitHubhttps://github.com/kamsar/Dianoga/issues/129#issuecomment-1610507284, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A5NLROKCMQN4A4YG23PHBETXNOFXFANCNFSM6AAAAAAZHPVX24. You are receiving this because you authored the thread.Message ID: @.***>

kpicavet commented 1 year ago

We have exactly the same issue. We tried to implement the solution from above, but no luck. The 'jssmedia' webp image does not get resized although it works for example for a jpeg image.

What we also notice is that when we have a 'jssmedia' url we don't have extension and hash query params.

Any idea how we should approach this any further?

andrewlansdowne commented 1 year ago

@kpicavet jssmedia URLs intentionally don't use hash query param - instead it relies on the extension whitelist to determine valid requests - https://doc.sitecore.com/xp/en/developers/hd/20/sitecore-headless-development/media-handler.html

Firstly, try adding something like this to the whitelist: mw=640,extension=webp Then try browsing to a jssimage URL plus ?mw=640&extension=webp Check the dianoga logs to see what it says, in theory it should use webp if it would result in a smaller file than jpeg.

In hindsight, we only used webp in the end, so I could have avoided most of the custom code and just duplicated all the whitelist params to add extension=webp versions of each.

We also had to customise our frontend next.js app to append the extension=webp querystring (when browser supports it) because however we were generating them before, didn't add it..

If you report back the detail I may be able to offer further suggestions....

kpicavet commented 1 year ago

adding extension=webp queryparam seems to work! Now we need to figure out a way to add this queryparam to the url. We use angular as frontend & the build in image component from sitecore jss.

Anyway, thanks a lot already for the help.

andrewlansdowne commented 1 year ago

@kpicavet not sure if this will work for you, but in react we are using srcset on the Image and for each of our srcset options we specify both size ( mw:640 ) and extension:webp Since the browsers that support srcset also support webp this works great. The fallback img src is set to the original URL that renders a jpeg.

AlexandruPaun4Ness commented 11 months ago

adding extension=webp queryparam seems to work! Now we need to figure out a way to add this queryparam to the url. We use angular as frontend & the build in image component from sitecore jss.

Anyway, thanks a lot already for the help.

@kpicavet did you get to make this work with angular ?