From 3beb3108661d2f9df478a6248ede0feb22fdd968 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Thu, 31 Aug 2017 16:30:33 -0700 Subject: [PATCH] Avoid saving TempData in case of unhandled exceptions. [Fixes #6598] BUG? Accessing TempData prevent response to have content on error. --- .../Internal/SaveTempDataFilter.cs | 66 ++++++++++++++----- .../TempDataTestBase.cs | 36 ++++++++++ .../Internal/SaveTempDataFilterTest.cs | 24 ++++++- .../WebSites/BasicWebSite/BasicWebSite.csproj | 1 + .../Controllers/TempDataController.cs | 15 +++++ .../Filters/TestExceptionFilter.cs | 23 +++++++ test/WebSites/BasicWebSite/Startup.cs | 2 + .../StartupWithSessionTempDataProvider.cs | 1 + 8 files changed, 152 insertions(+), 16 deletions(-) create mode 100644 test/WebSites/BasicWebSite/Filters/TestExceptionFilter.cs diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataFilter.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataFilter.cs index eb4653484d..2ecf4cbbef 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataFilter.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/SaveTempDataFilter.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; @@ -14,7 +15,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal public class SaveTempDataFilter : IResourceFilter, IResultFilter { // Internal for unit testing - internal static readonly object TempDataSavedKey = new object(); + internal static readonly object SaveTempDataFilterContextKey = new object(); private readonly ITempDataDictionaryFactory _factory; @@ -30,18 +31,34 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal /// public void OnResourceExecuting(ResourceExecutingContext context) { + if (!context.HttpContext.Items.ContainsKey(SaveTempDataFilterContextKey)) + { + var tempDataContext = new SaveTempDataContext() + { + Filters = context.Filters, + TempDataDictionaryFactory = _factory + }; + context.HttpContext.Items.Add(SaveTempDataFilterContextKey, tempDataContext); + } + if (!context.HttpContext.Response.HasStarted) { context.HttpContext.Response.OnStarting((state) => { - var saveTempDataContext = (SaveTempDataContext)state; + var httpContext = (HttpContext)state; + + var saveTempDataContext = GetTempDataContext(context.HttpContext); + if (saveTempDataContext.RequestHasUnhandledException) + { + return Task.CompletedTask; + } // If temp data was already saved, skip trying to save again as the calls here would potentially fail // because the session feature might not be available at this point. // Example: An action returns NoContentResult and since NoContentResult does not write anything to // the body of the response, this delegate would get executed way late in the pipeline at which point // the session feature would have been removed. - if (saveTempDataContext.HttpContext.Items.TryGetValue(TempDataSavedKey, out var obj)) + if (saveTempDataContext.TempDataSaved) { return Task.CompletedTask; } @@ -50,22 +67,29 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal result: null, factory: saveTempDataContext.TempDataDictionaryFactory, filters: saveTempDataContext.Filters, - httpContext: saveTempDataContext.HttpContext); + httpContext: httpContext); return Task.CompletedTask; }, - state: new SaveTempDataContext() - { - Filters = context.Filters, - HttpContext = context.HttpContext, - TempDataDictionaryFactory = _factory - }); + state: context.HttpContext); } } /// public void OnResourceExecuted(ResourceExecutedContext context) { + // If there is an unhandled exception, we would like to avoid setting tempdata as + // the end user is going to see an error page anyway and also it helps us in avoiding + // accessing resources like Session too late in the request lifecyle where SessionFeature might + // not be available. + if (!context.HttpContext.Response.HasStarted && context.Exception != null) + { + var saveTempDataContext = GetTempDataContext(context.HttpContext); + if (saveTempDataContext != null) + { + saveTempDataContext.RequestHasUnhandledException = true; + } + } } /// @@ -82,14 +106,25 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal if (!context.HttpContext.Response.HasStarted) { SaveTempData(context.Result, _factory, context.Filters, context.HttpContext); - // If SaveTempDataFilter got added twice this might already be in there. - if (!context.HttpContext.Items.ContainsKey(TempDataSavedKey)) + + var saveTempDataContext = GetTempDataContext(context.HttpContext); + if (saveTempDataContext != null) { - context.HttpContext.Items.Add(TempDataSavedKey, true); + saveTempDataContext.TempDataSaved = true; } } } + private SaveTempDataContext GetTempDataContext(HttpContext httpContext) + { + SaveTempDataContext saveTempDataContext = null; + if (httpContext.Items.TryGetValue(SaveTempDataFilterContextKey, out var value)) + { + saveTempDataContext = (SaveTempDataContext)value; + } + return saveTempDataContext; + } + private static void SaveTempData( IActionResult result, ITempDataDictionaryFactory factory, @@ -114,10 +149,11 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal tempData.Save(); } - private class SaveTempDataContext + internal class SaveTempDataContext { + public bool RequestHasUnhandledException { get; set; } + public bool TempDataSaved { get; set; } public IList Filters { get; set; } - public HttpContext HttpContext { get; set; } public ITempDataDictionaryFactory TempDataDictionaryFactory { get; set; } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataTestBase.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataTestBase.cs index ab9c284483..c370123994 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataTestBase.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataTestBase.cs @@ -207,6 +207,42 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); } + [Fact] + public async Task SaveTempDataFilter_DoesNotSaveTempData_OnUnhandledException() + { + // Arrange & Act + var response = await Client.GetAsync("/TempData/UnhandledExceptionAndSettingTempData"); + + // Assert + Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); + var responseBody = await response.Content.ReadAsStringAsync(); + Assert.Contains("Exception from action UnhandledExceptionAndSettingTempData", responseBody); + + // Arrange & Act + response = await Client.GetAsync("/TempData/UnhandledExceptionAndGetTempData"); + + // Assert + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + } + + [Fact] + public async Task SaveTempDataFilter_DoesNotSaveTempData_OnHandledExceptions() + { + // Arrange & Act + var response = await Client.GetAsync("/TempData/UnhandledExceptionAndSettingTempData?handleException=true"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var responseBody = await response.Content.ReadAsStringAsync(); + Assert.Contains("Exception was handled in TestExceptionFilter", responseBody); + + // Arrange & Act + response = await Client.GetAsync("/TempData/UnhandledExceptionAndGetTempData"); + + // Assert + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + } + public HttpRequestMessage GetRequest(string path, HttpResponseMessage response) { var request = new HttpRequestMessage(HttpMethod.Get, path); diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataFilterTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataFilterTest.cs index 00d4b86db3..9ccf374554 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataFilterTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Internal/SaveTempDataFilterTest.cs @@ -102,7 +102,29 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Internal // Arrange var responseFeature = new TestResponseFeature(); var httpContext = GetHttpContext(responseFeature); - httpContext.Items[SaveTempDataFilter.TempDataSavedKey] = true; // indicate that tempdata was already saved + httpContext.Items[SaveTempDataFilter.SaveTempDataFilterContextKey] = new SaveTempDataFilter.SaveTempDataContext() { TempDataSaved = true }; + var tempDataFactory = new Mock(MockBehavior.Strict); + tempDataFactory + .Setup(f => f.GetTempData(It.IsAny())) + .Verifiable(); + var filter = new SaveTempDataFilter(tempDataFactory.Object); + var context = GetResourceExecutingContext(httpContext); + filter.OnResourceExecuting(context); // registers callback + + // Act + await responseFeature.FireOnSendingHeadersAsync(); + + // Assert + tempDataFactory.Verify(tdf => tdf.GetTempData(It.IsAny()), Times.Never()); + } + + [Fact] + public async Task OnResourceExecuting_DoesNotSaveTempData_WhenUnhandledExceptionOccurs() + { + // Arrange + var responseFeature = new TestResponseFeature(); + var httpContext = GetHttpContext(responseFeature); + httpContext.Items[SaveTempDataFilter.SaveTempDataFilterContextKey] = new SaveTempDataFilter.SaveTempDataContext() { RequestHasUnhandledException = true }; var tempDataFactory = new Mock(MockBehavior.Strict); tempDataFactory .Setup(f => f.GetTempData(It.IsAny())) diff --git a/test/WebSites/BasicWebSite/BasicWebSite.csproj b/test/WebSites/BasicWebSite/BasicWebSite.csproj index c5fd68a314..363c11aefe 100644 --- a/test/WebSites/BasicWebSite/BasicWebSite.csproj +++ b/test/WebSites/BasicWebSite/BasicWebSite.csproj @@ -13,5 +13,6 @@ + diff --git a/test/WebSites/BasicWebSite/Controllers/TempDataController.cs b/test/WebSites/BasicWebSite/Controllers/TempDataController.cs index 1ad0d7a732..a0fedabe02 100644 --- a/test/WebSites/BasicWebSite/Controllers/TempDataController.cs +++ b/test/WebSites/BasicWebSite/Controllers/TempDataController.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Threading.Tasks; +using BasicWebSite.Filters; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; @@ -102,5 +103,19 @@ namespace BasicWebSite.Controllers { return TempData["LargeValue"]?.ToString(); } + + [HttpGet] + [TestExceptionFilter] + public IActionResult UnhandledExceptionAndSettingTempData() + { + TempData[nameof(UnhandledExceptionAndSettingTempData)] = "James"; + throw new InvalidOperationException($"Exception from action {nameof(UnhandledExceptionAndSettingTempData)}"); + } + + [HttpGet] + public string UnhandledExceptionAndGetTempData() + { + return TempData[nameof(UnhandledExceptionAndSettingTempData)]?.ToString(); + } } } diff --git a/test/WebSites/BasicWebSite/Filters/TestExceptionFilter.cs b/test/WebSites/BasicWebSite/Filters/TestExceptionFilter.cs new file mode 100644 index 0000000000..592d36dcd2 --- /dev/null +++ b/test/WebSites/BasicWebSite/Filters/TestExceptionFilter.cs @@ -0,0 +1,23 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; + +namespace BasicWebSite.Filters +{ + public class TestExceptionFilter : ExceptionFilterAttribute + { + public override void OnException(ExceptionContext context) + { + if (context.HttpContext.Request.Query.TryGetValue("handleException", out var handleException)) + { + if (handleException.Equals("true")) + { + context.Result = new ContentResult() { Content = "Exception was handled in TestExceptionFilter", StatusCode = 200 }; + context.ExceptionHandled = true; + } + } + } + } +} diff --git a/test/WebSites/BasicWebSite/Startup.cs b/test/WebSites/BasicWebSite/Startup.cs index 9cc9d295eb..158b3604c1 100644 --- a/test/WebSites/BasicWebSite/Startup.cs +++ b/test/WebSites/BasicWebSite/Startup.cs @@ -28,6 +28,8 @@ namespace BasicWebSite public void Configure(IApplicationBuilder app) { + app.UseDeveloperExceptionPage(); + app.UseStaticFiles(); // Initializes the RequestId service for each request diff --git a/test/WebSites/BasicWebSite/StartupWithSessionTempDataProvider.cs b/test/WebSites/BasicWebSite/StartupWithSessionTempDataProvider.cs index 6500601f0d..cc13dd6695 100644 --- a/test/WebSites/BasicWebSite/StartupWithSessionTempDataProvider.cs +++ b/test/WebSites/BasicWebSite/StartupWithSessionTempDataProvider.cs @@ -19,6 +19,7 @@ namespace BasicWebSite public void Configure(IApplicationBuilder app) { + app.UseDeveloperExceptionPage(); app.UseSession(); app.UseMvcWithDefaultRoute(); }