Use full URl as HMAC cache key

[JimBobSquarePants]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

As previously identified using only the token as a cache key opens the potential for abuse of the caching mechanism. PR #256 was opened to remove that cache.

I would consider removing the cache with current target frameworks to be too expensive given the large overhead of HMAC generation so have replaced that PR with an implementation that uses the full encoded URL as the key.

The cache key generation method causes low allocations due to the underlying usage of string.Create so I would consider it acceptable for now. For .NET 6 we can likely remove the cache in it's entirety due to the much improved reusable hashing methods available to that framework.

[codecov]

Codecov Report

Merging #265 (414abfe) into main (80d1578) will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #265   +/-   ##
===================================
  Coverage    85%    85%           
===================================
  Files        75     75           
  Lines      2036   2036           
  Branches    298    298           
===================================
  Hits       1741   1741           
  Misses      211    211           
  Partials     84     84           

Flag	Coverage Δ
unittests	85% <100%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs	81% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80d1578...414abfe. Read the comment docs.

[ronaldbarendse]

One major downside compared to PR #256 (removing the HMAC cache), is that you can't skip the token validation based on request specific conditions (e.g. by returning null from OnComputeHMACAsync to indicate no check is required).

You could then use this to skip validation when a user is logged in:

options.OnComputeHMACAsync = (context, secret) =>
{
    if (context.Context.User.Identity.IsAuthenticated)
    {
        return Task.FromResult<string>(null);
    }

    string uri = CaseHandlingUriBuilder.BuildRelative(
            CaseHandlingUriBuilder.CaseHandling.LowerInvariant,
            context.Context.Request.PathBase,
            context.Context.Request.Path,
            QueryString.Create(context.Commands));

    return Task.FromResult(HMACUtilities.ComputeHMACSHA256(uri, secret));
};

[JimBobSquarePants]

HMAC validation for requests should always be separate from general user authorization. They really do two different things.

[ronaldbarendse]

What about providing a way to skip validation for existing commands/values that you've used? When you currently configure the HMAC secret, all existing image URLs will return 400 error codes...

And will this also work when you've added/removed a command in OnParseCommandsAsync? That command won't be in the URL that's used as cache key, but will be used to compute the HMAC token.

[JimBobSquarePants]

What about providing a way to skip validation for existing commands/values that you've used? When you currently configure the HMAC secret, all existing image URLs will return 400 error codes...

Which is absolutely the right thing to do. The HMAC system is all or nothing by design and always should be..

And will this also work when you've added/removed a command in OnParseCommandsAsync?

Yes. The token is calculated and compared before OnParseCommandsAsync.