From 5bb69e495e9a7f62ac42bf77e1e8bcbb04af73cf Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Fri, 20 Feb 2015 13:38:25 -0800 Subject: [PATCH] [Fixes #1184] Formatting issues in requests should not cause a 500 response --- .../Formatters/JsonInputFormatter.cs | 28 +++----- .../ModelBinders/BodyModelBinder.cs | 24 ++++++- .../BodyModelBinderTests.cs | 70 ++++++++++++++++++- .../Formatters/JsonInputFormatterTest.cs | 36 +++------- ...ataContractSerializerInputFormatterTest.cs | 23 ++++++ .../XmlSerializerInputFormatterTests.cs | 13 ++-- test/WebSites/ModelBindingWebSite/Startup.cs | 2 - .../Controllers/HomeController.cs | 46 ++++++++++++ 8 files changed, 184 insertions(+), 58 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/JsonInputFormatter.cs b/src/Microsoft.AspNet.Mvc.Core/Formatters/JsonInputFormatter.cs index 7e4243614c..9cba809892 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/JsonInputFormatter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Formatters/JsonInputFormatter.cs @@ -57,12 +57,6 @@ namespace Microsoft.AspNet.Mvc } } - /// - /// Gets or sets if deserialization errors are captured. When set, these errors appear in - /// the instance of . - /// - public bool CaptureDeserilizationErrors { get; set; } - /// public override Task ReadRequestBodyAsync([NotNull] InputFormatterContext context) { @@ -82,19 +76,17 @@ namespace Microsoft.AspNet.Mvc var jsonSerializer = CreateJsonSerializer(); EventHandler errorHandler = null; - if (CaptureDeserilizationErrors) + errorHandler = (sender, e) => { - errorHandler = (sender, e) => - { - var exception = e.ErrorContext.Error; - context.ActionContext.ModelState.TryAddModelError(e.ErrorContext.Path, e.ErrorContext.Error); - // Error must always be marked as handled - // Failure to do so can cause the exception to be rethrown at every recursive level and - // overflow the stack for x64 CLR processes - e.ErrorContext.Handled = true; - }; - jsonSerializer.Error += errorHandler; - } + var exception = e.ErrorContext.Error; + context.ActionContext.ModelState.TryAddModelError(e.ErrorContext.Path, e.ErrorContext.Error); + + // Error must always be marked as handled + // Failure to do so can cause the exception to be rethrown at every recursive level and + // overflow the stack for x64 CLR processes + e.ErrorContext.Handled = true; + }; + jsonSerializer.Error += errorHandler; try { diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinders/BodyModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinders/BodyModelBinder.cs index b1bf739ed2..a8a9352744 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinders/BodyModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinders/BodyModelBinder.cs @@ -3,6 +3,7 @@ using System; using System.Linq; +using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.Core; using Microsoft.AspNet.Mvc.ModelBinding; @@ -59,8 +60,29 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return new ModelBindingResult(null, bindingContext.ModelName, isModelSet: false); } - var model = await formatter.ReadAsync(formatterContext); + object model = null; + try + { + model = await formatter.ReadAsync(formatterContext); + } + catch (Exception ex) + { + model = GetDefaultValueForType(bindingContext.ModelType); + bindingContext.ModelState.AddModelError(bindingContext.ModelName, ex); + return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); + } + return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true); } + + private object GetDefaultValueForType(Type modelType) + { + if (modelType.GetTypeInfo().IsValueType) + { + return Activator.CreateInstance(modelType); + } + + return null; + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/BodyModelBinderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/BodyModelBinderTests.cs index 2959e22b0d..386b2537c3 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/BodyModelBinderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/BodyModelBinderTests.cs @@ -2,13 +2,17 @@ // 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.Collections.Generic; +using System.IO; +using System.Text; using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Http.Core; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Routing; using Microsoft.Framework.DependencyInjection; +using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -35,6 +39,8 @@ namespace Microsoft.AspNet.Mvc // Assert mockInputFormatter.Verify(v => v.ReadAsync(It.IsAny()), Times.Once); + Assert.NotNull(binderResult); + Assert.True(binderResult.IsModelSet); } [Fact] @@ -116,14 +122,48 @@ namespace Microsoft.AspNet.Mvc Assert.Null(binderResult); } + [Fact] + public async Task CustomFormatterDeserializationException_AddedToModelState() + { + // Arrange + var httpContext = new DefaultHttpContext(); + httpContext.Request.Body = new MemoryStream(Encoding.UTF8.GetBytes("Bad data!")); + httpContext.Request.ContentType = "text/xyz"; + var bindingContext = GetBindingContext(httpContext, typeof(Person), inputFormatter: new XyzFormatter()); + bindingContext.ModelMetadata.BinderMetadata = new FromBodyAttribute(); + + var binder = bindingContext.OperationBindingContext.ModelBinder; + + // Act + var binderResult = await binder.BindModelAsync(bindingContext); + + // Assert + + // Returns true because it understands the metadata type. + Assert.NotNull(binderResult); + Assert.False(binderResult.IsModelSet); + Assert.Null(binderResult.Model); + Assert.True(bindingContext.ModelState.ContainsKey("someName")); + var errorMessage = bindingContext.ModelState["someName"].Errors[0].Exception.Message; + Assert.Equal("Your input is bad!", errorMessage); + } + private static ModelBindingContext GetBindingContext(Type modelType, IInputFormatter inputFormatter) + { + return GetBindingContext(new DefaultHttpContext(), modelType, inputFormatter); + } + + private static ModelBindingContext GetBindingContext( + HttpContext httpContext, + Type modelType, + IInputFormatter inputFormatter) { var metadataProvider = new EmptyModelMetadataProvider(); var operationBindingContext = new OperationBindingContext { - ModelBinder = GetBodyBinder(inputFormatter), + ModelBinder = GetBodyBinder(httpContext, inputFormatter), MetadataProvider = metadataProvider, - HttpContext = new DefaultHttpContext(), + HttpContext = httpContext, }; var bindingContext = new ModelBindingContext @@ -140,7 +180,12 @@ namespace Microsoft.AspNet.Mvc private static BodyModelBinder GetBodyBinder(IInputFormatter inputFormatter) { - var actionContext = CreateActionContext(new DefaultHttpContext()); + return GetBodyBinder(new DefaultHttpContext(), inputFormatter); + } + + private static BodyModelBinder GetBodyBinder(HttpContext httpContext, IInputFormatter inputFormatter) + { + var actionContext = CreateActionContext(httpContext); var inputFormatterSelector = new Mock(); inputFormatterSelector .Setup(o => o.SelectFormatter( @@ -194,5 +239,24 @@ namespace Microsoft.AspNet.Mvc { public string Name { get; set; } } + + private class XyzFormatter : InputFormatter + { + public XyzFormatter() + { + SupportedMediaTypes.Add(new MediaTypeHeaderValue("text/xyz")); + SupportedEncodings.Add(Encoding.UTF8); + } + + protected override bool CanReadType(Type type) + { + return true; + } + + public override Task ReadRequestBodyAsync(InputFormatterContext context) + { + throw new InvalidOperationException("Your input is bad!"); + } + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/JsonInputFormatterTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/JsonInputFormatterTest.cs index 9c3e30d05c..2f5d5e17ba 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/JsonInputFormatterTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Formatters/JsonInputFormatterTest.cs @@ -112,29 +112,13 @@ namespace Microsoft.AspNet.Mvc } [Fact] - public async Task ReadAsync_ThrowsOnDeserializationErrors() + public async Task ReadAsync_AddsModelValidationErrorsToModelState() { // Arrange var content = "{name: 'Person Name', Age: 'not-an-age'}"; var formatter = new JsonInputFormatter(); var contentBytes = Encoding.UTF8.GetBytes(content); - var httpContext = GetActionContext(contentBytes); - var metadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(User)); - var context = new InputFormatterContext(httpContext, metadata.ModelType); - - // Act and Assert - await Assert.ThrowsAsync(() => formatter.ReadAsync(context)); - } - - [Fact] - public async Task ReadAsync_AddsModelValidationErrorsToModelState_WhenCaptureErrorsIsSet() - { - // Arrange - var content = "{name: 'Person Name', Age: 'not-an-age'}"; - var formatter = new JsonInputFormatter { CaptureDeserilizationErrors = true }; - var contentBytes = Encoding.UTF8.GetBytes(content); - var actionContext = GetActionContext(contentBytes); var metadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(User)); var context = new InputFormatterContext(actionContext, metadata.ModelType); @@ -148,11 +132,11 @@ namespace Microsoft.AspNet.Mvc } [Fact] - public async Task ReadAsync_UsesTryAddModelValidationErrorsToModelState_WhenCaptureErrorsIsSet() + public async Task ReadAsync_UsesTryAddModelValidationErrorsToModelState() { // Arrange var content = "{name: 'Person Name', Age: 'not-an-age'}"; - var formatter = new JsonInputFormatter { CaptureDeserilizationErrors = true }; + var formatter = new JsonInputFormatter(); var contentBytes = Encoding.UTF8.GetBytes(content); var actionContext = GetActionContext(contentBytes); @@ -189,7 +173,7 @@ namespace Microsoft.AspNet.Mvc // missing password property here var contentBytes = Encoding.UTF8.GetBytes("{ \"UserName\" : \"John\"}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); // by default we ignore missing members, so here explicitly changing it jsonFormatter.SerializerSettings.MissingMemberHandling = MissingMemberHandling.Error; @@ -214,7 +198,7 @@ namespace Microsoft.AspNet.Mvc // missing password property here var contentBytes = Encoding.UTF8.GetBytes("{ \"UserName\" : \"John\"}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); // by default we ignore missing members, so here explicitly changing it jsonFormatter.SerializerSettings = new JsonSerializerSettings() { @@ -240,7 +224,7 @@ namespace Microsoft.AspNet.Mvc { // Arrange var contentBytes = Encoding.UTF8.GetBytes("{\"Id\":\"null\",\"Name\":\"Programming C#\"}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); var actionContext = GetActionContext(contentBytes, "application/json;charset=utf-8"); var metadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(Book)); var inputFormatterContext = new InputFormatterContext(actionContext, metadata.ModelType); @@ -267,7 +251,7 @@ namespace Microsoft.AspNet.Mvc { // Arrange var contentBytes = Encoding.UTF8.GetBytes("{ \"Name\" : \"Programming C#\"}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); var actionContext = GetActionContext(contentBytes, "application/json;charset=utf-8"); var metadata = new EmptyModelMetadataProvider().GetMetadataForType(type); var inputFormatterContext = new InputFormatterContext(actionContext, metadata.ModelType); @@ -288,7 +272,7 @@ namespace Microsoft.AspNet.Mvc { // Arrange var contentBytes = Encoding.UTF8.GetBytes("{\"Longitude\":{}}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); var actionContext = GetActionContext(contentBytes, "application/json;charset=utf-8"); var metadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(GpsCoordinate)); var inputFormatterContext = new InputFormatterContext(actionContext, metadata.ModelType); @@ -317,7 +301,7 @@ namespace Microsoft.AspNet.Mvc { // Arrange var contentBytes = Encoding.UTF8.GetBytes("{\"Name\":\"Seattle\"}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); var actionContext = GetActionContext(contentBytes, "application/json;charset=utf-8"); var metadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(Location)); var inputFormatterContext = new InputFormatterContext(actionContext, metadata.ModelType); @@ -338,7 +322,7 @@ namespace Microsoft.AspNet.Mvc { // Arrange var contentBytes = Encoding.UTF8.GetBytes("{}"); - var jsonFormatter = new JsonInputFormatter() { CaptureDeserilizationErrors = true }; + var jsonFormatter = new JsonInputFormatter(); var actionContext = GetActionContext(contentBytes, "application/json;charset=utf-8"); var metadata = new EmptyModelMetadataProvider().GetMetadataForType(typeof(Venue)); var inputFormatterContext = new InputFormatterContext(actionContext, metadata.ModelType); diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlDataContractSerializerInputFormatterTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlDataContractSerializerInputFormatterTest.cs index 4ddda9c425..c89f682409 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlDataContractSerializerInputFormatterTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlDataContractSerializerInputFormatterTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.Linq; +using System.Net; using System.Net.Http; using System.Net.Http.Headers; using System.Runtime.Serialization; @@ -30,6 +31,28 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests nameof(DataMemberAttribute.IsRequired), bool.TrueString); + [Fact] + public async Task ThrowsOnInvalidInput_AndAddsToModelState() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + var input = "Not a valid xml document"; + var content = new StringContent(input, Encoding.UTF8, "application/xml-dcs"); + + // Act + var response = await client.PostAsync("http://localhost/Home/Index", content); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var data = await response.Content.ReadAsStringAsync(); + Assert.Contains( + string.Format( + "dummyObject:There was an error deserializing the object of type {0}", + typeof(DummyClass).FullName), + data); + } + // Verifies that even though all the required data is posted to an action, the model // state has errors related to value types's Required attribute validation. [Fact] diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlSerializerInputFormatterTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlSerializerInputFormatterTests.cs index b8e2e387f3..e2d57e16ec 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlSerializerInputFormatterTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/XmlSerializerInputFormatterTests.cs @@ -38,24 +38,21 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests } [Fact] - public async Task XmlSerializerFormatter_ThrowsOnIncorrectInputNamespace() + public async Task ThrowsOnInvalidInput_AndAddsToModelState() { // Arrange var server = TestServer.Create(_services, _app); var client = server.CreateClient(); - var sampleInputInt = 10; - var input = "" + - "" + - "" + sampleInputInt.ToString() + ""; + var input = "Not a valid xml document"; var content = new StringContent(input, Encoding.UTF8, "application/xml-xmlser"); // Act var response = await client.PostAsync("http://localhost/Home/Index", content); // Assert - var exception = response.GetServerException(); - Assert.Equal(typeof(InvalidOperationException).FullName, exception.ExceptionType); + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var data = await response.Content.ReadAsStringAsync(); + Assert.Contains("dummyObject:There is an error in XML document", data); } } } \ No newline at end of file diff --git a/test/WebSites/ModelBindingWebSite/Startup.cs b/test/WebSites/ModelBindingWebSite/Startup.cs index aa4839714a..51a0b3a3db 100644 --- a/test/WebSites/ModelBindingWebSite/Startup.cs +++ b/test/WebSites/ModelBindingWebSite/Startup.cs @@ -29,8 +29,6 @@ namespace ModelBindingWebSite m.MaxModelValidationErrors = 8; m.ModelBinders.Insert(0, typeof(TestBindingSourceModelBinder)); - m.InputFormatters.InstanceOf().CaptureDeserilizationErrors = true; - m.AddXmlDataContractSerializerFormatter(); m.ValidationExcludeFilters.Add(typeof(Address)); }); diff --git a/test/WebSites/XmlFormattersWebSite/Controllers/HomeController.cs b/test/WebSites/XmlFormattersWebSite/Controllers/HomeController.cs index 0e02811166..c55f2f7625 100644 --- a/test/WebSites/XmlFormattersWebSite/Controllers/HomeController.cs +++ b/test/WebSites/XmlFormattersWebSite/Controllers/HomeController.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.Linq; using Microsoft.AspNet.Mvc; +using Microsoft.AspNet.Mvc.ModelBinding; namespace XmlFormattersWebSite { @@ -13,7 +14,52 @@ namespace XmlFormattersWebSite [HttpPost] public IActionResult Index([FromBody]DummyClass dummyObject) { + if (!ModelState.IsValid) + { + return new ObjectResult(GetModelStateErrorMessages(ModelState)) + { + StatusCode = 400 + }; + } + return Content(dummyObject.SampleInt.ToString()); } + + // Cannot use 'SerializableError' here as it sanitizes exceptions in model state with generic error message. + // Since the tests need to verify the messages, we are doing the following. + private List GetModelStateErrorMessages(ModelStateDictionary modelStateDictionary) + { + var allErrorMessages = new List(); + foreach (var keyModelStatePair in modelStateDictionary) + { + var key = keyModelStatePair.Key; + var errors = keyModelStatePair.Value.Errors; + if (errors != null && errors.Count > 0) + { + string errorMessage = null; + foreach (var modelError in errors) + { + if (string.IsNullOrEmpty(modelError.ErrorMessage)) + { + if (modelError.Exception != null) + { + errorMessage = modelError.Exception.Message; + } + } + else + { + errorMessage = modelError.ErrorMessage; + } + + if (errorMessage != null) + { + allErrorMessages.Add(string.Format("{0}:{1}", key, errorMessage)); + } + } + } + } + + return allErrorMessages; + } } } \ No newline at end of file