diff --git a/src/Hosting/Hosting/src/Properties/Resources.Designer.cs b/src/Hosting/Hosting/src/Properties/Resources.Designer.cs index 088072729c..4dcdd4b244 100644 --- a/src/Hosting/Hosting/src/Properties/Resources.Designer.cs +++ b/src/Hosting/Hosting/src/Properties/Resources.Designer.cs @@ -15,64 +15,56 @@ namespace Microsoft.AspNetCore.Hosting /// internal static string ErrorPageHtml_Title { - get { return GetString("ErrorPageHtml_Title"); } + get => GetString("ErrorPageHtml_Title"); } /// /// Internal Server Error /// internal static string FormatErrorPageHtml_Title() - { - return GetString("ErrorPageHtml_Title"); - } + => GetString("ErrorPageHtml_Title"); /// /// An error occurred while starting the application. /// internal static string ErrorPageHtml_UnhandledException { - get { return GetString("ErrorPageHtml_UnhandledException"); } + get => GetString("ErrorPageHtml_UnhandledException"); } /// /// An error occurred while starting the application. /// internal static string FormatErrorPageHtml_UnhandledException() - { - return GetString("ErrorPageHtml_UnhandledException"); - } + => GetString("ErrorPageHtml_UnhandledException"); /// /// Unknown location /// internal static string ErrorPageHtml_UnknownLocation { - get { return GetString("ErrorPageHtml_UnknownLocation"); } + get => GetString("ErrorPageHtml_UnknownLocation"); } /// /// Unknown location /// internal static string FormatErrorPageHtml_UnknownLocation() - { - return GetString("ErrorPageHtml_UnknownLocation"); - } + => GetString("ErrorPageHtml_UnknownLocation"); /// /// WebHostBuilder allows creation only of a single instance of WebHost /// internal static string WebHostBuilder_SingleInstance { - get { return GetString("WebHostBuilder_SingleInstance"); } + get => GetString("WebHostBuilder_SingleInstance"); } /// /// WebHostBuilder allows creation only of a single instance of WebHost /// internal static string FormatWebHostBuilder_SingleInstance() - { - return GetString("WebHostBuilder_SingleInstance"); - } + => GetString("WebHostBuilder_SingleInstance"); private static string GetString(string name, params string[] formatterNames) { diff --git a/src/Middleware/Localization/src/Properties/Resources.Designer.cs b/src/Middleware/Localization/src/Properties/Resources.Designer.cs index 581e3cda17..49608c3eaa 100644 --- a/src/Middleware/Localization/src/Properties/Resources.Designer.cs +++ b/src/Middleware/Localization/src/Properties/Resources.Designer.cs @@ -1,6 +1,7 @@ // namespace Microsoft.AspNetCore.Localization { + using System.Globalization; using System.Reflection; using System.Resources; @@ -14,9 +15,15 @@ namespace Microsoft.AspNetCore.Localization /// internal static string Exception_CulturesShouldNotBeEmpty { - get { return GetString("Exception_CulturesShouldNotBeEmpty"); } + get => GetString("Exception_CulturesShouldNotBeEmpty"); } + /// + /// Please provide at least one culture. + /// + internal static string FormatException_CulturesShouldNotBeEmpty() + => GetString("Exception_CulturesShouldNotBeEmpty"); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Middleware/Session/src/Properties/Resources.Designer.cs b/src/Middleware/Session/src/Properties/Resources.Designer.cs index d071f6b698..425ab7bc39 100644 --- a/src/Middleware/Session/src/Properties/Resources.Designer.cs +++ b/src/Middleware/Session/src/Properties/Resources.Designer.cs @@ -15,96 +15,84 @@ namespace Microsoft.AspNetCore.Session /// internal static string Exception_KeyLengthIsExceeded { - get { return GetString("Exception_KeyLengthIsExceeded"); } + get => GetString("Exception_KeyLengthIsExceeded"); } /// /// The key cannot be longer than '{0}' when encoded with UTF-8. /// internal static string FormatException_KeyLengthIsExceeded(object p0) - { - return string.Format(CultureInfo.CurrentCulture, GetString("Exception_KeyLengthIsExceeded"), p0); - } + => string.Format(CultureInfo.CurrentCulture, GetString("Exception_KeyLengthIsExceeded"), p0); /// /// The session cannot be established after the response has started. /// internal static string Exception_InvalidSessionEstablishment { - get { return GetString("Exception_InvalidSessionEstablishment"); } + get => GetString("Exception_InvalidSessionEstablishment"); } /// /// The session cannot be established after the response has started. /// internal static string FormatException_InvalidSessionEstablishment() - { - return GetString("Exception_InvalidSessionEstablishment"); - } + => GetString("Exception_InvalidSessionEstablishment"); /// /// The value cannot be serialized in two bytes. /// internal static string Exception_InvalidToSerializeIn2Bytes { - get { return GetString("Exception_InvalidToSerializeIn2Bytes"); } + get => GetString("Exception_InvalidToSerializeIn2Bytes"); } /// /// The value cannot be serialized in two bytes. /// internal static string FormatException_InvalidToSerializeIn2Bytes() - { - return GetString("Exception_InvalidToSerializeIn2Bytes"); - } + => GetString("Exception_InvalidToSerializeIn2Bytes"); /// /// The value cannot be serialized in three bytes. /// internal static string Exception_InvalidToSerializeIn3Bytes { - get { return GetString("Exception_InvalidToSerializeIn3Bytes"); } + get => GetString("Exception_InvalidToSerializeIn3Bytes"); } /// /// The value cannot be serialized in three bytes. /// internal static string FormatException_InvalidToSerializeIn3Bytes() - { - return GetString("Exception_InvalidToSerializeIn3Bytes"); - } + => GetString("Exception_InvalidToSerializeIn3Bytes"); /// /// The value cannot be negative. /// internal static string Exception_NumberShouldNotBeNegative { - get { return GetString("Exception_NumberShouldNotBeNegative"); } + get => GetString("Exception_NumberShouldNotBeNegative"); } /// /// The value cannot be negative. /// internal static string FormatException_NumberShouldNotBeNegative() - { - return GetString("Exception_NumberShouldNotBeNegative"); - } + => GetString("Exception_NumberShouldNotBeNegative"); /// /// Argument cannot be null or empty string. /// internal static string ArgumentCannotBeNullOrEmpty { - get { return GetString("ArgumentCannotBeNullOrEmpty"); } + get => GetString("ArgumentCannotBeNullOrEmpty"); } /// /// Argument cannot be null or empty string. /// internal static string FormatArgumentCannotBeNullOrEmpty() - { - return GetString("ArgumentCannotBeNullOrEmpty"); - } + => GetString("ArgumentCannotBeNullOrEmpty"); private static string GetString(string name, params string[] formatterNames) { diff --git a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs index 010d18a5b2..95f80b1812 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/DefaultApiDescriptionProviderTest.cs @@ -191,8 +191,6 @@ namespace Microsoft.AspNetCore.Mvc.Description var action = CreateActionDescriptor(nameof(FromRouting)); action.AttributeRouteInfo = new AttributeRouteInfo { Template = template }; - var parameterDescriptor = action.Parameters[0]; - // Act var descriptions = GetApiDescriptions(action); @@ -1511,7 +1509,6 @@ namespace Microsoft.AspNetCore.Mvc.Description { // Arrange var action = CreateActionDescriptor(nameof(AcceptsHasCollection)); - var parameterDescriptor = action.Parameters.Single(); // Act var descriptions = GetApiDescriptions(action); @@ -1531,7 +1528,6 @@ namespace Microsoft.AspNetCore.Mvc.Description { // Arrange var action = CreateActionDescriptor(nameof(AcceptsHasCollection_Complex)); - var parameterDescriptor = action.Parameters.Single(); // Act var descriptions = GetApiDescriptions(action); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinder.cs index 29705d88a6..035a0d0f80 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinder.cs @@ -52,6 +52,36 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { } + /// + /// Creates a new . + /// + /// + /// The for binding . + /// + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + /// The . + /// + /// This is the preferred constructor. + /// + /// The parameter is currently ignored. + /// is always + /// in . + /// + /// + public ArrayModelBinder( + IModelBinder elementBinder, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes, + MvcOptions mvcOptions) + : base(elementBinder, loggerFactory, allowValidatingTopLevelNodes: true, mvcOptions) + { + } + /// public override bool CanCreateInstance(Type targetType) { diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinderProvider.cs index 4eb195b7ab..eacedb17ba 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/ArrayModelBinderProvider.cs @@ -4,6 +4,7 @@ using System; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { @@ -23,15 +24,17 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders if (context.Metadata.ModelType.IsArray) { var elementType = context.Metadata.ElementMetadata.ModelType; + var binderType = typeof(ArrayModelBinder<>).MakeGenericType(elementType); var elementBinder = context.CreateBinder(context.Metadata.ElementMetadata); - var binderType = typeof(ArrayModelBinder<>).MakeGenericType(elementType); var loggerFactory = context.Services.GetRequiredService(); + var mvcOptions = context.Services.GetRequiredService>().Value; return (IModelBinder)Activator.CreateInstance( binderType, elementBinder, loggerFactory, - true /* allowValidatingTopLevelNodes */); + true /* allowValidatingTopLevelNodes */, + mvcOptions); } return null; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs index 8746cd487e..38955e2a9c 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs @@ -9,6 +9,8 @@ using System.Linq; using System.Linq.Expressions; using System.Reflection; using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Core; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.Logging; @@ -21,12 +23,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public class CollectionModelBinder : ICollectionModelBinder { private static readonly IValueProvider EmptyValueProvider = new CompositeValueProvider(); + private readonly int _maxModelBindingCollectionSize = MvcOptions.DefaultMaxModelBindingCollectionSize; private Func _modelCreator; /// /// Creates a new . /// - /// The for binding elements. + /// + /// The for binding . + /// /// The . public CollectionModelBinder(IModelBinder elementBinder, ILoggerFactory loggerFactory) : this(elementBinder, loggerFactory, allowValidatingTopLevelNodes: true) @@ -36,7 +41,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders /// /// Creates a new . /// - /// The for binding elements. + /// + /// The for binding . + /// /// The . /// /// Indication that validation of top-level models is enabled. If and @@ -63,6 +70,35 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders AllowValidatingTopLevelNodes = allowValidatingTopLevelNodes; } + /// + /// Creates a new . + /// + /// + /// The for binding . + /// + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + /// The . + /// This is the preferred constructor. + public CollectionModelBinder( + IModelBinder elementBinder, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes, + MvcOptions mvcOptions) + : this(elementBinder, loggerFactory, allowValidatingTopLevelNodes) + { + if (mvcOptions == null) + { + throw new ArgumentNullException(nameof(mvcOptions)); + } + + _maxModelBindingCollectionSize = mvcOptions.MaxModelBindingCollectionSize; + } + // Internal for testing. internal bool AllowValidatingTopLevelNodes { get; } @@ -305,8 +341,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders else { indexNamesIsFinite = false; - indexNames = Enumerable.Range(0, int.MaxValue) - .Select(i => i.ToString(CultureInfo.InvariantCulture)); + var limit = _maxModelBindingCollectionSize == int.MaxValue ? + int.MaxValue : + _maxModelBindingCollectionSize + 1; + indexNames = Enumerable + .Range(0, limit) + .Select(i => i.ToString(CultureInfo.InvariantCulture)); } var elementMetadata = bindingContext.ModelMetadata.ElementMetadata; @@ -345,6 +385,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders boundCollection.Add(ModelBindingHelper.CastOrDefault(boundValue)); } + // Did the collection grow larger than the limit? + if (boundCollection.Count > _maxModelBindingCollectionSize) + { + // Look for a non-empty name. Both ModelName and OriginalModelName may be empty at the top level. + var name = string.IsNullOrEmpty(bindingContext.ModelName) ? + (string.IsNullOrEmpty(bindingContext.OriginalModelName) && + bindingContext.ModelMetadata.MetadataKind != ModelMetadataKind.Type ? + bindingContext.ModelMetadata.Name : + bindingContext.OriginalModelName) : // This name may unfortunately be empty. + bindingContext.ModelName; + + throw new InvalidOperationException(Resources.FormatModelBinding_ExceededMaxModelBindingCollectionSize( + name, + nameof(MvcOptions), + nameof(MvcOptions.MaxModelBindingCollectionSize), + _maxModelBindingCollectionSize, + bindingContext.ModelMetadata.ElementType)); + } + return new CollectionResult { Model = boundCollection, diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinderProvider.cs index 6e66eb602d..1b59e77c5a 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinderProvider.cs @@ -32,22 +32,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return null; } - var loggerFactory = context.Services.GetRequiredService(); - var mvcOptions = context.Services.GetRequiredService>().Value; - // If the model type is ICollection<> then we can call its Add method, so we can always support it. var collectionType = ClosedGenericMatcher.ExtractGenericInterface(modelType, typeof(ICollection<>)); if (collectionType != null) { - var elementType = collectionType.GenericTypeArguments[0]; - var elementBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(elementType)); - - var binderType = typeof(CollectionModelBinder<>).MakeGenericType(collectionType.GenericTypeArguments); - return (IModelBinder)Activator.CreateInstance( - binderType, - elementBinder, - loggerFactory, - true /* allowValidatingTopLevelNodes */); + return CreateInstance(context, collectionType); } // If the model type is IEnumerable<> then we need to know if we can assign a List<> to it, since @@ -59,19 +48,29 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var listType = typeof(List<>).MakeGenericType(enumerableType.GenericTypeArguments); if (modelType.GetTypeInfo().IsAssignableFrom(listType.GetTypeInfo())) { - var elementType = enumerableType.GenericTypeArguments[0]; - var elementBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(elementType)); - - var binderType = typeof(CollectionModelBinder<>).MakeGenericType(enumerableType.GenericTypeArguments); - return (IModelBinder)Activator.CreateInstance( - binderType, - elementBinder, - loggerFactory, - true /* allowValidatingTopLevelNodes */); + return CreateInstance(context, listType); } } return null; } + + private static IModelBinder CreateInstance(ModelBinderProviderContext context, Type collectionType) + { + var binderType = typeof(CollectionModelBinder<>).MakeGenericType(collectionType.GenericTypeArguments); + var elementType = collectionType.GenericTypeArguments[0]; + var elementBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(elementType)); + + var loggerFactory = context.Services.GetRequiredService(); + var mvcOptions = context.Services.GetRequiredService>().Value; + var binder = (IModelBinder)Activator.CreateInstance( + binderType, + elementBinder, + loggerFactory, + true /* allowValidatingTopLevelNodes */, + mvcOptions); + + return binder; + } } } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinder.cs index 412cfd59af..ad5e5207fe 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinder.cs @@ -74,6 +74,48 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders _valueBinder = valueBinder; } + /// + /// Creates a new . + /// + /// The for . + /// The for . + /// The . + /// + /// Indication that validation of top-level models is enabled. If and + /// is for a top-level model, the binder + /// adds a error when the model is not bound. + /// + /// The . + /// + /// This is the preferred constructor. + /// + /// The parameter is currently ignored. + /// is always + /// in . This class ignores that + /// property and unconditionally checks for unbound top-level models with + /// . + /// + /// + public DictionaryModelBinder( + IModelBinder keyBinder, + IModelBinder valueBinder, + ILoggerFactory loggerFactory, + bool allowValidatingTopLevelNodes, + MvcOptions mvcOptions) + : base( + new KeyValuePairModelBinder(keyBinder, valueBinder, loggerFactory), + loggerFactory, + allowValidatingTopLevelNodes: false, + mvcOptions) + { + if (valueBinder == null) + { + throw new ArgumentNullException(nameof(valueBinder)); + } + + _valueBinder = valueBinder; + } + /// public override async Task BindModelAsync(ModelBindingContext bindingContext) { diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinderProvider.cs index 6958dc7370..8d37e5b04a 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DictionaryModelBinderProvider.cs @@ -27,13 +27,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var dictionaryType = ClosedGenericMatcher.ExtractGenericInterface(modelType, typeof(IDictionary<,>)); if (dictionaryType != null) { + var binderType = typeof(DictionaryModelBinder<,>).MakeGenericType(dictionaryType.GenericTypeArguments); + var keyType = dictionaryType.GenericTypeArguments[0]; var keyBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(keyType)); var valueType = dictionaryType.GenericTypeArguments[1]; var valueBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(valueType)); - var binderType = typeof(DictionaryModelBinder<,>).MakeGenericType(dictionaryType.GenericTypeArguments); var loggerFactory = context.Services.GetRequiredService(); var mvcOptions = context.Services.GetRequiredService>().Value; return (IModelBinder)Activator.CreateInstance( @@ -41,7 +42,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders keyBinder, valueBinder, loggerFactory, - true /* allowValidatingTopLevelNodes */); + true /* allowValidatingTopLevelNodes */, + mvcOptions); } return null; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/DefaultModelBindingContext.cs b/src/Mvc/Mvc.Core/src/ModelBinding/DefaultModelBindingContext.cs index c57992ceed..38d9dfd0a7 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/DefaultModelBindingContext.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/DefaultModelBindingContext.cs @@ -3,7 +3,10 @@ using System; using System.Collections.Generic; +using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Mvc.ModelBinding { @@ -18,6 +21,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private ActionContext _actionContext; private ModelStateDictionary _modelState; private ValidationStateDictionary _validationState; + private int? _maxModelBindingRecursionDepth; private State _state; private readonly Stack _stack = new Stack(); @@ -184,6 +188,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } } + private int MaxModelBindingRecursionDepth + { + get + { + if (!_maxModelBindingRecursionDepth.HasValue) + { + // Ignore incomplete initialization. This must be a test scenario because CreateBindingContext(...) + // has not been called or was called without MvcOptions in the service provider. + _maxModelBindingRecursionDepth = MvcOptions.DefaultMaxModelBindingRecursionDepth; + } + + return _maxModelBindingRecursionDepth.Value; + } + set + { + _maxModelBindingRecursionDepth = value; + } + } + /// /// Creates a new for top-level model binding operation. /// @@ -223,16 +246,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } var binderModelName = bindingInfo?.BinderModelName ?? metadata.BinderModelName; + var bindingSource = bindingInfo?.BindingSource ?? metadata.BindingSource; var propertyFilterProvider = bindingInfo?.PropertyFilterProvider ?? metadata.PropertyFilterProvider; - var bindingSource = bindingInfo?.BindingSource ?? metadata.BindingSource; - - return new DefaultModelBindingContext() + var bindingContext = new DefaultModelBindingContext() { ActionContext = actionContext, BinderModelName = binderModelName, BindingSource = bindingSource, PropertyFilter = propertyFilterProvider?.PropertyFilter, + ValidationState = new ValidationStateDictionary(), // Because this is the top-level context, FieldName and ModelName should be the same. FieldName = binderModelName ?? modelName, @@ -245,9 +268,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding OriginalValueProvider = valueProvider, ValueProvider = FilterValueProvider(valueProvider, bindingSource), - - ValidationState = new ValidationStateDictionary(), }; + + // mvcOptions may be null when this method is called in test scenarios. + var mvcOptions = actionContext.HttpContext.RequestServices?.GetService>(); + if (mvcOptions != null) + { + bindingContext.MaxModelBindingRecursionDepth = mvcOptions.Value.MaxModelBindingRecursionDepth; + } + + return bindingContext; } /// @@ -299,6 +329,21 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { _stack.Push(_state); + // Would this new scope (which isn't in _stack) exceed the allowed recursion depth? That is, has the model + // binding system already nested MaxModelBindingRecursionDepth binders? + if (_stack.Count >= MaxModelBindingRecursionDepth) + { + // Find the root of this deeply-nested model. + var states = _stack.ToArray(); + var rootModelType = states[states.Length - 1].ModelMetadata.ModelType; + + throw new InvalidOperationException(Resources.FormatModelBinding_ExceededMaxModelBindingRecursionDepth( + nameof(MvcOptions), + nameof(MvcOptions.MaxModelBindingRecursionDepth), + MaxModelBindingRecursionDepth, + rootModelType)); + } + Result = default; return new NestedScope(this); diff --git a/src/Mvc/Mvc.Core/src/MvcOptions.cs b/src/Mvc/Mvc.Core/src/MvcOptions.cs index 19626413b2..c05fdcfd21 100644 --- a/src/Mvc/Mvc.Core/src/MvcOptions.cs +++ b/src/Mvc/Mvc.Core/src/MvcOptions.cs @@ -4,15 +4,16 @@ using System; using System.Collections; using System.Collections.Generic; -using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Binders; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Microsoft.AspNetCore.WebUtilities; namespace Microsoft.AspNetCore.Mvc { @@ -21,8 +22,14 @@ namespace Microsoft.AspNetCore.Mvc /// public class MvcOptions : IEnumerable { + internal const int DefaultMaxModelBindingCollectionSize = FormReader.DefaultValueCountLimit; + internal const int DefaultMaxModelBindingRecursionDepth = 32; + private readonly IReadOnlyList _switches = Array.Empty(); + private int _maxModelStateErrors = ModelStateDictionary.DefaultMaxAllowedErrors; + private int _maxModelBindingCollectionSize = DefaultMaxModelBindingCollectionSize; + private int _maxModelBindingRecursionDepth = DefaultMaxModelBindingRecursionDepth; private int? _maxValidationDepth = 32; /// @@ -231,6 +238,85 @@ namespace Microsoft.AspNetCore.Mvc /// public bool SuppressAsyncSuffixInActionNames { get; set; } = true; + /// + /// Gets or sets the maximum size of a complex collection to model bind. When this limit is reached, the model + /// binding system will throw an . + /// + /// + /// + /// When binding a collection, some element binders may succeed unconditionally and model binding may run out + /// of memory. This limit constrains such unbounded collection growth; it is a safeguard against incorrect + /// model binders and models. + /// + /// + /// This limit does not correct the bound model. The instead + /// informs the developer of an issue in their model or model binder. The developer must correct that issue. + /// + /// + /// This limit does not apply to collections of simple types. When + /// relies entirely on s, it cannot + /// create collections larger than the available data. + /// + /// + /// A very high value for this option (int.MaxValue for example) effectively removes the limit and is + /// not recommended. + /// + /// + /// The default value is 1024, matching . + public int MaxModelBindingCollectionSize + { + get => _maxModelBindingCollectionSize; + set + { + // Disallowing an empty collection would cause the CollectionModelBinder to throw unconditionally. + if (value <= 0) + { + throw new ArgumentOutOfRangeException(nameof(value)); + } + + _maxModelBindingCollectionSize = value; + } + } + + /// + /// Gets or sets the maximum recursion depth of the model binding system. The + /// will throw an if more than + /// this number of s are on the stack. That is, an attempt to recurse beyond this + /// level will fail. + /// + /// + /// + /// For some self-referential models, some binders may succeed unconditionally and model binding may result in + /// stack overflow. This limit constrains such unbounded recursion; it is a safeguard against incorrect model + /// binders and models. This limit also protects against very deep model type hierarchies lacking + /// self-references. + /// + /// + /// This limit does not correct the bound model. The instead + /// informs the developer of an issue in their model. The developer must correct that issue. + /// + /// + /// A very high value for this option (int.MaxValue for example) effectively removes the limit and is + /// not recommended. + /// + /// + /// The default value is 32, matching the default value. + public int MaxModelBindingRecursionDepth + { + get => _maxModelBindingRecursionDepth; + set + { + // Disallowing one model binder (if supported) would cause the model binding system to throw + // unconditionally. DefaultModelBindingContext always allows a top-level binder i.e. its own creation. + if (value <= 1) + { + throw new ArgumentOutOfRangeException(nameof(value)); + } + + _maxModelBindingRecursionDepth = value; + } + } + IEnumerator IEnumerable.GetEnumerator() => _switches.GetEnumerator(); IEnumerator IEnumerable.GetEnumerator() => _switches.GetEnumerator(); diff --git a/src/Mvc/Mvc.Core/src/Properties/Resources.Designer.cs b/src/Mvc/Mvc.Core/src/Properties/Resources.Designer.cs index 0aff98e8f3..11a2b28a4e 100644 --- a/src/Mvc/Mvc.Core/src/Properties/Resources.Designer.cs +++ b/src/Mvc/Mvc.Core/src/Properties/Resources.Designer.cs @@ -1704,6 +1704,34 @@ namespace Microsoft.AspNetCore.Mvc.Core internal static string FormatReferenceToNewtonsoftJsonRequired(object p0, object p1, object p2, object p3, object p4) => string.Format(CultureInfo.CurrentCulture, GetString("ReferenceToNewtonsoftJsonRequired"), p0, p1, p2, p3, p4); + /// + /// Collection bound to '{0}' exceeded {1}.{2} ({3}). This limit is a safeguard against incorrect model binders and models. Address issues in '{4}'. For example, this type may have a property with a model binder that always succeeds. See the {0}.{1} documentation for more information. + /// + internal static string ModelBinding_ExceededMaxModelBindingCollectionSize + { + get => GetString("ModelBinding_ExceededMaxModelBindingCollectionSize"); + } + + /// + /// Collection bound to '{0}' exceeded {1}.{2} ({3}). This limit is a safeguard against incorrect model binders and models. Address issues in '{4}'. For example, this type may have a property with a model binder that always succeeds. See the {0}.{1} documentation for more information. + /// + internal static string FormatModelBinding_ExceededMaxModelBindingCollectionSize(object p0, object p1, object p2, object p3, object p4) + => string.Format(CultureInfo.CurrentCulture, GetString("ModelBinding_ExceededMaxModelBindingCollectionSize"), p0, p1, p2, p3, p4); + + /// + /// Model binding system exceeded {0}.{1} ({2}). Reduce the potential nesting of '{3}'. For example, this type may have a property with a model binder that always succeeds. See the {0}.{1} documentation for more information. + /// + internal static string ModelBinding_ExceededMaxModelBindingRecursionDepth + { + get => GetString("ModelBinding_ExceededMaxModelBindingRecursionDepth"); + } + + /// + /// Model binding system exceeded {0}.{1} ({2}). Reduce the potential nesting of '{3}'. For example, this type may have a property with a model binder that always succeeds. See the {0}.{1} documentation for more information. + /// + internal static string FormatModelBinding_ExceededMaxModelBindingRecursionDepth(object p0, object p1, object p2, object p3) + => string.Format(CultureInfo.CurrentCulture, GetString("ModelBinding_ExceededMaxModelBindingRecursionDepth"), p0, p1, p2, p3); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Mvc/Mvc.Core/src/Resources.resx b/src/Mvc/Mvc.Core/src/Resources.resx index 820d111536..181d9aa923 100644 --- a/src/Mvc/Mvc.Core/src/Resources.resx +++ b/src/Mvc/Mvc.Core/src/Resources.resx @@ -1,17 +1,17 @@  - @@ -493,4 +493,12 @@ '{0}' requires a reference to '{1}'. Configure your application by adding a reference to the '{1}' package and calling '{2}.{3}' inside the call to '{4}' in the application startup code. + + Collection bound to '{0}' exceeded {1}.{2} ({3}). This limit is a safeguard against incorrect model binders and models. Address issues in '{4}'. For example, this type may have a property with a model binder that always succeeds. See the {1}.{2} documentation for more information. + {0} is model name, {1} is MvcOptions, {2} is MaxModelBindingCollectionSize, {3} is option value, {4} is affected type. + + + Model binding system exceeded {0}.{1} ({2}). Reduce the potential nesting of '{3}'. For example, this type may have a property with a model binder that always succeeds. See the {0}.{1} documentation for more information. + {0} is MvcOptions, {1} is MaxModelBindingRecursionDepth, {2} is option value, {3} is (loopy or deeply nested) top-level model type. + \ No newline at end of file diff --git a/src/Mvc/test/Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs index 921fa1f03f..698ab0d2e5 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.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 System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -369,5 +370,38 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests (e) => Assert.Equal("Alias1", e), (e) => Assert.Equal("Alias2", e)); } + + [Fact] + public async Task ArrayModelBinder_ThrowsOn1025Items_AtTopLevel() + { + // Arrange + var expectedMessage = $"Collection bound to 'parameter' exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} (1024). This limit is a " + + $"safeguard against incorrect model binders and models. Address issues in " + + $"'{typeof(SuccessfulModel)}'. For example, this type may have a property with a model binder that " + + $"always succeeds. See the {nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} " + + $"documentation for more information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(SuccessfulModel[]), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + // CollectionModelBinder binds an empty collection when value providers are all empty. + request.QueryString = new QueryString("?a=b"); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } } -} \ No newline at end of file +} diff --git a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs index aec3949a3d..ebe8c73ce9 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -968,6 +968,164 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(0, modelState.ErrorCount); } + // Regression test for #7052 + [Fact] + public async Task CollectionModelBinder_ThrowsOn1025Items_AtTopLevel() + { + // Arrange + var expectedMessage = $"Collection bound to 'parameter' exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} (1024). This limit is a " + + $"safeguard against incorrect model binders and models. Address issues in " + + $"'{typeof(SuccessfulModel)}'. For example, this type may have a property with a model binder that " + + $"always succeeds. See the {nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} " + + $"documentation for more information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(IList), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + // CollectionModelBinder binds an empty collection when value providers are all empty. + request.QueryString = new QueryString("?a=b"); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + + // Ensure CollectionModelBinder allows MaxModelBindingCollectionSize items. + [Fact] + public async Task CollectionModelBinder_Binds3Items_WithIndices() + { + // Arrange + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(IList), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => + { + request.QueryString = new QueryString("?Index=0&Index=1&Index=2"); + }, + options => options.MaxModelBindingCollectionSize = 3); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act + var result = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelState.IsValid); + Assert.Equal(0, modelState.ErrorCount); + + Assert.True(result.IsModelSet); + var locations = Assert.IsType>(result.Model); + Assert.Collection( + locations, + item => + { + Assert.True(item.IsBound); + Assert.Null(item.Name); + }, + item => + { + Assert.True(item.IsBound); + Assert.Null(item.Name); + }, + item => + { + Assert.True(item.IsBound); + Assert.Null(item.Name); + }); + } + + // Ensure CollectionModelBinder disallows one more than MaxModelBindingCollectionSize items. + [Fact] + public async Task CollectionModelBinder_ThrowsOn4Items_WithIndices() + { + // Arrange + var expectedMessage = $"Collection bound to 'parameter' exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} (3). This limit is a " + + $"safeguard against incorrect model binders and models. Address issues in " + + $"'{typeof(SuccessfulModel)}'. For example, this type may have a property with a model binder that " + + $"always succeeds. See the {nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} " + + $"documentation for more information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(IList), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => + { + request.QueryString = new QueryString("?Index=0&Index=1&Index=2&Index=3"); + }, + options => options.MaxModelBindingCollectionSize = 3); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + + private class SuccessfulContainer + { + public IList Successes { get; set; } + } + + [Fact] + public async Task CollectionModelBinder_ThrowsOn1025Items() + { + // Arrange + var expectedMessage = $"Collection bound to 'Successes' exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} (1024). This limit is a " + + $"safeguard against incorrect model binders and models. Address issues in " + + $"'{typeof(SuccessfulModel)}'. For example, this type may have a property with a model binder that " + + $"always succeeds. See the {nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} " + + $"documentation for more information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(SuccessfulContainer), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + // CollectionModelBinder binds an empty collection when value providers lack matching data. + request.QueryString = new QueryString("?Successes[0]=b"); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + private class ClosedGenericCollection : Collection { } @@ -978,7 +1136,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class ExplicitClosedGenericCollection : ICollection { - private List _data = new List(); + private readonly List _data = new List(); int ICollection.Count { @@ -1034,7 +1192,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class ExplicitClosedGenericList : IList { - private List _data = new List(); + private readonly List _data = new List(); string IList.this[int index] { @@ -1118,7 +1276,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class ExplicitCollection : ICollection { - private List _data = new List(); + private readonly List _data = new List(); int ICollection.Count { @@ -1174,7 +1332,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class ExplicitList : IList { - private List _data = new List(); + private readonly List _data = new List(); T IList.this[int index] { diff --git a/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs index 72219b7ad8..39d4a8c01a 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs @@ -3411,6 +3411,181 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.True(modelState.IsValid); } + private class LoopyModel + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public LoopyModel SelfReference { get; set; } + } + + // Regression test for #7052 + [Fact] + public async Task ModelBindingSystem_ThrowsOn33Binders() + { + // Arrange + var expectedMessage = $"Model binding system exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingRecursionDepth)} (32). Reduce the " + + $"potential nesting of '{typeof(LoopyModel)}'. For example, this type may have a property with a " + + $"model binder that always succeeds. See the " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingRecursionDepth)} documentation for more " + + $"information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(LoopyModel), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + + private class TwoDeepModel + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + } + + private class ThreeDeepModel + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public TwoDeepModel Inner { get; set; } + } + + // Ensure model binding system allows MaxModelBindingRecursionDepth binders on the stack. + [Fact] + public async Task ModelBindingSystem_BindsWith3Binders() + { + // Arrange + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(ThreeDeepModel), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + updateOptions: options => options.MaxModelBindingRecursionDepth = 3); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act + var result = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelState.IsValid); + Assert.Equal(0, modelState.ErrorCount); + + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + Assert.True(model.IsBound); + Assert.NotNull(model.Inner); + Assert.True(model.Inner.IsBound); + } + + private class FourDeepModel + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public ThreeDeepModel Inner { get; set; } + } + + // Ensure model binding system disallows one more than MaxModelBindingRecursionDepth binders on the stack. + [Fact] + public async Task ModelBindingSystem_ThrowsOn4Binders() + { + // Arrange + var expectedMessage = $"Model binding system exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingRecursionDepth)} (3). Reduce the " + + $"potential nesting of '{typeof(FourDeepModel)}'. For example, this type may have a property with a " + + $"model binder that always succeeds. See the " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingRecursionDepth)} documentation for more " + + $"information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(FourDeepModel), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + updateOptions: options => options.MaxModelBindingRecursionDepth = 3); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + + private class LoopyModel1 + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public LoopyModel2 Inner { get; set; } + } + + private class LoopyModel2 + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public LoopyModel3 Inner { get; set; } + } + + private class LoopyModel3 + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public LoopyModel1 Inner { get; set; } + } + + [Fact] + public async Task ModelBindingSystem_ThrowsOn33Binders_WithIndirectModelTypeLoop() + { + // Arrange + var expectedMessage = $"Model binding system exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingRecursionDepth)} (32). Reduce the " + + $"potential nesting of '{typeof(LoopyModel1)}'. For example, this type may have a property with a " + + $"model binder that always succeeds. See the " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingRecursionDepth)} documentation for more " + + $"information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(LoopyModel1), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + private static void SetJsonBodyContent(HttpRequest request, string content) { var stream = new MemoryStream(new UTF8Encoding(encoderShouldEmitUTF8Identifier: false).GetBytes(content)); diff --git a/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs index c19e7cfac5..e48a33586f 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs @@ -1128,6 +1128,40 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(0, modelState.ErrorCount); } + [Fact] + public async Task DictionaryModelBinder_ThrowsOn1025Items_AtTopLevel() + { + // Arrange + var expectedMessage = $"Collection bound to 'parameter' exceeded " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} (1024). This limit is a " + + $"safeguard against incorrect model binders and models. Address issues in " + + $"'{typeof(KeyValuePair)}'. For example, this type may have a " + + $"property with a model binder that always succeeds. See the " + + $"{nameof(MvcOptions)}.{nameof(MvcOptions.MaxModelBindingCollectionSize)} documentation for more " + + $"information."; + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + // CollectionModelBinder binds an empty collection when value providers are all empty. + request.QueryString = new QueryString("?a=b"); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => parameterBinder.BindModelAsync(parameter, testContext)); + Assert.Equal(expectedMessage, exception.Message); + } + private class ClosedGenericDictionary : Dictionary { } @@ -1138,7 +1172,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class ExplicitClosedGenericDictionary : IDictionary { - private IDictionary _data = new Dictionary(); + private readonly IDictionary _data = new Dictionary(); string IDictionary.this[string key] { @@ -1243,7 +1277,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class ExplicitDictionary : IDictionary { - private IDictionary _data = new Dictionary(); + private readonly IDictionary _data = new Dictionary(); TValue IDictionary.this[TKey key] { diff --git a/src/Mvc/test/Mvc.IntegrationTests/Models/SuccessfulModel.cs b/src/Mvc/test/Mvc.IntegrationTests/Models/SuccessfulModel.cs new file mode 100644 index 0000000000..5f3488d787 --- /dev/null +++ b/src/Mvc/test/Mvc.IntegrationTests/Models/SuccessfulModel.cs @@ -0,0 +1,13 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.AspNetCore.Mvc.IntegrationTests +{ + public class SuccessfulModel + { + [ModelBinder(typeof(SuccessfulModelBinder))] + public bool IsBound { get; set; } + + public string Name { get; set; } + } +} diff --git a/src/Mvc/test/Mvc.IntegrationTests/SuccessfulModelBinder.cs b/src/Mvc/test/Mvc.IntegrationTests/SuccessfulModelBinder.cs new file mode 100644 index 0000000000..b8bfff02e3 --- /dev/null +++ b/src/Mvc/test/Mvc.IntegrationTests/SuccessfulModelBinder.cs @@ -0,0 +1,22 @@ +// 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.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.ModelBinding; + +namespace Microsoft.AspNetCore.Mvc.IntegrationTests +{ + /// + /// An unconditionally-successful model binder. + /// + public class SuccessfulModelBinder : IModelBinder + { + public Task BindModelAsync(ModelBindingContext bindingContext) + { + var model = bindingContext.ModelType == typeof(bool) ? (object)true : null; + bindingContext.Result = ModelBindingResult.Success(model); + + return Task.CompletedTask; + } + } +} diff --git a/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs b/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs index 9a577877e8..8729db9063 100644 --- a/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs +++ b/src/Servers/Kestrel/Core/src/Properties/CoreStrings.Designer.cs @@ -2239,6 +2239,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core /// internal static string FormatServerShutdownDuringConnectionInitialization() => GetString("ServerShutdownDuringConnectionInitialization"); + /// /// Cannot call GetMemory() until response has started. Call HttpResponse.StartAsync() before calling GetMemory(). ///