diff --git a/src/Microsoft.AspNetCore.Antiforgery/IAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/IAntiforgery.cs index 6ac2e44..d6dd05a 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/IAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/IAntiforgery.cs @@ -14,7 +14,8 @@ public interface IAntiforgery { /// /// Generates an for this request and stores the cookie token - /// in the response. + /// in the response. This operation also sets the "Cache-control" and "Pragma" headers to "no-cache" and + /// the "X-Frame-Options" header to "SAMEORIGIN". /// /// The associated with the current request. /// An with tokens for the response. diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/AntiforgeryLoggerExtensions.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/AntiforgeryLoggerExtensions.cs index 5e115bb..7dd4306 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/AntiforgeryLoggerExtensions.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/AntiforgeryLoggerExtensions.cs @@ -15,6 +15,7 @@ internal static class AntiforgeryLoggerExtensions private static readonly Action _newCookieToken; private static readonly Action _reusedCookieToken; private static readonly Action _tokenDeserializeException; + private static readonly Action _responseCacheHeadersOverridenToNoCache; static AntiforgeryLoggerExtensions() { @@ -47,6 +48,11 @@ static AntiforgeryLoggerExtensions() LogLevel.Error, 7, "An exception was thrown while deserializing the token."); + _responseCacheHeadersOverridenToNoCache = LoggerMessage.Define( + LogLevel.Warning, + 8, + "The 'Cache-Control' and 'Pragma' headers have been overridden and set to 'no-cache' to prevent " + + "caching of this response. Any response that uses antiforgery should not be cached."); } public static void ValidationFailed(this ILogger logger, string message) @@ -83,5 +89,10 @@ public static void TokenDeserializeException(this ILogger logger, Exception exce { _tokenDeserializeException(logger, exception); } + + public static void ResponseCacheHeadersOverridenToNoCache(this ILogger logger) + { + _responseCacheHeadersOverridenToNoCache(logger, null); + } } } diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs index eeb92cf..755d8ae 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs @@ -7,6 +7,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Microsoft.Net.Http.Headers; namespace Microsoft.AspNetCore.Antiforgery.Internal { @@ -66,6 +67,10 @@ public AntiforgeryTokenSet GetAndStoreTokens(HttpContext httpContext) } } + // Explicitly set the cache headers to 'no-cache'. This could override any user set value but this is fine + // as a response with antiforgery token must never be cached. + SetDoNotCacheHeaders(httpContext); + return tokenSet; } @@ -237,6 +242,10 @@ public void SetCookieTokenAndHeader(HttpContext httpContext) { _logger.ReusedCookieToken(); } + + // Explicitly set the cache headers to 'no-cache'. This could override any user set value but this is fine + // as a response with antiforgery token must never be cached. + SetDoNotCacheHeaders(httpContext); } private void SaveCookieTokenAndHeader(HttpContext httpContext, string cookieToken) @@ -358,6 +367,43 @@ private IAntiforgeryFeature GetTokensInternal(HttpContext httpContext) return antiforgeryFeature; } + private void SetDoNotCacheHeaders(HttpContext httpContext) + { + // Since antifogery 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"; + httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; + } + + private void LogCacheHeaderOverrideWarning(HttpResponse response) + { + var logWarning = false; + CacheControlHeaderValue cacheControlHeaderValue; + if (CacheControlHeaderValue.TryParse(response.Headers[HeaderNames.CacheControl], out cacheControlHeaderValue)) + { + if (!cacheControlHeaderValue.NoCache) + { + logWarning = true; + } + } + + var pragmaHeader = response.Headers[HeaderNames.Pragma]; + if (!logWarning + && !string.IsNullOrEmpty(pragmaHeader) + && string.Compare(pragmaHeader, "no-cache", ignoreCase: true) != 0) + { + logWarning = true; + } + + if (logWarning) + { + _logger.ResponseCacheHeadersOverridenToNoCache(); + } + } + private AntiforgeryTokenSet Serialize(IAntiforgeryFeature antiforgeryFeature) { // Should only be called after new tokens have been generated. diff --git a/src/Microsoft.AspNetCore.Antiforgery/project.json b/src/Microsoft.AspNetCore.Antiforgery/project.json index 259531c..5c9b560 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/project.json +++ b/src/Microsoft.AspNetCore.Antiforgery/project.json @@ -22,6 +22,7 @@ "dependencies": { "Microsoft.AspNetCore.DataProtection": "1.1.0-*", "Microsoft.AspNetCore.Http.Abstractions": "1.1.0-*", + "Microsoft.AspNetCore.Http.Extensions": "1.1.0-*", "Microsoft.AspNetCore.WebUtilities": "1.1.0-*", "Microsoft.Extensions.ObjectPool": "1.1.0-*", "NETStandard.Library": "1.6.1-*" diff --git a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs index 09de924..943eb94 100644 --- a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs +++ b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs @@ -2,12 +2,15 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Linq; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Testing; using Microsoft.Extensions.Options; +using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -15,6 +18,10 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal { public class DefaultAntiforgeryTest { + private const string ResponseCacheHeadersOverrideWarningMessage = + "The 'Cache-Control' and 'Pragma' headers have been overridden and set to 'no-cache' to prevent caching " + + "of this response. Any response that uses antiforgery should not be cached."; + [Fact] public async Task ChecksSSL_ValidateRequestAsync_Throws() { @@ -275,6 +282,67 @@ public void GetAndStoreTokens_ExistingValidCookieToken_NotOverriden() Assert.Equal(context.TestTokenSet.FormTokenString, antiforgeryFeature.NewRequestTokenString); } + [Fact] + public void GetAndStoreTokens_ExistingValidCookieToken_NotOverriden_AndSetsDoNotCacheHeaders() + { + // Arrange + var antiforgeryFeature = new AntiforgeryFeature(); + var context = CreateMockContext( + new AntiforgeryOptions(), + useOldCookie: true, + isOldCookieValid: true, + antiforgeryFeature: antiforgeryFeature); + var antiforgery = GetAntiforgery(context); + + // Act + var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); + + // Assert + // We shouldn't have saved the cookie because it already existed. + context.TokenStore.Verify( + t => t.SaveCookieToken(It.IsAny(), It.IsAny()), + Times.Never); + + Assert.Null(tokenSet.CookieToken); + Assert.Equal(context.TestTokenSet.FormTokenString, tokenSet.RequestToken); + + Assert.NotNull(antiforgeryFeature); + Assert.Equal(context.TestTokenSet.OldCookieToken, antiforgeryFeature.CookieToken); + Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.CacheControl]); + Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.Pragma]); + } + + [Fact] + public void GetAndStoreTokens_ExistingCachingHeaders_Overriden() + { + // Arrange + var antiforgeryFeature = new AntiforgeryFeature(); + var context = CreateMockContext( + new AntiforgeryOptions(), + useOldCookie: true, + isOldCookieValid: true, + antiforgeryFeature: antiforgeryFeature); + var antiforgery = GetAntiforgery(context); + context.HttpContext.Response.Headers["Cache-Control"] = "public"; + + // Act + var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); + + // Assert + // We shouldn't have saved the cookie because it already existed. + context.TokenStore.Verify( + t => t.SaveCookieToken(It.IsAny(), It.IsAny()), + Times.Never); + + Assert.Null(tokenSet.CookieToken); + Assert.Equal(context.TestTokenSet.FormTokenString, tokenSet.RequestToken); + + Assert.NotNull(antiforgeryFeature); + Assert.Equal(context.TestTokenSet.OldCookieToken, antiforgeryFeature.CookieToken); + Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.CacheControl]); + Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.Pragma]); + } + [Fact] public void GetAndStoreTokens_NoExistingCookieToken_Saved() { @@ -309,6 +377,36 @@ public void GetAndStoreTokens_NoExistingCookieToken_Saved() Assert.True(antiforgeryFeature.HaveStoredNewCookieToken); } + [Fact] + public void GetAndStoreTokens_NoExistingCookieToken_Saved_AndSetsDoNotCacheHeaders() + { + // Arrange + var antiforgeryFeature = new AntiforgeryFeature(); + var context = CreateMockContext( + new AntiforgeryOptions(), + useOldCookie: false, + isOldCookieValid: false, + antiforgeryFeature: antiforgeryFeature); + var antiforgery = GetAntiforgery(context); + + // Act + var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); + + // Assert + context.TokenStore.Verify( + t => t.SaveCookieToken(It.IsAny(), context.TestTokenSet.NewCookieTokenString), + Times.Once); + + Assert.Equal(context.TestTokenSet.NewCookieTokenString, tokenSet.CookieToken); + Assert.Equal(context.TestTokenSet.FormTokenString, tokenSet.RequestToken); + + Assert.NotNull(antiforgeryFeature); + Assert.True(antiforgeryFeature.HaveDeserializedCookieToken); + Assert.Equal(context.TestTokenSet.OldCookieToken, antiforgeryFeature.CookieToken); + Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.CacheControl]); + Assert.Equal("no-cache", context.HttpContext.Response.Headers[HeaderNames.Pragma]); + } + [Fact] public void GetAndStoreTokens_DoesNotSerializeTwice() { @@ -808,6 +906,76 @@ public void SetCookieTokenAndHeader_PreserveXFrameOptionsHeader() Assert.Equal(expectedHeaderValue, xFrameOptions); } + [Fact] + public void SetCookieTokenAndHeader_NewCookieToken_SetsDoNotCacheHeaders() + { + // Arrange + var options = new AntiforgeryOptions(); + var antiforgeryFeature = new AntiforgeryFeature(); + + // Generate a new cookie. + var context = CreateMockContext( + options, + useOldCookie: false, + isOldCookieValid: false, + antiforgeryFeature: antiforgeryFeature); + var antiforgery = GetAntiforgery(context); + + // Act + antiforgery.SetCookieTokenAndHeader(context.HttpContext); + + // Assert + Assert.Equal("no-cache", context.HttpContext.Response.Headers["Cache-Control"]); + Assert.Equal("no-cache", context.HttpContext.Response.Headers["Pragma"]); + } + + [Fact] + public void SetCookieTokenAndHeader_ValidOldCookieToken_SetsDoNotCacheHeaders() + { + // Arrange + var options = new AntiforgeryOptions(); + var antiforgeryFeature = new AntiforgeryFeature(); + + // Generate a new cookie. + var context = CreateMockContext( + options, + useOldCookie: true, + isOldCookieValid: true, + antiforgeryFeature: antiforgeryFeature); + var antiforgery = GetAntiforgery(context); + + // Act + antiforgery.SetCookieTokenAndHeader(context.HttpContext); + + // Assert + Assert.Equal("no-cache", context.HttpContext.Response.Headers["Cache-Control"]); + Assert.Equal("no-cache", context.HttpContext.Response.Headers["Pragma"]); + } + + [Fact] + public void SetCookieTokenAndHeader_OverridesExistingCachingHeaders() + { + // Arrange + var options = new AntiforgeryOptions(); + var antiforgeryFeature = new AntiforgeryFeature(); + + // Generate a new cookie. + var context = CreateMockContext( + options, + useOldCookie: true, + isOldCookieValid: true, + antiforgeryFeature: antiforgeryFeature); + var antiforgery = GetAntiforgery(context); + context.HttpContext.Response.Headers["Cache-Control"] = "public"; + + // Act + antiforgery.SetCookieTokenAndHeader(context.HttpContext); + + // Assert + Assert.Equal("no-cache", context.HttpContext.Response.Headers["Cache-Control"]); + Assert.Equal("no-cache", context.HttpContext.Response.Headers["Pragma"]); + } + [Theory] [InlineData(false, "SAMEORIGIN")] [InlineData(true, null)] @@ -965,6 +1133,110 @@ public void SetCookieTokenAndHeader_NullCookieToken() context.TokenSerializer.Verify(s => s.Deserialize(null), Times.Never); } + [Fact] + public void GetAndStoreTokens_DoesNotLogWarning_IfNoExistingCacheHeadersPresent() + { + // Arrange + var testSink = new TestSink(); + var loggerFactory = new Mock(); + 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: false, + isOldCookieValid: false, + antiforgeryFeature: antiforgeryFeature); + context.HttpContext.RequestServices = services.BuildServiceProvider(); + var antiforgery = GetAntiforgery(context); + + // Act + var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); + + // Assert + var hasWarningMessage = testSink.Writes + .Where(wc => wc.LogLevel == LogLevel.Warning) + .Select(wc => wc.State?.ToString()) + .Contains(ResponseCacheHeadersOverrideWarningMessage); + Assert.False(hasWarningMessage); + } + + [Theory] + [InlineData("Cache-Control", "Public")] + [InlineData("Cache-Control", "PuBlic")] + [InlineData("Cache-Control", "Private")] + [InlineData("Cache-Control", "PriVate")] + [InlineData("Cache-Control", "No-Store")] + [InlineData("Cache-Control", "No-store")] + [InlineData("Pragma", "Foo")] + public void GetAndStoreTokens_LogsWarning_NonNoCacheHeadersAlreadyPresent(string headerName, string headerValue) + { + // Arrange + var testSink = new TestSink(); + var loggerFactory = new Mock(); + 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: false, + isOldCookieValid: false, + antiforgeryFeature: antiforgeryFeature); + context.HttpContext.RequestServices = services.BuildServiceProvider(); + var antiforgery = GetAntiforgery(context); + context.HttpContext.Response.Headers[headerName] = headerValue; + + // Act + var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); + + // Assert + 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-cache")] + [InlineData("Pragma", "no-cache")] + public void GetAndStoreTokens_DoesNotLogsWarning_ForNoCacheHeaders_AlreadyPresent(string headerName, string headerValue) + { + // Arrange + var testSink = new TestSink(); + var loggerFactory = new Mock(); + 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: false, + isOldCookieValid: false, + antiforgeryFeature: antiforgeryFeature); + context.HttpContext.RequestServices = services.BuildServiceProvider(); + var antiforgery = GetAntiforgery(context); + context.HttpContext.Response.Headers[headerName] = headerValue; + + // Act + var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); + + // Assert + var hasWarningMessage = testSink.Writes + .Where(wc => wc.LogLevel == LogLevel.Warning) + .Select(wc => wc.State?.ToString()) + .Contains(ResponseCacheHeadersOverrideWarningMessage); + Assert.False(hasWarningMessage); + } + private DefaultAntiforgery GetAntiforgery( HttpContext httpContext, AntiforgeryOptions options = null, diff --git a/test/Microsoft.AspNetCore.Antiforgery.Test/project.json b/test/Microsoft.AspNetCore.Antiforgery.Test/project.json index ac8572c..c8cff04 100644 --- a/test/Microsoft.AspNetCore.Antiforgery.Test/project.json +++ b/test/Microsoft.AspNetCore.Antiforgery.Test/project.json @@ -10,6 +10,7 @@ "Microsoft.AspNetCore.Testing": "1.1.0-*", "Microsoft.Extensions.DependencyInjection": "1.1.0-*", "Microsoft.Extensions.Logging": "1.1.0-*", + "Microsoft.Extensions.Logging.Testing": "1.1.0-*", "Microsoft.Extensions.WebEncoders": "1.1.0-*", "Moq": "4.6.36-*", "xunit": "2.2.0-*"