ResponseCache sometimes reorders [FromQuery] array / List parameters depending on the Cache-Control header

[marcelltoth]

Describe the bug

I have an endpoint where I pass multiple int parameters, like: https://some.origin/testEndpoint?ids=21&ids=31&ids=1 I applied a ResponseCacheAttribute to it.

For some reason if the agent calls with Cache-Control: max-age=0 (chrome does so after a regular F5) my action method gets called, but the argument array contains the arguments in a different order.

Calling with Cache-Control: no-cache (CTRL-F5 in Chrome) does not cause the problem to appear.

To Reproduce

Steps to reproduce the behavior:

1. Take an ASP.NET Core 2.2 MVC application (no ApiController attribute). May affect other versions / with ApiController attribute, not tested.

2. Add this action method to a controller:

   [HttpGet("testEndpoint")]
   [ResponseCache(Location = ResponseCacheLocation.Any, VaryByQueryKeys = new[] { "*" }, Duration = 3600)]
   public IActionResult TestEndpoint([FromQuery] int[] ids)
   {
    return Ok(ids);
   }

3. Call it with CURL with no-cache: curl 'http://some-endpoint/testEndpoint?ids=21&ids=3&ids=25' -H 'Cache-Control: no-cache'

4. See the correct output: [21,3,25]

5. Now call it with max-age: curl 'http://some-endpoint/testEndpoint?ids=21&ids=3&ids=25' -H 'Cache-Control: max-age=0'

Calling without either header seems to produce unstable results, to me it looks like it produces the output of the previous call.

The problem is exactly the same if I use List<int> instead of the array.

Expected behavior

The received order of arguments always matches the order they appear in the query string, regardless of the Cache-Control headers sent.

Additional context

I am running my example in a docker container based on the microsoft/dotnet:2.2-aspnetcore-runtime image.

❯ docker exec 858e40046859 dotnet --info Host (useful for support): Version: 2.2.1 Commit: 878dd11e62

.NET Core SDKs installed: No SDKs were found. .NET Core runtimes installed: Microsoft.AspNetCore.All 2.2.1 [/usr/share/dotnet/shared/Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.2.1 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.2.1 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

[marcelltoth]

Okay, so I think I figured out what's going on here, and it is a nasty one:

Look at these innocent looking lines from ResponseCachingMiddleware's ResponseCachingKeyProvider.cs

var queryKeyValues = context.HttpContext.Request.Query[queryKey]; <-- actual type is StringValues
var queryValueArray = queryKeyValues.ToArray();
Array.Sort(queryValueArray, StringComparer.Ordinal);

Looks all safe and fun, StringValues is an IEnumerable<string> so what mess can one make by copying it into a new array and playing with it, right?

Except that we are not calling the Enumerable.ToArray extension method here!, as the author assumed. It happens that StringValues provides its own ToArray which shadows the extension method and contrary to the convention does not copy the values but works like an AsArray method would, if there are multiple strings in the StringValues it returns the original array! Which does cause pain when we order it afterwards.

I really feel like this is a bug on the Extensions side. I can't blame any .NET developer who assumes that a ToArray method performs a copy. Especially on an IEnumerable.

I am going to file a pull request for that repo and link it here.

Simple workarounds would be:

1. casting queryKeyValues to IEnumerable<string>
2. calling the extension method statically
3. using a List

but I wouldn't really go down either route unless absolutely necessary, I believe we need to address the core problem first.

[analogrelay]

Closing in lieu of aspnet/Extensions#1813 then. Further discussion can move there. Thanks!

[davidfowl]

cc @mkArtakMSFT @pranavkm we need to fix this in MVC

[javiercn]

@davidfowl why is this MVC and not BasicMiddleware or other area?

[davidfowl]

If you can find instances where we mutate them array returned, then we should fix those too

[davidfowl]

Ah I missed that the change is in response caching