From 6138087de65010552dc5b5a7ffc2663f389cab74 Mon Sep 17 00:00:00 2001 From: Artak Mkrtchyan Date: Mon, 22 Jan 2018 14:50:45 -0800 Subject: [PATCH 1/6] Updated the DefaultAntiforgery to set the the cache headers only if they aren't set yet. --- .../Internal/DefaultAntiforgery.cs | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs index 24aa3cb7d3..a989ea61de 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs @@ -24,11 +24,11 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal private readonly ILogger _logger; public DefaultAntiforgery( - IOptions antiforgeryOptionsAccessor, - IAntiforgeryTokenGenerator tokenGenerator, - IAntiforgeryTokenSerializer tokenSerializer, - IAntiforgeryTokenStore tokenStore, - ILoggerFactory loggerFactory) + IOptions antiforgeryOptionsAccessor, + IAntiforgeryTokenGenerator tokenGenerator, + IAntiforgeryTokenSerializer tokenSerializer, + IAntiforgeryTokenStore tokenStore, + ILoggerFactory loggerFactory) { _options = antiforgeryOptionsAccessor.Value; _tokenGenerator = tokenGenerator; @@ -374,13 +374,28 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal /// The . protected virtual 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); + bool cacheHeadersChanged = SetHeaderIfNotSet(httpContext, HeaderNames.CacheControl, "no-cache, no-store"); + cacheHeadersChanged |= SetHeaderIfNotSet(httpContext, HeaderNames.Pragma, "no-cache"); - httpContext.Response.Headers[HeaderNames.CacheControl] = "no-cache, no-store"; - httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; + if (cacheHeadersChanged) + { + // 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); + } + } + + private static bool SetHeaderIfNotSet(HttpContext context, string headerName, string value) + { + if (!context.Response.Headers.ContainsKey(headerName)) + { + context.Response.Headers[headerName] = value; + + return true; + } + + return false; } private void LogCacheHeaderOverrideWarning(HttpResponse response) From 77b04e3c82491047449abb2008ecebe8072716c3 Mon Sep 17 00:00:00 2001 From: Artak Mkrtchyan Date: Mon, 22 Jan 2018 14:53:46 -0800 Subject: [PATCH 2/6] Removed redundant spacings --- .../Internal/DefaultAntiforgery.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs index a989ea61de..bc7a3d2646 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs @@ -24,11 +24,11 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal private readonly ILogger _logger; public DefaultAntiforgery( - IOptions antiforgeryOptionsAccessor, - IAntiforgeryTokenGenerator tokenGenerator, - IAntiforgeryTokenSerializer tokenSerializer, - IAntiforgeryTokenStore tokenStore, - ILoggerFactory loggerFactory) + IOptions antiforgeryOptionsAccessor, + IAntiforgeryTokenGenerator tokenGenerator, + IAntiforgeryTokenSerializer tokenSerializer, + IAntiforgeryTokenStore tokenStore, + ILoggerFactory loggerFactory) { _options = antiforgeryOptionsAccessor.Value; _tokenGenerator = tokenGenerator; From 4fa975a416b8402aa82c7267e58b118048ec021a Mon Sep 17 00:00:00 2001 From: Artak Mkrtchyan Date: Mon, 22 Jan 2018 16:40:57 -0800 Subject: [PATCH 3/6] Writing header cache values only when the response hasn't yet started --- .../Internal/DefaultAntiforgery.cs | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs index bc7a3d2646..5cc860b593 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs @@ -67,9 +67,12 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal } } - // 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); + if (!httpContext.Response.HasStarted) + { + // 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; } @@ -247,7 +250,10 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal _logger.ReusedCookieToken(); } - SetDoNotCacheHeaders(httpContext); + if (!httpContext.Response.HasStarted) + { + SetDoNotCacheHeaders(httpContext); + } } private void SaveCookieTokenAndHeader(HttpContext httpContext, string cookieToken) @@ -374,28 +380,13 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal /// The . protected virtual void SetDoNotCacheHeaders(HttpContext httpContext) { - bool cacheHeadersChanged = SetHeaderIfNotSet(httpContext, HeaderNames.CacheControl, "no-cache, no-store"); - cacheHeadersChanged |= SetHeaderIfNotSet(httpContext, HeaderNames.Pragma, "no-cache"); - - if (cacheHeadersChanged) - { // 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); - } - } - private static bool SetHeaderIfNotSet(HttpContext context, string headerName, string value) - { - if (!context.Response.Headers.ContainsKey(headerName)) - { - context.Response.Headers[headerName] = value; - - return true; - } - - return false; + httpContext.Response.Headers[HeaderNames.CacheControl] = "no-cache, no-store"; + httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; } private void LogCacheHeaderOverrideWarning(HttpResponse response) From ea38977599ac51e2542985a9815d1dd6107a376b Mon Sep 17 00:00:00 2001 From: Artak Mkrtchyan Date: Mon, 22 Jan 2018 16:41:43 -0800 Subject: [PATCH 4/6] Removed unnecessary spaces --- .../Internal/DefaultAntiforgery.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs index 5cc860b593..cef46be893 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs @@ -380,13 +380,13 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal /// The . protected virtual 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); + // 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, no-store"; - httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; + httpContext.Response.Headers[HeaderNames.CacheControl] = "no-cache, no-store"; + httpContext.Response.Headers[HeaderNames.Pragma] = "no-cache"; } private void LogCacheHeaderOverrideWarning(HttpResponse response) From 763393efc48f410be954d833f706577c06f16038 Mon Sep 17 00:00:00 2001 From: Artak Mkrtchyan Date: Mon, 22 Jan 2018 17:51:13 -0800 Subject: [PATCH 5/6] Added a test verifying that the cache headers are not set after the response has started. --- .../Internal/DefaultAntiforgeryTest.cs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs index 4eebaad29f..4f041d4fc3 100644 --- a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs +++ b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Testing; @@ -1137,6 +1138,47 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal context.TokenSerializer.Verify(s => s.Deserialize(null), Times.Never); } + [Fact] + public void SetCookieTokenAndHeader_DoesNotModifyHeadersAfterResponseHasStarted() + { + // Arrange + var antiforgeryFeature = new AntiforgeryFeature + { + HaveDeserializedCookieToken = false, + HaveGeneratedNewCookieToken = false, + HaveStoredNewCookieToken = true, + NewCookieToken = new AntiforgeryToken(), + NewCookieTokenString = "serialized-cookie-token-from-context", + NewRequestToken = new AntiforgeryToken(), + NewRequestTokenString = "serialized-form-token-from-context", + }; + var context = CreateMockContext( + new AntiforgeryOptions(), + useOldCookie: false, + isOldCookieValid: false, + antiforgeryFeature: antiforgeryFeature); + var testTokenSet = new TestTokenSet + { + OldCookieTokenString = null + }; + + var nullTokenStore = GetTokenStore(context.HttpContext, testTokenSet, false); + var antiforgery = GetAntiforgery( + context.HttpContext, + tokenGenerator: context.TokenGenerator.Object, + tokenStore: nullTokenStore.Object); + + TestResponseFeature testResponse = new TestResponseFeature(); + context.HttpContext.Features.Set(testResponse); + context.HttpContext.Response.Headers["Cache-Control"] = "public"; + testResponse.StartResponse(); + + // Act + antiforgery.SetCookieTokenAndHeader(context.HttpContext); + + Assert.Equal("public", context.HttpContext.Response.Headers["Cache-Control"]); + } + [Fact] public void GetAndStoreTokens_DoesNotLogWarning_IfNoExistingCacheHeadersPresent() { @@ -1196,6 +1238,7 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal var antiforgery = GetAntiforgery(context); context.HttpContext.Response.Headers[headerName] = headerValue; + //context.HttpContext.Features // Act var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext); @@ -1435,5 +1478,21 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal { public AntiforgeryOptions Value { get; set; } = new AntiforgeryOptions(); } + + private class TestResponseFeature : HttpResponseFeature + { + private bool _hasStarted = false; + + public override bool HasStarted { get => _hasStarted; } + + public TestResponseFeature() + { + } + + public void StartResponse() + { + _hasStarted = true; + } + } } } From 18bd033d8ec320ef7f58a477d090818822cc257e Mon Sep 17 00:00:00 2001 From: Artak Mkrtchyan Date: Mon, 22 Jan 2018 18:10:01 -0800 Subject: [PATCH 6/6] Removed unnecessary comment --- .../Internal/DefaultAntiforgeryTest.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs index 4f041d4fc3..faf895d524 100644 --- a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs +++ b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs @@ -1238,7 +1238,6 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal var antiforgery = GetAntiforgery(context); context.HttpContext.Response.Headers[headerName] = headerValue; - //context.HttpContext.Features // Act var tokenSet = antiforgery.GetAndStoreTokens(context.HttpContext);