[Fixes #1184] Formatting issues in requests should not cause a 500 response

src/Microsoft.AspNet.Mvc.Core/Formatters/JsonInputFormatter.cs

@@ -57,12 +57,6 @@ namespace Microsoft.AspNet.Mvc
             }
         }
 
-        /// <summary>
-        /// Gets or sets if deserialization errors are captured. When set, these errors appear in
-        /// the <see cref="ActionContext.ModelState"/> instance of <see cref="InputFormatterContext"/>.
-        /// </summary>
-        public bool CaptureDeserilizationErrors { get; set; }
-
         /// <inheritdoc />
         public override Task<object> ReadRequestBodyAsync([NotNull] InputFormatterContext context)
         {
@@ -82,19 +76,17 @@ namespace Microsoft.AspNet.Mvc
                 var jsonSerializer = CreateJsonSerializer();
 
                 EventHandler<Newtonsoft.Json.Serialization.ErrorEventArgs> 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);
-                        var exception = e.ErrorContext.Error;
+
-                        context.ActionContext.ModelState.TryAddModelError(e.ErrorContext.Path, e.ErrorContext.Error);
+                    // Error must always be marked as handled
-                        // Error must always be marked as handled
+                    // Failure to do so can cause the exception to be rethrown at every recursive level and
-                        // Failure to do so can cause the exception to be rethrown at every recursive level and
+                    // overflow the stack for x64 CLR processes
-                        // overflow the stack for x64 CLR processes
+                    e.ErrorContext.Handled = true;
-                        e.ErrorContext.Handled = true;
+                };
-                    };
+                jsonSerializer.Error += errorHandler;
-                    jsonSerializer.Error += errorHandler;
-                }
 
                 try
                 {

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;
+        }
     }
 }

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<InputFormatterContext>()), 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<IInputFormatterSelector>();
             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<object> ReadRequestBodyAsync(InputFormatterContext context)
+            {
+                throw new InvalidOperationException("Your input is bad!");
+            }
+        }
     }
 }

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<JsonReaderException>(() => 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);

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]

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 = "Not a valid xml document";
-            var input = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
-                "<DummyClas xmlns:i=\"http://www.w3.org/2001/XMLSchema-instance\" " +
-                "i:type=\"DerivedDummyClass\" xmlns=\"http://schemas.datacontract.org/2004/07/XmlFormattersWebSite\">" +
-                "<SampleInt>" + sampleInputInt.ToString() + "</SampleInt></DummyClass>";
             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(HttpStatusCode.BadRequest, response.StatusCode);
-            Assert.Equal(typeof(InvalidOperationException).FullName, exception.ExceptionType);
+            var data = await response.Content.ReadAsStringAsync();
+            Assert.Contains("dummyObject:There is an error in XML document", data);
         }
     }
 }

test/WebSites/ModelBindingWebSite/Startup.cs

@@ -29,8 +29,6 @@ namespace ModelBindingWebSite
                             m.MaxModelValidationErrors = 8;
                             m.ModelBinders.Insert(0, typeof(TestBindingSourceModelBinder));
 
-                            m.InputFormatters.InstanceOf<JsonInputFormatter>().CaptureDeserilizationErrors = true;
-
                             m.AddXmlDataContractSerializerFormatter();
                             m.ValidationExcludeFilters.Add(typeof(Address));
                         });

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<string> GetModelStateErrorMessages(ModelStateDictionary modelStateDictionary)
+        {
+            var allErrorMessages = new List<string>();
+            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;
+        }
     }
 }