umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
Other
4.49k stars 2.69k forks source link

Adding cache control headers stops image cropping from working. #12666

Closed bjarnef closed 1 year ago

bjarnef commented 2 years ago

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

10.0.1

Bug summary

Not sure if I am doing this wrong, but when I in Startup.cs use UseStaticFiles it seems images isn't cropped or scaled.

app.UseStaticFiles(new StaticFileOptions
{
    OnPrepareResponse = ctx =>
    {
        var headers = ctx.Context.Response.GetTypedHeaders();

        headers.CacheControl = new CacheControlHeaderValue
        {
            Public = true,
            MaxAge = TimeSpan.FromDays(365) // 1 year
        };

        headers.Expires = DateTime.UtcNow.AddYears(1);
    }
});

app.UseRewriter(new RewriteOptions().AddIISUrlRewrite(env.ContentRootFileProvider, "IISUrlRewrite.xml"));

I tried switching the order of UseStaticFiles and UseRewriter, but didn't seem to make a difference.

Strange thing is that I also have this in a v9.4.1 without issues.

app.UseStaticFiles(new StaticFileOptions
{
    OnPrepareResponse = ctx =>
    {
        var headers = ctx.Context.Response.GetTypedHeaders();

        headers.CacheControl = new CacheControlHeaderValue
        {
            Public = true,
            MaxAge = TimeSpan.FromDays(365) // 1 year
        };

        headers.Expires = DateTime.UtcNow.AddYears(1);
    }
}); 

Specifics

No response

Steps to reproduce

Add app.UseStaticFiles() to Startup.cs before app.UseUmbraco() and notice that images in frontend is no longer cropped/scaled.

You may need to clear browser cache after this change to see the effect.

Expected result / actual result

Images to stay cropped.

Without UseStaticFiles():

image

With UseStaticFiles():

image

bjarnef commented 2 years ago

@bergmania @ronaldbarendse I wonder if any of you know if something regarding this has changed from v9 to v10?

dawoe commented 2 years ago

This is also a issue with Umbraco 9.5.1

I worked around using this code which I found on the forum ( https://our.umbraco.com/forum/umbraco-9/107501-add-cache-control-header-for-static-files-starjs-starcss-starwoff2-etc#comment-334432)

public static void CacheStaticFiles(this IApplicationBuilder app) =>         
            app.Use(async (context, next) =>
            {
                string path = context.Request.Path;

                if (path.StartsWith("/umbraco/") == false)
                {
                    if (path.EndsWith(".css") || path.EndsWith(".js") || path.EndsWith(".svg") || path.EndsWith(".woff2"))
                    {
                        context.Response.Headers.Add("Cache-Control", "public, max-age=31536000");
                    }
                }

                await next();
            });
a-karandashov commented 2 years ago

@bjarnef You can set it up with ImageSharp configuration. here is the details: https://our.umbraco.com/Documentation/Reference/Configuration/ImagingSettings/ But I'm not sure what is the best way to handle SVG files, cuz you can't use cropping for SVG files.

If you're using crops and scaling, it's handled by ImageSharp, not directly by the Umbraco Media file provider.

 "Imaging": {
        "Cache": {
          "BrowserMaxAge": "365.00:00:00",
          "CacheMaxAge": "365.00:00:00",
          "CachedNameLength": 8,
          "CacheFolder": "~/umbraco/Data/TEMP/MediaCache"
        },
        "Resize": {
          "MaxWidth": 5000,
          "MaxHeight": 5000
        }
      },
bjarnef commented 2 years ago

@dawoe thanks for sharing.

I have added this version for now:

app.Use(async (context, next) =>
{
    string path = context.Request.Path;

    if (path.StartsWith("/umbraco/") == false)
    {
        if (path.EndsWith(".css") || path.EndsWith(".js") || path.EndsWith(".svg") || path.EndsWith(".woff2"))
        {
            var headers = context.Response.GetTypedHeaders();

            headers.CacheControl = new CacheControlHeaderValue
            {
                Public = true,
                MaxAge = TimeSpan.FromDays(365) // 1 year
            };

            headers.Expires = DateTime.UtcNow.AddYears(1);
        }
    }

    await next();
});
bjarnef commented 2 years ago

@AndreyKarandashovUKAD thanks. However it would only be relevant to images processed through ImageSharp. So not css, js, fonts, svg or imags not requested via GetCropUrl() or via URL parameters added to image path.

a-karandashov commented 2 years ago
            app.UseStaticFiles(new StaticFileOptions()
            {
                FileProvider = new PhysicalFileProvider(Path.Combine(env.WebRootPath, "scripts")),
                RequestPath = "/scripts",
                HttpsCompression = Microsoft.AspNetCore.Http.Features.HttpsCompressionMode.Compress,
                OnPrepareResponse = (context) =>
                {
                    string path = context.File.PhysicalPath;
                    if (path.EndsWith(".css") || path.EndsWith(".js") || path.EndsWith(".gif") || path.EndsWith(".jpg") || path.EndsWith(".png") || path.EndsWith(".svg"))
                    {
                        context.Context.Response.Headers.Append("Cache-Control", $"public, max-age={cacheMaxAgeOneWeek}");
                    }
                }
            });

For other file types I was using the code above, but it for v10. Works fine :)

nul800sebastiaan commented 2 years ago

Thanks for the discussion all! Do we agree that this is something that we should add to the documentation? I don't think there anything Umbraco itself can do to "fix" the problem other than documenting how to enable UseStaticFiles properly?

kinless commented 2 years ago

Just here to report that the solution from @bjarnef is what worked for me. If I tried to put app.UseStaticFiles() after app.UseUmbraco() then the cache methods inside UseStaticFiles() were completely ignored. Placing the conditionals in .Use() instead is what fixed it. I also added "png" "gif" and "jpg" to the conditionals, which appear to still work OK as well as local crops. (I'm assuming because path.EndsWith() is looking for the file extension but sees query strings instead, so it happily ignores those while ImageSharp takes over caching for the files that fall under its jurisdiction.)

There definitely should be some documentation detailing how to properly cache static files in tandem with using ImageSharp.

nul800sebastiaan commented 2 years ago

Thanks @kinless for adding the additional information, do you have a more complete code sample we could see to make it super easy for the docs team to add some information please? 🙏

kinless commented 2 years ago

So it looks as if app.UseStaticFiles() should not be utilized at all. Here's what I have in the Use() StartUp.cs file before app.UseUmbraco() (removed Health Check headers for sake of clarity). Sort of a mish-mash between what @bjarnef and @dawoe came up with.

app.Use(async (context, next) =>
    {
        string path = context.Request.Path;
        if (path.StartsWith("/umbraco/") == false)
        {
             if (path.EndsWith(".css") || path.EndsWith(".js") || path.EndsWith(".svg") || path.EndsWith(".woff2") || path.EndsWith(".woff") || path.EndsWith(".gif") || path.EndsWith(".png") || path.EndsWith(".jpg") || path.EndsWith(".webp") || path.EndsWith(".ico"))
             {
                 context.Response.Headers.Add("Cache-Control", "public, max-age=31536000");
             }
        }
        await next();
    });
  1. I'm unsure if having the /umbraco/ path check is necessary, or if the backoffice files have their own caching in place and are already excluded. @nul800sebastiaan you'd probably know better than us.
  2. I wonder if there's a more elegant/efficient way to do this check, maybe utilizing a string array instead? (var extensions = {".css", ".js", ".svg", ... and then iterate with some kind of path.Any() Linx command?) Using the multiple conditionals method, though effective, seems a little archaic to me...
jpinto3488 commented 2 years ago

The given solution to use app.Use(...) is not working on Smidge generated URls, which is automatically set to max-age=864000 Anyone found a solution that works with smidge too?

nul800sebastiaan commented 2 years ago

Thanks again all, I've narrowed down the examples a bit to the following:

        app.Use(async (context, next) =>
        {
            string path = context.Request.Path;
            var fileExtenstion = path.GetFileExtension();
            string[] ignoredExtensions = {".css", ".js", ".svg", ".woff2", ".gif", ".png", ".jpg", ".webp", ".ico"};
            var globalSettings = _config.GetSection(Umbraco.Cms.Core.Constants.Configuration.ConfigGlobal).Get<GlobalSettings>();

            var currentPathIsUmbracoPath = path.StartsWith(globalSettings.UmbracoPath.Replace("~", string.Empty));
            if (currentPathIsUmbracoPath == false && Array.IndexOf(ignoredExtensions, fileExtenstion) == -1) // -1 indicates not found in the array
            {
                context.Response.Headers.Add("Cache-Control", "public, max-age=31536000");
            }

            await next();
        });

I am tempted to add a check if(path == "/") { // do nothing } or even better if(fileExtension == string.Empty) { // do nothing }, does that seem right? You'd want to cache static files, not the HMTL output of each page.. right?

The given solution to use app.Use(...) is not working on Smidge generated URls, which is automatically set to max-age=864000 Anyone found a solution that works with smidge too?

Sounds like a default from Smidge, but you don't have to use our runtime minification, you can use "raw" Smidge I guess: https://github.com/Shazwazza/Smidge/wiki/Declarations#pre-defined-bundles

ronaldbarendse commented 2 years ago

@bjarnef Umbraco already adds the static file middleware (in the correct order), so you shouldn't add it again by calling UseStaticFiles() yourself.

Because the default overload uses the configured StaticFileOptions from the DI container, you can configure that instead, see: https://github.com/umbraco/Umbraco-CMS/pull/11783#issuecomment-1005739307. As mentioned in that comment, images processed by ImageSharp will use a different middleware that can be configured similarly.

bjarnef commented 2 years ago

@ronaldbarendse thanks for the elaboration.

Is this documented anywhere? This only thing I could find is StaticFileOptions mentioned here: https://our.umbraco.com/Documentation/Reference/Configuration/filesystemProviders

It would be great with an example of best practice to set Cache-Control header.

In my updated version here https://github.com/umbraco/Umbraco-CMS/issues/12666#issuecomment-1177289828 I didn't call UseStaticFiles() method though.

ronaldbarendse commented 2 years ago

This isn't really related to the file system providers, as it's about serving files and not how to access/store them.

Since version 9.3.0 (which includes PR https://github.com/umbraco/Umbraco-CMS/pull/11783), we've ensured all static files are available via the WebRootFileProvider that's used by the default static file options/middleware, so this works the same as a generic ASP.NET Core application (although you need to configure the options separately).

So if you only want to set the Cache-Control headers on static files, you can use this composer:

using Microsoft.Net.Http.Headers;
using Umbraco.Cms.Core.Composing;

public class StaticFilesComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.Services.Configure<StaticFileOptions>(options => options.OnPrepareResponse = ctx =>
        {
            // Exclude Umbraco static assets (you should use GlobalSettings.UmbracoPath instead in case you've changed this)
            if (ctx.Context.Request.Path.StartsWithSegments("/umbraco"))
            {
                return;
            }

            // Set headers for specific file names/extensions
            if (ctx.File.Name.InvariantEndsWith(".css") ||
                ctx.File.Name.InvariantEndsWith(".js") ||
                ctx.File.Name.InvariantEndsWith(".svg") ||
                ctx.File.Name.InvariantEndsWith(".woff2"))
            {
                var headers = ctx.Context.Response.GetTypedHeaders();

                // Update or set Cache-Control header
                var cacheControl = headers.CacheControl ?? new CacheControlHeaderValue();
                cacheControl.Public = true;
                cacheControl.MaxAge = TimeSpan.FromDays(365);
                headers.CacheControl = cacheControl;
            }
        });
    }
}
bjarnef commented 2 years ago

Yes, it would be great to document this as StaticFilesComposer doesn't return any results on Our and only documentation regarding file system providers mention StaticFileOptions.

ronaldbarendse commented 2 years ago

Yes, it would be great to document this as StaticFilesComposer doesn't return any results on Our and only documentation regarding file system providers mention StaticFileOptions.

And that part of the documentation is actually obsolete (see the note at the top), as you can now change the physical media file path by changing the UmbracoMediaPhysicalRootPath setting 😇

Regardless, I agree that everyone would benefit if we had a complete overview of how to correctly configure response headers, either static files or images processed by ImageSharp: maybe we should add a 'Configure response headers' tutorial?

bjarnef commented 2 years ago

@ronaldbarendse I think that tutorial would be useful to many.

Do you have an example of how to access GlobalSettings in the composer context? Furthermore your example before was missing a few namespaces.

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Net.Http.Headers;
using System;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Extensions;

I think we may also want to set Expires

Maybe there a better/cleaner way to write this with this with several file extensions:

// Set headers for specific file names/extensions
if (ctx.File.Name.InvariantEndsWith(".css") ||
    ctx.File.Name.InvariantEndsWith(".js") ||
    ctx.File.Name.InvariantEndsWith(".svg") ||
    ctx.File.Name.InvariantEndsWith(".woff2"))
{

}

Something like this?

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Net.Http.Headers;
using System;
using System.Collections.Generic;
using System.Linq;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Extensions;

namespace MyProject.Core
{
    public class StaticFilesComposer : IComposer
    {
        private static readonly HashSet<string> extensions = new HashSet<string>()
        {
            "css",
            "js",
            "svg",
            "woff2"
        };

        public void Compose(IUmbracoBuilder builder)
        {
            builder.Services.Configure<StaticFileOptions>(options => options.OnPrepareResponse = ctx =>
            {
                // Exclude Umbraco static assets (you should use GlobalSettings.UmbracoPath instead in case you've changed this)
                if (ctx.Context.Request.Path.StartsWithSegments("/umbraco"))
                {
                    return;
                }

                // Set headers for specific file names/extensions
                if (extensions.Any(x => ctx.File.Name.InvariantEndsWith($".{x}")))
                {
                    var headers = ctx.Context.Response.GetTypedHeaders();

                    // Update or set Cache-Control header
                    var cacheControl = headers.CacheControl ?? new CacheControlHeaderValue();
                    cacheControl.Public = true;
                    cacheControl.MaxAge = TimeSpan.FromDays(365); // 1 year
                    headers.CacheControl = cacheControl;
                }
            });
        }
    }
}
ronaldbarendse commented 2 years ago

Do you have an example of how to access GlobalSettings in the composer context?

You can use a slightly different configure overload (exposed on the OptionsBuilder<T>) or register a type that implements IConfigureOptions<StaticFileOptions>, see the Microsoft docs.

Furthermore your example before was missing a few namespaces.

Not if you're using the implicit global usings in v10 😇 But we should add those in sample code indeed!

I think we may also want to set Expires

Setting Cache-Control: max-age= will cause the Expires header to be ignored, so you don't need to bother setting it, see the MDN docs.

Maybe there a better/cleaner way to write this with this with several file extensions:

I thought the same, but wanted to keep the sample as simple as possible 😄


To close this off with a complete example (which you're free to copy into a new tutorial page if you're up for creating a PR for that 😇), I came up with this:

using System;
using System.Collections.Generic;
using System.IO;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using Microsoft.Net.Http.Headers;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Hosting;
using Umbraco.Extensions;

namespace UmbracoProject
{
    public class StaticFilesComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
            => builder.Services.AddTransient<IConfigureOptions<StaticFileOptions>, ConfigureStaticFileOptions>();

        private class ConfigureStaticFileOptions : IConfigureOptions<StaticFileOptions>
        {
            private readonly string _umbracoPath;
            private static readonly HashSet<string> _fileExtensions = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
            {
                ".ico",
                ".css",
                ".js",
                ".svg",
                ".woff2"
            };

            public ConfigureStaticFileOptions(IOptions<GlobalSettings> globalSettings, IHostingEnvironment hostingEnvironment)
                => _umbracoPath = hostingEnvironment.ToAbsolute(globalSettings.Value.UmbracoPath);

            public void Configure(StaticFileOptions options)
                => options.OnPrepareResponse = ctx =>
                {
                    // Exclude Umbraco backoffice assets
                    if (ctx.Context.Request.Path.StartsWithSegments(_umbracoPath))
                    {
                        return;
                    }

                    // Set headers for specific file extensions
                    var fileExtension = Path.GetExtension(ctx.File.Name);
                    if (_fileExtensions.Contains(fileExtension))
                    {
                        var headers = ctx.Context.Response.GetTypedHeaders();

                        // Update or set Cache-Control header
                        var cacheControl = headers.CacheControl ?? new CacheControlHeaderValue();
                        cacheControl.Public = true;
                        cacheControl.MaxAge = TimeSpan.FromDays(365);
                        headers.CacheControl = cacheControl;
                    }
                };
        }
    }
}

And for setting Cache-Control max-age header for images processed by the ImageSharp middleware, you can set the Umbraco:CMS:Imaging:Cache:BrowserMaxAge setting. You can also configure the ImageSharpMiddlewareOptions in the same way as above, but if you overwrite the existing OnPrepareResponseAsync callback, make sure to either capture and call the existing callback or copy the code from Umbraco: https://github.com/umbraco/Umbraco-CMS/blob/c576bbea03875088685655bda20bf6bae8649207/src/Umbraco.Web.Common/DependencyInjection/ConfigureImageSharpMiddlewareOptions.cs#L69-L85

bjarnef commented 2 years ago

@ronaldbarendse thanks, I will try that out.

Would it be better to use GlobalSettings in a private variable in case one want to access further configuration options? E.g. _globalSettings.ReservedPaths. Maybe one don't want to cache any assets under ~/hangfire or similar path.

Does it need to use ToAbsolute()? Previously it was a relative path and since it use StartsWithSegment().

private class ConfigureStaticFileOptions : IConfigureOptions<StaticFileOptions>
{
        private readonly IHostingEnvironment _hostingEnvironment;
        private readonly GlobalSettings _globalSettings;
        private static readonly HashSet<string> _fileExtensions = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
        {
            ".ico",
            ".css",
            ".js",
            ".svg",
            ".woff2"
        };

        public ConfigureStaticFileOptions(IOptions<GlobalSettings> globalSettings, IHostingEnvironment hostingEnvironment)
        {
            _globalSettings = globalSettings.Value;
            _hostingEnvironment = hostingEnvironment;
        }

        public void Configure(StaticFileOptions options)
                => options.OnPrepareResponse = ctx =>
                {
                    string umbracoPath = _hostingEnvironment.ToAbsolute(_globalSettings.UmbracoPath);

                    // Exclude Umbraco backoffice assets
                    if (ctx.Context.Request.Path.StartsWithSegments(umbracoPath))
                    {
                        return;
                    }
               }
}
bjarnef commented 2 years ago

It seems we can use the GetBackOfficePath() extension method in GlobalSettingsExtensions

string backOfficePath = _globalSettings.GetBackOfficePath(_hostingEnvironment);

which also seems to use hostingEnvironment.ToAbsolute(globalSettings.UmbracoPath);

ronaldbarendse commented 2 years ago

@bjarnef Those are all valid points and if you're keeping a reference to the settings, you can also use IOptionsMonitor<GlobalSettings> to always get the latest, updated values (although changing the UmbracoPath requires a restart anyway, but other settings might not).

The GetBackOfficePath() is indeed similar to calling ToAbsolute() yourself, but that does cache the resolved value in a static field (which might also be the reason for requiring a restart).

Keep in mind that this only affects static assets, so images processed by ImageSharp, Smidge bundles or any other generated response (from e.g. API controllers) aren't handled by the static file middleware. This is different to the generic middleware that's added by app.Use(async (context, next) => {...}) and processes all requests/responses... Maybe all those checks aren't needed at all?

kjac commented 1 year ago

For future reference, the tutorial that was the outcome of this great discussion can be found here: https://docs.umbraco.com/umbraco-cms/reference/response-caching

@bjarnef I'm going to close this issue now, as I suspect it's very much handled by the tutorial. Feel free to reopen the issue if more needs doing here.