Better JSON deserialization errors. Implements #4607, #4862

Author: SteveSandersonMS

src/Microsoft.AspNetCore.Mvc.Core/InputFormatterException.cs renamed to src/Microsoft.AspNetCore.Mvc.Abstractions/Formatters/InputFormatterException.cs

@@ -7,6 +7,7 @@
 using System.Diagnostics;
 using System.Runtime.CompilerServices;
 using Microsoft.AspNetCore.Mvc.Abstractions;
+using Microsoft.AspNetCore.Mvc.Formatters;
 using Microsoft.Extensions.Primitives;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding
@@ -263,6 +264,11 @@ public bool TryAddModelError(string key, Exception exception, ModelMetadata meta
 
                 return TryAddModelError(key, errorMessage);
             }
+            else if (exception is InputFormatterException && !string.IsNullOrEmpty(exception.Message))
+            {
+                // InputFormatterException is a signal that the message is safe to expose to clients
+                return TryAddModelError(key, exception.Message);
+            }
 
             ErrorCount++;
             AddModelErrorCore(key, exception);

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs

@@ -170,11 +170,20 @@ public int MaxModelValidationErrors
         public bool AllowBindingUndefinedValueToEnumType { get; set; }
 
         /// <summary>
-        /// Gets or sets the option to determine if model binding should convert all exceptions(including ones not related to bad input)
+        /// Gets or sets the option to determine if model binding should convert all exceptions (including ones not related to bad input)
         /// that occur during deserialization in <see cref="IInputFormatter"/>s into model state errors.
         /// This option applies only to custom <see cref="IInputFormatter"/>s.
         /// Default is <see cref="InputFormatterExceptionModelStatePolicy.AllExceptions"/>.
         /// </summary>
         public InputFormatterExceptionModelStatePolicy InputFormatterExceptionModelStatePolicy { get; set; }
+
+        /// <summary>
+        /// Gets or sets a flag to determine whether, if an action receives invalid JSON in
+        /// the request body, the JSON deserialization exception message should be replaced
+        /// by a generic error message in model state.
+        /// <see langword="false"/> by default, meaning that clients may receive details about
+        /// why the JSON they posted is considered invalid.
+        /// </summary>
+        public bool SuppressJsonDeserializationExceptionMessagesInModelState { get; set; } = false;
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs

@@ -2,6 +2,8 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Runtime.CompilerServices;
+using Microsoft.AspNetCore.Mvc.Formatters;
 
 [assembly: InternalsVisibleTo("Microsoft.AspNetCore.Mvc.Core.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]
 [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
+[assembly: TypeForwardedTo(typeof(InputFormatterException))]

src/Microsoft.AspNetCore.Mvc.Core/Properties/AssemblyInfo.cs

@@ -69,15 +69,17 @@ public void Configure(MvcOptions options)
                 _jsonSerializerSettings,
                 _charPool,
                 _objectPoolProvider,
-                options.SuppressInputFormatterBuffering));
+                options.SuppressInputFormatterBuffering,
+                options.SuppressJsonDeserializationExceptionMessagesInModelState));
 
             var jsonInputLogger = _loggerFactory.CreateLogger<JsonInputFormatter>();
             options.InputFormatters.Add(new JsonInputFormatter(
                 jsonInputLogger,
                 _jsonSerializerSettings,
                 _charPool,
                 _objectPoolProvider,
-                options.SuppressInputFormatterBuffering));
+                options.SuppressInputFormatterBuffering,
+                options.SuppressJsonDeserializationExceptionMessagesInModelState));
 
             options.FormatterMappings.SetMediaTypeMappingForFormat("json", MediaTypeHeaderValue.Parse("application/json"));
 

src/Microsoft.AspNetCore.Mvc.Formatters.Json/Internal/MvcJsonMvcOptionsSetup.cs

@@ -28,6 +28,7 @@ public class JsonInputFormatter : TextInputFormatter, IInputFormatterExceptionPo
         private readonly ILogger _logger;
         private readonly ObjectPoolProvider _objectPoolProvider;
         private readonly bool _suppressInputFormatterBuffering;
+        private readonly bool _suppressJsonDeserializationExceptionMessages;
 
         private ObjectPool<JsonSerializer> _jsonSerializerPool;
 
@@ -70,6 +71,32 @@ public JsonInputFormatter(
             ArrayPool<char> charPool,
             ObjectPoolProvider objectPoolProvider,
             bool suppressInputFormatterBuffering)
+            : this(logger, serializerSettings, charPool, objectPoolProvider, suppressInputFormatterBuffering, suppressJsonDeserializationExceptionMessages: false)
+        {
+            // This constructor by default treats JSON deserialization exceptions as safe
+            // because this is the default for applications generally
+        }
+
+        /// <summary>
+        /// Initializes a new instance of <see cref="JsonInputFormatter"/>.
+        /// </summary>
+        /// <param name="logger">The <see cref="ILogger"/>.</param>
+        /// <param name="serializerSettings">
+        /// The <see cref="JsonSerializerSettings"/>. Should be either the application-wide settings
+        /// (<see cref="MvcJsonOptions.SerializerSettings"/>) or an instance
+        /// <see cref="JsonSerializerSettingsProvider.CreateSerializerSettings"/> initially returned.
+        /// </param>
+        /// <param name="charPool">The <see cref="ArrayPool{Char}"/>.</param>
+        /// <param name="objectPoolProvider">The <see cref="ObjectPoolProvider"/>.</param>
+        /// <param name="suppressInputFormatterBuffering">Flag to buffer entire request body before deserializing it.</param>
+        /// <param name="suppressJsonDeserializationExceptionMessages">If <see langword="true"/>, JSON deserialization exception messages will replaced by a generic message in model state.</param>
+        public JsonInputFormatter(
+            ILogger logger,
+            JsonSerializerSettings serializerSettings,
+            ArrayPool<char> charPool,
+            ObjectPoolProvider objectPoolProvider,
+            bool suppressInputFormatterBuffering,
+            bool suppressJsonDeserializationExceptionMessages)
         {
             if (logger == null)
             {
@@ -96,6 +123,7 @@ public JsonInputFormatter(
             _charPool = new JsonArrayPool<char>(charPool);
             _objectPoolProvider = objectPoolProvider;
             _suppressInputFormatterBuffering = suppressInputFormatterBuffering;
+            _suppressJsonDeserializationExceptionMessages = suppressJsonDeserializationExceptionMessages;
 
             SupportedEncodings.Add(UTF8EncodingWithoutBOM);
             SupportedEncodings.Add(UTF16EncodingLittleEndian);
@@ -187,7 +215,8 @@ void ErrorHandler(object sender, Newtonsoft.Json.Serialization.ErrorEventArgs ev
                         }
 
                         var metadata = GetPathMetadata(context.Metadata, eventArgs.ErrorContext.Path);
-                        context.ModelState.TryAddModelError(key, eventArgs.ErrorContext.Error, metadata);
+                        var modelStateException = WrapExceptionForModelState(eventArgs.ErrorContext.Error);
+                        context.ModelState.TryAddModelError(key, modelStateException, metadata);
 
                         _logger.JsonInputException(eventArgs.ErrorContext.Error);
 
@@ -315,5 +344,19 @@ private ModelMetadata GetPathMetadata(ModelMetadata metadata, string path)
 
             return metadata;
         }
+
+        private Exception WrapExceptionForModelState(Exception exception)
+        {
+            // It's not known that Json.NET currently ever raises error events with exceptions
+            // other than these two types, but we're being conservative and limiting which ones
+            // we regard as having safe messages to expose to clients
+            var isJsonExceptionType =
+                exception is JsonReaderException || exception is JsonSerializationException;
+            var suppressOriginalMessage =
+                _suppressJsonDeserializationExceptionMessages || !isJsonExceptionType;
+            return suppressOriginalMessage
+                ? exception
+                : new InputFormatterException(exception.Message, exception);
+        }
     }
 }

src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonInputFormatter.cs

@@ -27,7 +27,8 @@ public class JsonPatchInputFormatter : JsonInputFormatter
         /// The <see cref="JsonSerializerSettings"/>. Should be either the application-wide settings
         /// (<see cref="MvcJsonOptions.SerializerSettings"/>) or an instance
         /// <see cref="JsonSerializerSettingsProvider.CreateSerializerSettings"/> initially returned.
-        /// </param>/// <param name="charPool">The <see cref="ArrayPool{Char}"/>.</param>
+        /// </param>
+        /// <param name="charPool">The <see cref="ArrayPool{Char}"/>.</param>
         /// <param name="objectPoolProvider">The <see cref="ObjectPoolProvider"/>.</param>
         public JsonPatchInputFormatter(
             ILogger logger,
@@ -46,7 +47,8 @@ public JsonPatchInputFormatter(
         /// The <see cref="JsonSerializerSettings"/>. Should be either the application-wide settings
         /// (<see cref="MvcJsonOptions.SerializerSettings"/>) or an instance
         /// <see cref="JsonSerializerSettingsProvider.CreateSerializerSettings"/> initially returned.
-        /// </param>/// <param name="charPool">The <see cref="ArrayPool{Char}"/>.</param>
+        /// </param>
+        /// <param name="charPool">The <see cref="ArrayPool{Char}"/>.</param>
         /// <param name="objectPoolProvider">The <see cref="ObjectPoolProvider"/>.</param>
         /// <param name="suppressInputFormatterBuffering">Flag to buffer entire request body before deserializing it.</param>
         public JsonPatchInputFormatter(
@@ -55,7 +57,31 @@ public JsonPatchInputFormatter(
             ArrayPool<char> charPool,
             ObjectPoolProvider objectPoolProvider,
             bool suppressInputFormatterBuffering)
-            : base(logger, serializerSettings, charPool, objectPoolProvider, suppressInputFormatterBuffering)
+            : this(logger, serializerSettings, charPool, objectPoolProvider, suppressInputFormatterBuffering, suppressJsonDeserializationExceptionMessages: false)
+        {
+        }
+
+        /// <summary>
+        /// Initializes a new <see cref="JsonPatchInputFormatter"/> instance.
+        /// </summary>
+        /// <param name="logger">The <see cref="ILogger"/>.</param>
+        /// <param name="serializerSettings">
+        /// The <see cref="JsonSerializerSettings"/>. Should be either the application-wide settings
+        /// (<see cref="MvcJsonOptions.SerializerSettings"/>) or an instance
+        /// <see cref="JsonSerializerSettingsProvider.CreateSerializerSettings"/> initially returned.
+        /// </param>
+        /// <param name="charPool">The <see cref="ArrayPool{Char}"/>.</param>
+        /// <param name="objectPoolProvider">The <see cref="ObjectPoolProvider"/>.</param>
+        /// <param name="suppressInputFormatterBuffering">Flag to buffer entire request body before deserializing it.</param>
+        /// <param name="suppressJsonDeserializationExceptionMessages">If <see langword="true"/>, JSON deserialization exception messages will replaced by a generic message in model state.</param>
+        public JsonPatchInputFormatter(
+            ILogger logger,
+            JsonSerializerSettings serializerSettings,
+            ArrayPool<char> charPool,
+            ObjectPoolProvider objectPoolProvider,
+            bool suppressInputFormatterBuffering,
+            bool suppressJsonDeserializationExceptionMessages)
+            : base(logger, serializerSettings, charPool, objectPoolProvider, suppressInputFormatterBuffering, suppressJsonDeserializationExceptionMessages)
         {
             // Clear all values and only include json-patch+json value.
             SupportedMediaTypes.Clear();

src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonPatchInputFormatter.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.AspNetCore.Mvc.Formatters;
 using Microsoft.AspNetCore.Mvc.Internal;
 using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
 using Microsoft.Extensions.Options;
@@ -1005,7 +1006,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithNo
         }
 
         [Fact]
-        public void ModelStateDictionary_NoErrorMessage_ForNonFormatException()
+        public void ModelStateDictionary_NoErrorMessage_ForUnrecognizedException()
         {
             // Arrange
             var dictionary = new ModelStateDictionary();
@@ -1021,6 +1022,28 @@ public void ModelStateDictionary_NoErrorMessage_ForNonFormatException()
             Assert.Empty(error.ErrorMessage);
         }
 
+        [Fact]
+        public void ModelStateDictionary_AddsErrorMessage_ForInputFormatterException()
+        {
+            // Arrange
+            var expectedMessage = "This is an InputFormatterException";
+            var dictionary = new ModelStateDictionary();
+
+            var bindingMetadataProvider = new DefaultBindingMetadataProvider();
+            var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider });
+            var provider = new DefaultModelMetadataProvider(compositeProvider, new OptionsAccessor());
+            var metadata = provider.GetMetadataForType(typeof(int));
+
+            // Act
+            dictionary.TryAddModelError("key", new InputFormatterException(expectedMessage), metadata);
+
+            // Assert
+            var entry = Assert.Single(dictionary);
+            Assert.Equal("key", entry.Key);
+            var error = Assert.Single(entry.Value.Errors);
+            Assert.Equal(expectedMessage, error.ErrorMessage);
+        }
+
         [Fact]
         public void ModelStateDictionary_ClearEntriesThatMatchWithKey_NonEmptyKey()
         {

test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs

@@ -232,10 +232,9 @@ public async Task BindModel_CustomFormatter_ThrowingInputFormatterException_Adds
             // Key is the empty string because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
             Assert.Equal(string.Empty, entry.Key);
-            var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message;
+            var errorMessage = Assert.Single(entry.Value.Errors).ErrorMessage;
             Assert.Equal("Bad input!!", errorMessage);
-            var formatException = Assert.IsType<FormatException>(entry.Value.Errors[0].Exception.InnerException);
-            Assert.Same(expectedFormatException, formatException);
+            Assert.Null(entry.Value.Errors[0].Exception);
         }
 
         public static TheoryData<IInputFormatter, InputFormatterExceptionModelStatePolicy> BuiltInFormattersThrowingInputFormatterException
@@ -282,9 +281,9 @@ public async Task BindModel_BuiltInXmlInputFormatters_ThrowingInputFormatterExce
             // Key is the empty string because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
             Assert.Equal(string.Empty, entry.Key);
-            var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message;
+            var errorMessage = Assert.Single(entry.Value.Errors).ErrorMessage;
             Assert.Equal("An error occured while deserializing input data.", errorMessage);
-            Assert.IsType<InputFormatterException>(entry.Value.Errors[0].Exception);
+            Assert.Null(entry.Value.Errors[0].Exception);
         }
 
         [Theory]
@@ -319,7 +318,7 @@ public async Task BindModel_BuiltInJsonInputFormatter_ThrowingInputFormatterExce
             // Key is the empty string because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
             Assert.Equal(string.Empty, entry.Key);
-            Assert.IsType<JsonReaderException>(entry.Value.Errors[0].Exception);
+            Assert.NotEmpty(entry.Value.Errors[0].ErrorMessage);
         }
 
         public static TheoryData<IInputFormatter, InputFormatterExceptionModelStatePolicy> DerivedFormattersThrowingInputFormatterException
@@ -366,9 +365,9 @@ public async Task BindModel_DerivedXmlInputFormatters_AddsErrorToModelState_(
             // Key is the empty string because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
             Assert.Equal(string.Empty, entry.Key);
-            var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message;
+            var errorMessage = Assert.Single(entry.Value.Errors).ErrorMessage;
             Assert.Equal("An error occured while deserializing input data.", errorMessage);
-            Assert.IsType<InputFormatterException>(entry.Value.Errors[0].Exception);
+            Assert.Null(entry.Value.Errors[0].Exception);
         }
 
         [Theory]
@@ -403,7 +402,8 @@ public async Task BindModel_DerivedJsonInputFormatter_AddsErrorToModelState(
             // Key is the empty string because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
             Assert.Equal(string.Empty, entry.Key);
-            Assert.IsType<JsonReaderException>(entry.Value.Errors[0].Exception);
+            Assert.NotEmpty(entry.Value.Errors[0].ErrorMessage);
+            Assert.Null(entry.Value.Errors[0].Exception);
         }
 
         // Throwing Non-InputFormatterException

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/BodyModelBinderTests.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Xunit;
 
@@ -47,5 +48,30 @@ public void Constructor_SerializesErrorsFromModelStateDictionary()
                     Assert.Equal(new[] { "error2", "error3" }, item.Value);
                 });
         }
+
+        [Fact]
+        public void Constructor_SerializesErrorsFromModelStateDictionary_AddsDefaultMessage()
+        {
+            // Arrange
+            var modelStateDictionary = new ModelStateDictionary();
+            var provider = new EmptyModelMetadataProvider();
+            var metadata = provider.GetMetadataForProperty(typeof(string), nameof(string.Length));
+            modelStateDictionary.AddModelError("unsafeError",
+                new Exception("This message should not be returned to clients"),
+                metadata);
+
+            // Act
+            var problemDescription = new ValidationProblemDetails(modelStateDictionary);
+
+            // Assert
+            Assert.Equal("One or more validation errors occured.", problemDescription.Title);
+            Assert.Collection(
+                problemDescription.Errors,
+                item =>
+                {
+                    Assert.Equal("unsafeError", item.Key);
+                    Assert.Equal(new[] { "The input was not valid." }, item.Value);
+                });
+        }
     }
 }