Treat FormatExceptions and OverflowExceptions to be treated as model state errors

src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs

@@ -8,7 +8,6 @@
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Formatters.Json;
-using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters
@@ -87,6 +86,16 @@ public sealed override async Task<InputFormatterResult> ReadRequestBodyAsync(
 
                 return InputFormatterResult.Failure();
             }
+            catch (Exception exception) when (exception is FormatException || exception is OverflowException)
+            {
+                // The code in System.Text.Json never throws these exceptions. However a custom converter could produce these errors for instance when
+                // parsing a value. These error messages are considered safe to report to users using ModelState.
+
+                context.ModelState.TryAddModelError(string.Empty, exception, context.Metadata);
+                Log.JsonInputException(_logger, exception);
+
+                return InputFormatterResult.Failure();
+            }
             finally
             {
                 if (inputStream is TranscodingReadStream transcoding)

src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs

@@ -334,7 +334,8 @@ public virtual async Task ReadAsync_InvalidArray_AddsOverflowErrorsToModelState(
             // Assert
             Assert.True(result.HasError, "Model should have produced an error!");
             Assert.Collection(formatterContext.ModelState.OrderBy(k => k.Key),
-                kvp => {
+                kvp =>
+                {
                     Assert.Equal(expectedValue, kvp.Key);
                 });
         }

src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs

@@ -5,6 +5,8 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Text;
+using System.Text.Json;
+using System.Text.Json.Serialization;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Xunit;
@@ -81,6 +83,48 @@ public async Task ReadAsync_SingleError()
                 });
         }
 
+        [Fact]
+        public async Task ReadAsync_DoesNotThrowFormatException()
+        {
+            // Arrange
+            var formatter = GetInputFormatter();
+
+            var contentBytes = Encoding.UTF8.GetBytes("{\"dateValue\":\"not-a-date\"}");
+            var httpContext = GetHttpContext(contentBytes);
+
+            var formatterContext = CreateInputFormatterContext(typeof(TypeWithBadConverters), httpContext);
+
+            // Act
+            await formatter.ReadAsync(formatterContext);
+
+            Assert.False(formatterContext.ModelState.IsValid);
+            var kvp = Assert.Single(formatterContext.ModelState);
+            Assert.Empty(kvp.Key);
+            var error = Assert.Single(kvp.Value.Errors);
+            Assert.Equal("The supplied value is invalid.", error.ErrorMessage);
+        }
+
+        [Fact]
+        public async Task ReadAsync_DoesNotThrowOverflowException()
+        {
+            // Arrange
+            var formatter = GetInputFormatter();
+
+            var contentBytes = Encoding.UTF8.GetBytes("{\"shortValue\":\"32768\"}");
+            var httpContext = GetHttpContext(contentBytes);
+
+            var formatterContext = CreateInputFormatterContext(typeof(TypeWithBadConverters), httpContext);
+
+            // Act
+            await formatter.ReadAsync(formatterContext);
+
+            Assert.False(formatterContext.ModelState.IsValid);
+            var kvp = Assert.Single(formatterContext.ModelState);
+            Assert.Empty(kvp.Key);
+            var error = Assert.Single(kvp.Value.Errors);
+            Assert.Equal("The supplied value is invalid.", error.ErrorMessage);
+        }
+
         protected override TextInputFormatter GetInputFormatter()
         {
             return new SystemTextJsonInputFormatter(new JsonOptions(), LoggerFactory.CreateLogger<SystemTextJsonInputFormatter>());
@@ -99,5 +143,40 @@ protected override TextInputFormatter GetInputFormatter()
         internal override string ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected => "$[1].Small";
 
         internal override string ReadAsync_ComplexPoco_Expected => "$.Person.Numbers[2]";
+
+        private class TypeWithBadConverters
+        {
+            [JsonConverter(typeof(DateTimeConverter))]
+            public DateTime DateValue { get; set; }
+
+            [JsonConverter(typeof(ShortConverter))]
+            public short ShortValue { get; set; }
+        }
+
+        private class ShortConverter : JsonConverter<short>
+        {
+            public override short Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+            {
+                return short.Parse(reader.GetString());
+            }
+
+            public override void Write(Utf8JsonWriter writer, short value, JsonSerializerOptions options)
+            {
+                throw new NotImplementedException();
+            }
+        }
+
+        private class DateTimeConverter : JsonConverter<DateTime>
+        {
+            public override DateTime Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+            {
+                return DateTime.Parse(reader.GetString());
+            }
+
+            public override void Write(Utf8JsonWriter writer, DateTime value, JsonSerializerOptions options)
+            {
+                throw new NotImplementedException();
+            }
+        }
     }
 }

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

@@ -196,8 +196,15 @@ public override async Task<InputFormatterResult> ReadRequestBodyAsync(
                 }
             }
 
-            if (!(exception is JsonException || exception is OverflowException))
+            if (!(exception is JsonException || exception is OverflowException || exception is FormatException))
             {
+                // At this point we've already recorded all exceptions as an entry in the ModelStateDictionary.
+                // We only need to rethrow an exception if we believe it needs to be handled by something further up
+                // the stack.
+                // JsonException, OverflowException, and FormatException are assumed to be only encountered when
+                // parsing the JSON and are consequently "safe" to be exposed as part of ModelState. Everything else
+                // needs to be rethrown.
+
                 var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception);
                 exceptionDispatchInfo.Throw();
             }

src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs

@@ -14,15 +14,16 @@
 using Microsoft.Extensions.ObjectPool;
 using Moq;
 using Newtonsoft.Json;
+using Newtonsoft.Json.Converters;
 using Newtonsoft.Json.Serialization;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters
 {
     public class NewtonsoftJsonInputFormatterTest : JsonInputFormatterTestBase
     {
-        private static readonly ObjectPoolProvider _objectPoolProvider = new DefaultObjectPoolProvider();
-        private static readonly JsonSerializerSettings _serializerSettings = new JsonSerializerSettings();
+        private readonly ObjectPoolProvider _objectPoolProvider = new DefaultObjectPoolProvider();
+        private readonly JsonSerializerSettings _serializerSettings = new JsonSerializerSettings();
 
         [Fact]
         public async Task Constructor_BuffersRequestBody_UsingDefaultOptions()
@@ -144,7 +145,7 @@ public void Constructor_UsesSerializerSettings()
             var serializerSettings = new JsonSerializerSettings();
 
             // Act
-            var formatter = new TestableJsonInputFormatter(serializerSettings);
+            var formatter = new TestableJsonInputFormatter(serializerSettings, _objectPoolProvider);
 
             // Assert
             Assert.Same(serializerSettings, formatter.SerializerSettings);
@@ -185,7 +186,7 @@ public void CreateJsonSerializer_UsesJsonSerializerSettings()
                 MaxDepth = 2,
                 DateTimeZoneHandling = DateTimeZoneHandling.RoundtripKind,
             };
-            var formatter = new TestableJsonInputFormatter(settings);
+            var formatter = new TestableJsonInputFormatter(settings, _objectPoolProvider);
 
             // Act
             var actual = formatter.CreateJsonSerializer(null);
@@ -304,7 +305,7 @@ public async Task ReadAsync_DoNotAllowInputFormatterExceptionMessages_DoesNotWra
         }
 
         [Fact]
-        public async Task ReadAsync_lowInputFormatterExceptionMessages_DoesNotWrapJsonInputExceptions()
+        public async Task ReadAsync_AllowInputFormatterExceptionMessages_DoesNotWrapJsonInputExceptions()
         {
             // Arrange
             var formatter = new NewtonsoftJsonInputFormatter(
@@ -336,10 +337,72 @@ public async Task ReadAsync_lowInputFormatterExceptionMessages_DoesNotWrapJsonIn
             Assert.NotEmpty(modelError.ErrorMessage);
         }
 
+        [Fact]
+        public async Task ReadAsync_DoesNotRethrowFormatExceptions()
+        {
+            // Arrange
+            _serializerSettings.Converters.Add(new IsoDateTimeConverter());
+
+            var formatter = new NewtonsoftJsonInputFormatter(
+                GetLogger(),
+                _serializerSettings,
+                ArrayPool<char>.Shared,
+                _objectPoolProvider,
+                new MvcOptions(),
+                new MvcNewtonsoftJsonOptions());
+
+            var contentBytes = Encoding.UTF8.GetBytes("{\"dateValue\":\"not-a-date\"}");
+            var httpContext = GetHttpContext(contentBytes);
+
+            var formatterContext = CreateInputFormatterContext(typeof(TypeWithPrimitives), httpContext);
+
+            // Act
+            var result = await formatter.ReadAsync(formatterContext);
+
+            // Assert
+            Assert.True(result.HasError);
+            Assert.False(formatterContext.ModelState.IsValid);
+
+            var modelError = Assert.Single(formatterContext.ModelState["dateValue"].Errors);
+            Assert.Null(modelError.Exception);
+            Assert.Equal("The supplied value is invalid.", modelError.ErrorMessage);
+        }
+
+        [Fact]
+        public async Task ReadAsync_DoesNotRethrowOverflowExceptions()
+        {
+            // Arrange
+            _serializerSettings.Converters.Add(new IsoDateTimeConverter());
+
+            var formatter = new NewtonsoftJsonInputFormatter(
+                GetLogger(),
+                _serializerSettings,
+                ArrayPool<char>.Shared,
+                _objectPoolProvider,
+                new MvcOptions(),
+                new MvcNewtonsoftJsonOptions());
+
+            var contentBytes = Encoding.UTF8.GetBytes("{\"shortValue\":\"32768\"}");
+            var httpContext = GetHttpContext(contentBytes);
+
+            var formatterContext = CreateInputFormatterContext(typeof(TypeWithPrimitives), httpContext);
+
+            // Act
+            var result = await formatter.ReadAsync(formatterContext);
+
+            // Assert
+            Assert.True(result.HasError);
+            Assert.False(formatterContext.ModelState.IsValid);
+
+            var modelError = Assert.Single(formatterContext.ModelState["shortValue"].Errors);
+            Assert.Null(modelError.Exception);
+            Assert.Equal("The supplied value is invalid.", modelError.ErrorMessage);
+        }
+
         private class TestableJsonInputFormatter : NewtonsoftJsonInputFormatter
         {
-            public TestableJsonInputFormatter(JsonSerializerSettings settings)
-                : base(GetLogger(), settings, ArrayPool<char>.Shared, _objectPoolProvider, new MvcOptions(), new MvcNewtonsoftJsonOptions())
+            public TestableJsonInputFormatter(JsonSerializerSettings settings, ObjectPoolProvider objectPoolProvider)
+                : base(GetLogger(), settings, ArrayPool<char>.Shared, objectPoolProvider, new MvcOptions(), new MvcNewtonsoftJsonOptions())
             {
             }
 
@@ -418,5 +481,26 @@ private sealed class UserLogin
             [JsonProperty(Required = Required.Always)]
             public string Password { get; set; }
         }
+
+        public class TypeWithPrimitives
+        {
+            public DateTime DateValue { get; set; }
+
+            [JsonConverter(typeof(IncorrectShortConverter))]
+            public short ShortValue { get; set; }
+        }
+
+        private class IncorrectShortConverter : JsonConverter<short>
+        {
+            public override short ReadJson(JsonReader reader, Type objectType, short existingValue, bool hasExistingValue, JsonSerializer serializer)
+            {
+                return short.Parse(reader.Value.ToString());
+            }
+
+            public override void WriteJson(JsonWriter writer, short value, JsonSerializer serializer)
+            {
+                throw new NotImplementedException();
+            }
+        }
     }
 }