diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index aeecafa6b1..6b43fdf276 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -8,7 +8,6 @@ using System.Text.Json; 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 index 11c8ce9baf..fd08c4978e 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs @@ -334,7 +334,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters // 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 index 411725c7a9..d3019b0887 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs @@ -5,6 +5,8 @@ using System; 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters }); } + [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()); @@ -99,5 +143,40 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 + { + 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 + { + 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 index 1d30afcbac..2138c5b117 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs @@ -196,8 +196,15 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } } - 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 index d9b1406a05..a5eec9f307 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs @@ -14,6 +14,7 @@ using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.ObjectPool; using Moq; using Newtonsoft.Json; +using Newtonsoft.Json.Converters; using Newtonsoft.Json.Serialization; using Xunit; @@ -21,8 +22,8 @@ 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters 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 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } [Fact] - public async Task ReadAsync_lowInputFormatterExceptionMessages_DoesNotWrapJsonInputExceptions() + public async Task ReadAsync_AllowInputFormatterExceptionMessages_DoesNotWrapJsonInputExceptions() { // Arrange var formatter = new NewtonsoftJsonInputFormatter( @@ -336,10 +337,72 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.NotEmpty(modelError.ErrorMessage); } + [Fact] + public async Task ReadAsync_DoesNotRethrowFormatExceptions() + { + // Arrange + _serializerSettings.Converters.Add(new IsoDateTimeConverter()); + + var formatter = new NewtonsoftJsonInputFormatter( + GetLogger(), + _serializerSettings, + ArrayPool.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.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.Shared, _objectPoolProvider, new MvcOptions(), new MvcNewtonsoftJsonOptions()) + public TestableJsonInputFormatter(JsonSerializerSettings settings, ObjectPoolProvider objectPoolProvider) + : base(GetLogger(), settings, ArrayPool.Shared, objectPoolProvider, new MvcOptions(), new MvcNewtonsoftJsonOptions()) { } @@ -418,5 +481,26 @@ namespace Microsoft.AspNetCore.Mvc.Formatters [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 + { + 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(); + } + } } }