From aab383e4c2b8987ae84b685bf8b17b3504d6d038 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 7 Jul 2017 10:46:21 -0700 Subject: [PATCH] Fix #6480 This change logs when we encounter and exception reading temp data from a cookie and swallows the exception. Additionally, we clear the cookie so that this won't happen on subsequent requests. This will handle cases where data protection is misconfigured, or a request just has general garbage in the cookies. --- .../MvcViewFeaturesLoggerExtensions.cs | 34 ++++++++++++++ .../ViewFeatures/CookieTempDataProvider.cs | 43 +++++++++++++++--- .../CookieTempDataProviderTest.cs | 45 ++++++++++++++++++- 3 files changed, 114 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/MvcViewFeaturesLoggerExtensions.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/MvcViewFeaturesLoggerExtensions.cs index b29284b00a..2f0145a466 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/MvcViewFeaturesLoggerExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/MvcViewFeaturesLoggerExtensions.cs @@ -30,6 +30,10 @@ internal static class MvcViewFeaturesLoggerExtensions private static readonly Action _viewFound; private static readonly Action, Exception> _viewNotFound; + private static readonly Action _tempDataCookieNotFound; + private static readonly Action _tempDataCookieLoadSuccess; + private static readonly Action _tempDataCookieLoadFailure; + static MvcViewFeaturesLoggerExtensions() { _viewComponentExecuting = LoggerMessage.Define( @@ -82,6 +86,21 @@ static MvcViewFeaturesLoggerExtensions() LogLevel.Error, 3, "The view '{ViewName}' was not found. Searched locations: {SearchedViewLocations}"); + + _tempDataCookieNotFound = LoggerMessage.Define( + LogLevel.Debug, + 1, + "The temp data cookie {CookieName} was not found."); + + _tempDataCookieLoadSuccess = LoggerMessage.Define( + LogLevel.Debug, + 2, + "The temp data cookie {CookieName} was used to successfully load temp data."); + + _tempDataCookieLoadFailure = LoggerMessage.Define( + LogLevel.Warning, + 3, + "The temp data cookie {CookieName} could not be loaded."); } public static IDisposable ViewComponentScope(this ILogger logger, ViewComponentContext context) @@ -192,6 +211,21 @@ public static void ViewNotFound(this ILogger logger, string viewName, _viewNotFound(logger, viewName, searchedLocations, null); } + public static void TempDataCookieNotFound(this ILogger logger, string cookieName) + { + _tempDataCookieNotFound(logger, cookieName, null); + } + + public static void TempDataCookieLoadSuccess(this ILogger logger, string cookieName) + { + _tempDataCookieLoadSuccess(logger, cookieName, null); + } + + public static void TempDataCookieLoadFailure(this ILogger logger, string cookieName, Exception exception) + { + _tempDataCookieLoadFailure(logger, cookieName, exception); + } + private class ViewComponentLogScope : IReadOnlyList> { private readonly ViewComponentDescriptor _descriptor; diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs index 726b6bd83c..0cf8f29cdc 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs @@ -9,6 +9,7 @@ using Microsoft.AspNetCore.Internal; using Microsoft.AspNetCore.Mvc.ViewFeatures.Internal; using Microsoft.Extensions.Options; +using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.ViewFeatures { @@ -19,14 +20,20 @@ public class CookieTempDataProvider : ITempDataProvider { public static readonly string CookieName = ".AspNetCore.Mvc.CookieTempDataProvider"; private static readonly string Purpose = "Microsoft.AspNetCore.Mvc.CookieTempDataProviderToken.v1"; + private readonly IDataProtector _dataProtector; + private readonly ILogger _logger; private readonly TempDataSerializer _tempDataSerializer; private readonly ChunkingCookieManager _chunkingCookieManager; private readonly CookieTempDataProviderOptions _options; - public CookieTempDataProvider(IDataProtectionProvider dataProtectionProvider, IOptions options) + public CookieTempDataProvider( + IDataProtectionProvider dataProtectionProvider, + ILoggerFactory loggerFactory, + IOptions options) { _dataProtector = dataProtectionProvider.CreateProtector(Purpose); + _logger = loggerFactory.CreateLogger(); _tempDataSerializer = new TempDataSerializer(); _chunkingCookieManager = new ChunkingCookieManager(); _options = options.Value; @@ -41,15 +48,39 @@ public IDictionary LoadTempData(HttpContext context) if (context.Request.Cookies.ContainsKey(_options.Cookie.Name)) { - var encodedValue = _chunkingCookieManager.GetRequestCookie(context, _options.Cookie.Name); - if (!string.IsNullOrEmpty(encodedValue)) + // The cookie we use for temp data is user input, and might be invalid in a whole host of ways. + // + // Since TempData is a best-effort system, we don't want to throw and get a 500 if the cookie is + // bad, we will just clear it and ignore the exception. The common case that we've identified for + // this is misconfigured data protection settings, which can cause the key used to create the + // cookie to no longer be available. + try { - var protectedData = Base64UrlTextEncoder.Decode(encodedValue); - var unprotectedData = _dataProtector.Unprotect(protectedData); - return _tempDataSerializer.Deserialize(unprotectedData); + var encodedValue = _chunkingCookieManager.GetRequestCookie(context, _options.Cookie.Name); + if (!string.IsNullOrEmpty(encodedValue)) + { + var protectedData = Base64UrlTextEncoder.Decode(encodedValue); + var unprotectedData = _dataProtector.Unprotect(protectedData); + var tempData = _tempDataSerializer.Deserialize(unprotectedData); + + _logger.TempDataCookieLoadSuccess(_options.Cookie.Name); + return tempData; + } + } + catch (Exception ex) + { + _logger.TempDataCookieLoadFailure(_options.Cookie.Name, ex); + + // If we're failed, we want to try and clear the cookie so that this won't keep happening + // over and over. + if (!context.Response.HasStarted) + { + _chunkingCookieManager.DeleteCookie(context, _options.Cookie.Name, _options.Cookie.Build(context)); + } } } + _logger.TempDataCookieNotFound(_options.Cookie.Name); return new Dictionary(StringComparer.OrdinalIgnoreCase); } diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs index dc9ad123cf..44aeceb741 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs @@ -9,7 +9,9 @@ using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.ViewFeatures.Internal; using Microsoft.AspNetCore.WebUtilities; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; +using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -66,6 +68,41 @@ public void LoadTempData_ReturnsEmptyDictionary_WhenNoCookieDataIsAvailable() Assert.Empty(tempDataDictionary); } + [Fact] + public void LoadTempData_ReturnsEmptyDictionary_AndClearsCookie_WhenDataIsInvalid() + { + // Arrange + var dataProtector = new Mock(MockBehavior.Strict); + dataProtector + .Setup(d => d.Unprotect(It.IsAny())) + .Throws(new Exception()); + + var tempDataProvider = GetProvider(dataProtector.Object); + + var inputData = new Dictionary(); + inputData.Add("int", 10); + var tempDataProviderSerializer = new TempDataSerializer(); + var expectedDataToUnprotect = tempDataProviderSerializer.Serialize(inputData); + var base64AndUrlEncodedDataInCookie = Base64UrlTextEncoder.Encode(expectedDataToUnprotect); + + var context = new DefaultHttpContext(); + context.Request.Cookies = new RequestCookieCollection(new Dictionary() + { + { CookieTempDataProvider.CookieName, base64AndUrlEncodedDataInCookie } + }); + + // Act + var tempDataDictionary = tempDataProvider.LoadTempData(context); + + // Assert + Assert.Empty(tempDataDictionary); + + var setCookieHeader = SetCookieHeaderValue.Parse(context.Response.Headers["Set-Cookie"].ToString()); + Assert.Equal(CookieTempDataProvider.CookieName, setCookieHeader.Name.ToString()); + Assert.Equal(string.Empty, setCookieHeader.Value.ToString()); + + } + [Fact] public void LoadTempData_Base64UrlDecodesAnd_UnprotectsData_FromCookie() { @@ -219,7 +256,11 @@ public void SaveTempData_DefaultProviderOptions_SetsCookie_WithAppropriateCookie [InlineData("/", "/vdir1", ".abc.com", "/vdir1", ".abc.com")] [InlineData("/vdir1", "/", ".abc.com", "/", ".abc.com")] public void SaveTempData_CustomProviderOptions_SetsCookie_WithAppropriateCookieOptions( - string requestPathBase, string optionsPath, string optionsDomain, string expectedCookiePath, string expectedDomain) + string requestPathBase, + string optionsPath, + string optionsDomain, + string expectedCookiePath, + string expectedDomain) { // Arrange var values = new Dictionary(); @@ -637,7 +678,7 @@ private CookieTempDataProvider GetProvider(IDataProtector dataProtector = null, var testOptions = new Mock>(); testOptions.SetupGet(o => o.Value).Returns(options); - return new CookieTempDataProvider(new PassThroughDataProtectionProvider(dataProtector), testOptions.Object); + return new CookieTempDataProvider(new PassThroughDataProtectionProvider(dataProtector), NullLoggerFactory.Instance, testOptions.Object); } private class PassThroughDataProtectionProvider : IDataProtectionProvider