From eeaa85b8d0ed10d4a793f9e0aee5df014dea786d Mon Sep 17 00:00:00 2001 From: Chris Ross Date: Fri, 26 Jun 2020 11:28:27 -0700 Subject: [PATCH] Do not cache 304 responses (#23367) * Do not cache 304 responses #23345 * More guids --- src/Servers/HttpSys/src/FeatureContext.cs | 7 ++++ .../FunctionalTests/ResponseCachingTests.cs | 34 +++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/Servers/HttpSys/src/FeatureContext.cs b/src/Servers/HttpSys/src/FeatureContext.cs index ab569e1bfa..4845013a6b 100644 --- a/src/Servers/HttpSys/src/FeatureContext.cs +++ b/src/Servers/HttpSys/src/FeatureContext.cs @@ -635,6 +635,13 @@ namespace Microsoft.AspNetCore.Server.HttpSys private static TimeSpan? GetCacheTtl(RequestContext requestContext) { var response = requestContext.Response; + // A 304 response is supposed to have the same headers as its associated 200 response, including Cache-Control, but the 304 response itself + // should not be cached. Otherwise Http.Sys will serve the 304 response to all requests without checking conditional headers like If-None-Match. + if (response.StatusCode == StatusCodes.Status304NotModified) + { + return null; + } + // Only consider kernel-mode caching if the Cache-Control response header is present. var cacheControlHeader = response.Headers[HeaderNames.CacheControl]; if (string.IsNullOrEmpty(cacheControlHeader)) diff --git a/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs b/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs index 1112420ed6..ed9fffebbb 100644 --- a/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs +++ b/src/Servers/HttpSys/test/FunctionalTests/ResponseCachingTests.cs @@ -80,11 +80,33 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests return httpContext.Response.Body.WriteAsync(new byte[10], 0, 10); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await SendRequestAsync(address)); Assert.Equal("2", await SendRequestAsync(address)); } } + [ConditionalFact] + public async Task Caching_304_NotCached() + { + var requestCount = 1; + using (Utilities.CreateHttpServer(out string address, httpContext => + { + // 304 responses are not themselves cachable. Their cache header mirrors the resource's original cache header. + httpContext.Response.StatusCode = StatusCodes.Status304NotModified; + httpContext.Response.ContentType = "some/thing"; // Http.Sys requires content-type for caching + httpContext.Response.Headers["x-request-count"] = (requestCount++).ToString(); + httpContext.Response.Headers["Cache-Control"] = "public, max-age=10"; + httpContext.Response.ContentLength = 10; + return httpContext.Response.Body.WriteAsync(new byte[10], 0, 10); + })) + { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. + Assert.Equal("1", await SendRequestAsync(address, StatusCodes.Status304NotModified)); + Assert.Equal("2", await SendRequestAsync(address, StatusCodes.Status304NotModified)); + } + } + [ConditionalFact] [QuarantinedTest("https://github.com/dotnet/aspnetcore-internal/issues/2207")] public async Task Caching_WithoutContentType_Cached_OnWin7AndWin2008R2() @@ -105,6 +127,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests return httpContext.Response.Body.WriteAsync(new byte[10], 0, 10); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await SendRequestAsync(address)); Assert.Equal("1", await SendRequestAsync(address)); } @@ -144,6 +167,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests return httpContext.Response.Body.WriteAsync(new byte[10], 0, 10); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await SendRequestAsync(address)); Assert.Equal("1", await SendRequestAsync(address)); } @@ -314,6 +338,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests return httpContext.Response.Body.WriteAsync(new byte[10], 0, 10); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await SendRequestAsync(address)); Assert.Equal("2", await SendRequestAsync(address)); } @@ -335,6 +360,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests Assert.Null(httpContext.Response.ContentLength); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await SendRequestAsync(address)); Assert.Equal("1", await SendRequestAsync(address)); } @@ -353,6 +379,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests await httpContext.Response.SendFileAsync(_absoluteFilePath, 0, null, CancellationToken.None); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await GetFileAsync(address)); Assert.Equal("2", await GetFileAsync(address)); } @@ -372,6 +399,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests await httpContext.Response.SendFileAsync(_absoluteFilePath, 0, null, CancellationToken.None); })) { + address += Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. Assert.Equal("1", await GetFileAsync(address)); Assert.Equal("1", await GetFileAsync(address)); } @@ -400,13 +428,15 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests switch (status) { case 206: // 206 (Partial Content) is not cached + case 304: // 304 (Not Modified) is not cached case 407: // 407 (Proxy Authentication Required) makes CoreCLR's HttpClient throw continue; } requestCount = 1; + var query = "?" + Guid.NewGuid().ToString(); // Avoid cache collisions for failed tests. try { - Assert.Equal("1", await SendRequestAsync(address + status, status)); + Assert.Equal("1", await SendRequestAsync(address + status + query, status)); } catch (Exception ex) { @@ -414,7 +444,7 @@ namespace Microsoft.AspNetCore.Server.HttpSys.FunctionalTests } try { - Assert.Equal("1", await SendRequestAsync(address + status, status)); + Assert.Equal("1", await SendRequestAsync(address + status + query, status)); } catch (Exception ex) {