Prevent SetDoNotCacheHeaders from overwriting valid cache headers (#28679)

* Prevent SetDoNotCacheHeaders from overwriting valid cache headers

* Applied review suggestions

src/Antiforgery/src/Internal/DefaultAntiforgery.cs

@@ -376,35 +376,42 @@ namespace Microsoft.AspNetCore.Antiforgery
         /// </summary>
         /// <param name="httpContext">The <see cref="HttpContext"/>.</param>
         protected virtual void SetDoNotCacheHeaders(HttpContext httpContext)
-        {
-            // Since antiforgery token generation is not very obvious to the end users (ex: MVC's form tag generates them
-            // by default), log a warning to let users know of the change in behavior to any cache headers they might
-            // have set explicitly.
-            LogCacheHeaderOverrideWarning(httpContext.Response);
-
-            httpContext.Response.Headers[HeaderNames.CacheControl] = "no-cache, no-store";
-            httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache";
-        }
-
-        private void LogCacheHeaderOverrideWarning(HttpResponse response)
         {
             var logWarning = false;
-            if (CacheControlHeaderValue.TryParse(response.Headers[HeaderNames.CacheControl].ToString(), out var cacheControlHeaderValue))
+            var responseHeaders = httpContext.Response.Headers;
+
+            if (responseHeaders.TryGetValue(HeaderNames.CacheControl, out var cacheControlHeader) &&
+                CacheControlHeaderValue.TryParse(cacheControlHeader.ToString(), out var cacheControlHeaderValue))
             {
-                if (!cacheControlHeaderValue.NoCache)
+                // If the Cache-Control is already set, override it only if required
+                if (!cacheControlHeaderValue.NoCache || !cacheControlHeaderValue.NoStore)
                 {
                     logWarning = true;
+                    responseHeaders[HeaderNames.CacheControl] = "no-cache, no-store";
                 }
             }
+            else
+            {
+                responseHeaders[HeaderNames.CacheControl] = "no-cache, no-store";
+            }
 
-            var pragmaHeader = response.Headers[HeaderNames.Pragma];
-            if (!logWarning
-                && !string.IsNullOrEmpty(pragmaHeader)
-                && !string.Equals(pragmaHeader, "no-cache", StringComparison.OrdinalIgnoreCase))
+            if (responseHeaders.TryGetValue(HeaderNames.Pragma, out var pragmaHeader) && pragmaHeader.Count > 0)
+            {
+                // If the Pragma is already set, override it only if required
+                if (!string.Equals(pragmaHeader[0], "no-cache", StringComparison.OrdinalIgnoreCase))
+                {
+                    logWarning = true;
+                    httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache";
+                }
+            }
+            else
             {
-                logWarning = true;
+                httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache";
             }
 
+            // Since antiforgery token generation is not very obvious to the end users (ex: MVC's form tag generates them
+            // by default), log a warning to let users know of the change in behavior to any cache headers they might
+            // have set explicitly.
             if (logWarning)
             {
                 _logger.ResponseCacheHeadersOverridenToNoCache();

src/Antiforgery/test/DefaultAntiforgeryTest.cs

@@ -348,6 +348,104 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal
             Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.Pragma]);
         }
 
+        private string GetAndStoreTokens_CacheHeadersArrangeAct(TestSink testSink, string headerName, string headerValue)
+        {
+            // Arrange
+            var loggerFactory = new Mock<ILoggerFactory>();
+            loggerFactory
+                .Setup(lf => lf.CreateLogger(typeof(DefaultAntiforgery).FullName!))
+                .Returns(new TestLogger("test logger", testSink, enabled: true));
+            var services = new ServiceCollection();
+            services.AddSingleton(loggerFactory.Object);
+            var antiforgeryFeature = new AntiforgeryFeature();
+            var context = CreateMockContext(
+                new AntiforgeryOptions(),
+                useOldCookie: true,
+                isOldCookieValid: true,
+                antiforgeryFeature: antiforgeryFeature);
+            context.HttpContext.RequestServices = services.BuildServiceProvider();
+            var antiforgery = GetAntiforgery(context);
+            context.HttpContext.Response.Headers[headerName] = headerValue;
+
+            // Act
+            antiforgery.GetAndStoreTokens(context.HttpContext);
+            return context.HttpContext.Response.Headers[headerName].ToString();
+        }
+
+        [Theory]
+        [InlineData("Cache-Control", "no-cache, no-store")]
+        [InlineData("Cache-Control", "NO-CACHE, NO-STORE")]
+        [InlineData("Cache-Control", "no-cache, no-store, private")]
+        [InlineData("Cache-Control", "NO-CACHE, NO-STORE, PRIVATE")]        
+        public void GetAndStoreTokens_DoesNotOverwriteCacheControlHeader(string headerName, string headerValue)
+        {
+            var testSink = new TestSink();
+            var actualHeaderValue = GetAndStoreTokens_CacheHeadersArrangeAct(testSink, headerName, headerValue);
+
+            // Assert
+            Assert.Equal(headerValue, actualHeaderValue);
+
+            var hasWarningMessage = testSink.Writes
+                .Where(wc => wc.LogLevel == LogLevel.Warning)
+                .Select(wc => wc.State?.ToString())
+                .Contains(ResponseCacheHeadersOverrideWarningMessage);
+            Assert.False(hasWarningMessage);
+        }
+
+        [Theory]
+        [InlineData("Cache-Control", "no-cache, private")]
+        [InlineData("Cache-Control", "NO-CACHE, PRIVATE")]
+        public void GetAndStoreTokens_OverwritesCacheControlHeader_IfNoStoreIsNotSet(string headerName, string headerValue)
+        {
+            var testSink = new TestSink();
+            var actualHeaderValue = GetAndStoreTokens_CacheHeadersArrangeAct(testSink, headerName, headerValue);
+
+            // Assert
+            Assert.NotEqual(headerValue, actualHeaderValue);
+
+            var hasWarningMessage = testSink.Writes
+                .Where(wc => wc.LogLevel == LogLevel.Warning)
+                .Select(wc => wc.State?.ToString())
+                .Contains(ResponseCacheHeadersOverrideWarningMessage);
+            Assert.True(hasWarningMessage);
+        }
+
+        [Theory]
+        [InlineData("Cache-Control", "no-store, private")]
+        [InlineData("Cache-Control", "NO-STORE, PRIVATE")]
+        public void GetAndStoreTokens_OverwritesCacheControlHeader_IfNoCacheIsNotSet(string headerName, string headerValue)
+        {
+            var testSink = new TestSink();
+            var actualHeaderValue = GetAndStoreTokens_CacheHeadersArrangeAct(testSink, headerName, headerValue);
+
+            // Assert
+            Assert.NotEqual(headerValue, actualHeaderValue);
+
+            var hasWarningMessage = testSink.Writes
+                .Where(wc => wc.LogLevel == LogLevel.Warning)
+                .Select(wc => wc.State?.ToString())
+                .Contains(ResponseCacheHeadersOverrideWarningMessage);
+            Assert.True(hasWarningMessage);
+        }
+
+        [Theory]
+        [InlineData("Pragma", "no-cache")]
+        [InlineData("Pragma", "NO-CACHE")]
+        public void GetAndStoreTokens_DoesNotOverwritePragmaHeader(string headerName, string headerValue)
+        {
+            var testSink = new TestSink();
+            var actualHeaderValue = GetAndStoreTokens_CacheHeadersArrangeAct(testSink, headerName, headerValue);
+
+            // Assert
+            Assert.Equal(headerValue, actualHeaderValue);
+
+            var hasWarningMessage = testSink.Writes
+                .Where(wc => wc.LogLevel == LogLevel.Warning)
+                .Select(wc => wc.State?.ToString())
+                .Contains(ResponseCacheHeadersOverrideWarningMessage);
+            Assert.False(hasWarningMessage);
+        }
+
         [Fact]
         public void GetAndStoreTokens_NoExistingCookieToken_Saved()
         {
@@ -1250,7 +1348,7 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal
         }
 
         [Theory]
-        [InlineData("Cache-Control", "no-cache")]
+        [InlineData("Cache-Control", "no-cache, no-store")]
         [InlineData("Pragma", "no-cache")]
         public void GetAndStoreTokens_DoesNotLogsWarning_ForNoCacheHeaders_AlreadyPresent(string headerName, string headerValue)
         {