[release/7.0] Prevent NRE when body is not set (#46613)

* Prevent NRE when body is not set

Fixes #46333

* Typo

* PR feedback

* PR feedback

---------

Co-authored-by: Sebastien Ros <[email protected]>

Author: github-actions[bot]

src/Middleware/OutputCaching/src/OutputCacheEntry.cs

@@ -20,12 +20,12 @@ internal sealed class OutputCacheEntry
     /// <summary>
     /// Gets the headers of the cache entry.
     /// </summary>
-    public HeaderDictionary Headers { get; set; } = default!;
+    public HeaderDictionary? Headers { get; set; }
 
     /// <summary>
     /// Gets the body of the cache entry.
     /// </summary>
-    public CachedResponseBody Body { get; set; } = default!;
+    public CachedResponseBody? Body { get; set; }
 
     /// <summary>
     /// Gets the tags of the cache entry.

src/Middleware/OutputCaching/src/OutputCacheMiddleware.cs

@@ -119,7 +119,7 @@ private async Task InvokeAwaited(HttpContext httpContext, IReadOnlyList<IOutputC
                 // Should we store the response to this request?
                 if (context.AllowCacheStorage)
                 {
-                    // It is also a pre-condition to reponse locking
+                    // It is also a pre-condition to response locking
 
                     var executed = false;
 
@@ -172,6 +172,14 @@ private async Task InvokeAwaited(HttpContext httpContext, IReadOnlyList<IOutputC
                             UnshimResponseStream(context);
                         }
 
+                        // If the policies prevented this response from being cached we can't reuse it for other
+                        // pending requests
+
+                        if (!context.AllowCacheStorage)
+                        {
+                            return null;
+                        }
+
                         return context.CachedResponse;
                     }
 
@@ -277,9 +285,13 @@ internal async Task<bool> TryServeCachedResponseAsync(OutputCacheContext context
                 var response = context.HttpContext.Response;
                 // Copy the cached status code and response headers
                 response.StatusCode = context.CachedResponse.StatusCode;
-                foreach (var header in context.CachedResponse.Headers)
+
+                if (context.CachedResponse.Headers != null)
                 {
-                    response.Headers[header.Key] = header.Value;
+                    foreach (var header in context.CachedResponse.Headers)
+                    {
+                        response.Headers[header.Key] = header.Value;
+                    }
                 }
 
                 // Note: int64 division truncates result and errors may be up to 1 second. This reduction in
@@ -289,7 +301,8 @@ internal async Task<bool> TryServeCachedResponseAsync(OutputCacheContext context
 
                 // Copy the cached response body
                 var body = context.CachedResponse.Body;
-                if (body.Length > 0)
+
+                if (body != null && body.Length > 0)
                 {
                     try
                     {
@@ -372,12 +385,13 @@ internal void FinalizeCacheHeaders(OutputCacheContext context)
             {
                 Created = context.ResponseTime!.Value,
                 StatusCode = response.StatusCode,
-                Headers = new HeaderDictionary(),
                 Tags = context.Tags.ToArray()
             };
 
             foreach (var header in headers)
             {
+                context.CachedResponse.Headers ??= new();
+
                 if (!string.Equals(header.Key, HeaderNames.Age, StringComparison.OrdinalIgnoreCase))
                 {
                     context.CachedResponse.Headers[header.Key] = header.Value;
@@ -402,6 +416,7 @@ internal async ValueTask FinalizeCacheBodyAsync(OutputCacheContext context)
 
             var contentLength = context.HttpContext.Response.ContentLength;
             var cachedResponseBody = context.OutputCacheStream.GetCachedResponseBody();
+
             if (!contentLength.HasValue || contentLength == cachedResponseBody.Length
                 || (cachedResponseBody.Length == 0
                     && HttpMethods.IsHead(context.HttpContext.Request.Method)))
@@ -410,6 +425,7 @@ internal async ValueTask FinalizeCacheBodyAsync(OutputCacheContext context)
                 // Add a content-length if required
                 if (!response.ContentLength.HasValue && StringValues.IsNullOrEmpty(response.Headers.TransferEncoding))
                 {
+                    context.CachedResponse.Headers ??= new();
                     context.CachedResponse.Headers.ContentLength = cachedResponseBody.Length;
                 }
 
@@ -508,13 +524,13 @@ internal bool ContentIsNotModified(OutputCacheContext context)
                 return true;
             }
 
-            if (!StringValues.IsNullOrEmpty(cachedResponseHeaders[HeaderNames.ETag])
+            if (cachedResponseHeaders != null && !StringValues.IsNullOrEmpty(cachedResponseHeaders[HeaderNames.ETag])
                 && EntityTagHeaderValue.TryParse(cachedResponseHeaders[HeaderNames.ETag].ToString(), out var eTag)
-                && EntityTagHeaderValue.TryParseList(ifNoneMatchHeader, out var ifNoneMatchEtags))
+                && EntityTagHeaderValue.TryParseList(ifNoneMatchHeader, out var ifNoneMatchETags))
             {
-                for (var i = 0; i < ifNoneMatchEtags?.Count; i++)
+                for (var i = 0; i < ifNoneMatchETags?.Count; i++)
                 {
-                    var requestETag = ifNoneMatchEtags[i];
+                    var requestETag = ifNoneMatchETags[i];
                     if (eTag.Compare(requestETag, useStrongComparison: false))
                     {
                         _logger.NotModifiedIfNoneMatchMatched(requestETag);
@@ -528,6 +544,11 @@ internal bool ContentIsNotModified(OutputCacheContext context)
             var ifModifiedSince = context.HttpContext.Request.Headers.IfModifiedSince;
             if (!StringValues.IsNullOrEmpty(ifModifiedSince))
             {
+                if (cachedResponseHeaders == null)
+                {
+                    return false;
+                }
+
                 if (!HeaderUtilities.TryParseDate(cachedResponseHeaders[HeaderNames.LastModified].ToString(), out var modified) &&
                     !HeaderUtilities.TryParseDate(cachedResponseHeaders[HeaderNames.Date].ToString(), out modified))
                 {

src/Middleware/OutputCaching/test/OutputCacheMiddlewareTests.cs

@@ -1,6 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Globalization;
+using System.Text;
+using System.Text.Unicode;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.OutputCaching.Memory;
@@ -150,7 +153,7 @@ public void ContentIsNotModified_NotConditionalRequest_False()
     }
 
     [Fact]
-    public void ContentIsNotModified_IfModifiedSince_FallsbackToDateHeader()
+    public void ContentIsNotModified_IfModifiedSince_FallsBackToDateHeader()
     {
         var utcNow = DateTimeOffset.UtcNow;
         var sink = new TestSink();
@@ -329,7 +332,7 @@ public void ContentIsNotModified_IfNoneMatch_MatchesAtLeastOneValue_True()
     }
 
     [Fact]
-    public void StartResponsegAsync_IfAllowResponseCaptureIsTrue_SetsResponseTime()
+    public void StartResponseAsync_IfAllowResponseCaptureIsTrue_SetsResponseTime()
     {
         var clock = new TestClock
         {
@@ -798,10 +801,8 @@ public async Task Locking_PreventsConcurrentRequests()
             // Wait for the second request to start before processing the first one
             task2Executing.Wait();
 
-            // Simluate some delay to allow for the second request to run while this one is pending
+            // Simulate some delay to allow for the second request to run while this one is pending
             await Task.Delay(500);
-
-            c.Response.Write("Hello" + responseCounter);
         });
 
         var context1 = TestUtils.CreateTestContext();
@@ -826,35 +827,96 @@ public async Task Locking_PreventsConcurrentRequests()
         Assert.Equal(1, responseCounter);
     }
 
+    [Fact]
+    public async Task Locking_IgnoresNonCacheableResponses()
+    {
+        var responseCounter = 0;
+
+        var blocker1 = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+        var blocker2 = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+
+        var memoryStream1 = new MemoryStream();
+        var memoryStream2 = new MemoryStream();
+
+        var options = new OutputCacheOptions();
+        options.AddBasePolicy(build => build.Cache());
+
+        var middleware = TestUtils.CreateTestMiddleware(options: options, next: async c =>
+        {
+            responseCounter++;
+
+            if (responseCounter == 1)
+            {
+                blocker1.SetResult(true);
+            }
+
+            c.Response.Cookies.Append("a", "b");
+            c.Response.Write("Hello" + responseCounter);
+
+            await blocker2.Task;
+        });
+
+        var context1 = TestUtils.CreateTestContext();
+        context1.HttpContext.Request.Method = "GET";
+        context1.HttpContext.Request.Path = "/";
+        context1.HttpContext.Response.Body = memoryStream1;
+
+        var context2 = TestUtils.CreateTestContext();
+        context2.HttpContext.Request.Method = "GET";
+        context2.HttpContext.Request.Path = "/";
+        context2.HttpContext.Response.Body = memoryStream2;
+
+        var task1 = Task.Run(() => middleware.Invoke(context1.HttpContext));
+
+        // Wait for context1 to be processed
+        await blocker1.Task;
+
+        // Start context2
+        var task2 = Task.Run(() => middleware.Invoke(context2.HttpContext));
+
+        // Wait for it to be blocked by the locking feature
+        await Task.Delay(500);
+
+        // Unblock context1
+        blocker2.SetResult(true);
+
+        await Task.WhenAll(task1, task2);
+
+        Assert.Equal(2, responseCounter);
+
+        // Ensure that even though two requests were processed, no result was returned from cache
+        Assert.Equal("Hello1", Encoding.UTF8.GetString(memoryStream1.ToArray()));
+        Assert.Equal("Hello2", Encoding.UTF8.GetString(memoryStream2.ToArray()));
+    }
+
     [Fact]
     public async Task Locking_ExecuteAllRequestsWhenDisabled()
     {
         var responseCounter = 0;
 
-        var task1Executing = new ManualResetEventSlim(false);
-        var task2Executing = new ManualResetEventSlim(false);
+        var blocker1 = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+        var blocker2 = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
 
         var options = new OutputCacheOptions();
         options.AddBasePolicy(build => build.Cache().SetLocking(false));
 
-        var middleware = TestUtils.CreateTestMiddleware(options: options, next: c =>
+        var middleware = TestUtils.CreateTestMiddleware(options: options, next: async c =>
         {
             responseCounter++;
 
             switch (responseCounter)
             {
                 case 1:
-                    task1Executing.Set();
-                    task2Executing.Wait();
+                    blocker1.SetResult(true);
+                    await blocker2.Task;
                     break;
                 case 2:
-                    task1Executing.Wait();
-                    task2Executing.Set();
+                    await blocker1.Task;
+                    blocker2.SetResult(true);
                     break;
             }
 
             c.Response.Write("Hello" + responseCounter);
-            return Task.CompletedTask;
         });
 
         var context1 = TestUtils.CreateTestContext();