Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
andrewlansdowne opened this issue Jun 15, 2023 · 9 comments

Comments

@andrewlansdowne
Copy link

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
Copy link
Author

andrewlansdowne commented Jun 15, 2023

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
Copy link
Author

andrewlansdowne commented Jun 15, 2023

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
Copy link
Collaborator

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

@andrewlansdowne
Copy link
Author

andrewlansdowne commented Jun 28, 2023 via email

@kpicavet
Copy link

kpicavet commented Oct 3, 2023

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
Copy link
Author

@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
Copy link

kpicavet commented Oct 4, 2023

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
Copy link
Author

@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
Copy link

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants