Treat FormatExceptions and OverflowExceptions to be treated as model state errors (#15035)

pranavkm · web-flow · commit c76df96c701e · 2019-10-17T10:06:57.000-07:00
In SystemTextJsonInputFormatter and NewtonsoftJsonInputFormatter, suppress FormatExceptions and OverflowExceptions and instead report them as model state errors. This makes the behavior more consistent between these formatters, model binders, and the XML formatters Fixes #14504
diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/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)
diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/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);
                 });
         }
diff --git a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs b/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();
+            }
+        }
     }
 }
diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/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();
             }
diff --git a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs b/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();
+            }
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -196,8 +196,15 @@ public override async Task<InputFormatterResult> ReadRequestBodyAsync(`
`196`	`196`	`}`
`197`	`197`	`}`
`198`	`198`
`199`		`- if (!(exception is JsonException \|\| exception is OverflowException))`
	`199`	`+ if (!(exception is JsonException \|\| exception is OverflowException \|\| exception is FormatException))`
`200`	`200`	`{`
	`201`	`+ // At this point we've already recorded all exceptions as an entry in the ModelStateDictionary.`
	`202`	`+ // We only need to rethrow an exception if we believe it needs to be handled by something further up`
	`203`	`+ // the stack.`
	`204`	`+ // JsonException, OverflowException, and FormatException are assumed to be only encountered when`
	`205`	`+ // parsing the JSON and are consequently "safe" to be exposed as part of ModelState. Everything else`
	`206`	`+ // needs to be rethrown.`
	`207`	`+`
`201`	`208`	`var exceptionDispatchInfo = ExceptionDispatchInfo.Capture(exception);`
`202`	`209`	`exceptionDispatchInfo.Throw();`
`203`	`210`	`}`