Fix for #3743 - CancellationToken validation

This is a fix for the specific case reported in #3743 and also a few
related cases where the object being validated does not come from user
input.

Because the CancellationToken is bound using the 'empty prefix',
suppressing validation causes the validation system to suppress ALL keys
in the MSD. This will wipe out validation info for other, unrelated model
data. The same can occur for [FromServices], IFormCollection, IFormFile,
and HttpRequestMessage (anythere when MSD key == null and
SuppressValidation == true).

The change is to just ignore anything where the key is null and
SuppressValidation == true.

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs

@@ -65,10 +65,7 @@ private async Task<ModelBindingResult> BindModelCoreAsync(ModelBindingContext bi
                 throw new ArgumentNullException(nameof(bindingContext));
             }
 
-            // For compatibility with MVC 5.0 for top level object we want to consider an empty key instead of
-            // the parameter name/a custom name. In all other cases (like when binding body to a property) we
-            // consider the entire ModelName as a prefix.
-            var modelBindingKey = bindingContext.IsTopLevelObject ? string.Empty : bindingContext.ModelName;
+            var modelBindingKey = bindingContext.IsTopLevelObject ? bindingContext.FieldName : bindingContext.ModelName;
 
             var httpContext = bindingContext.OperationBindingContext.HttpContext;
 

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs

@@ -3,6 +3,7 @@
 
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding
 {
@@ -16,7 +17,9 @@ public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContex
         {
             if (bindingContext.ModelType == typeof(CancellationToken))
             {
-                var model = bindingContext.OperationBindingContext.HttpContext.RequestAborted;
+                // Force boxing so we can maintain a reference to this token in validation state.
+                var model = (object)bindingContext.OperationBindingContext.HttpContext.RequestAborted;
+                bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true });
                 return ModelBindingResult.SuccessAsync(bindingContext.ModelName, model);
             }
 

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs

@@ -76,12 +76,13 @@ private async Task<ModelBindingResult> RunModelBinders(ModelBindingContext bindi
                             ValidationStateEntry entry;
                             if (!bindingContext.ValidationState.TryGetValue(result.Model, out entry))
                             {
-                                entry = new ValidationStateEntry();
+                                entry = new ValidationStateEntry()
+                                {
+                                    Key = result.Key,
+                                    Metadata = bindingContext.ModelMetadata,
+                                };
                                 bindingContext.ValidationState.Add(result.Model, entry);
                             }
-
-                            entry.Key = entry.Key ?? result.Key;
-                            entry.Metadata = entry.Metadata ?? bindingContext.ModelMetadata;
                         }
 
                         return result;

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs

@@ -43,15 +43,22 @@ public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContex
 
         private async Task<ModelBindingResult> BindModelCoreAsync(ModelBindingContext bindingContext)
         {
+            // If we're at the top level, then use the FieldName (paramter or property name).
+            // This handles the fact that there will be nothing in the ValueProviders for this parameter
+            // and so we'll do the right thing even though we 'fell-back' to the empty prefix.
+            var modelName = bindingContext.IsTopLevelObject
+                ? bindingContext.FieldName
+                : bindingContext.ModelName;
+
             object value;
             if (bindingContext.ModelType == typeof(IFormFile))
             {
-                var postedFiles = await GetFormFilesAsync(bindingContext);
+                var postedFiles = await GetFormFilesAsync(modelName, bindingContext);
                 value = postedFiles.FirstOrDefault();
             }
             else if (typeof(IEnumerable<IFormFile>).IsAssignableFrom(bindingContext.ModelType))
             {
-                var postedFiles = await GetFormFilesAsync(bindingContext);
+                var postedFiles = await GetFormFilesAsync(modelName, bindingContext);
                 value = ModelBindingHelper.ConvertValuesToCollectionType(bindingContext.ModelType, postedFiles);
             }
             else
@@ -67,31 +74,29 @@ private async Task<ModelBindingResult> BindModelCoreAsync(ModelBindingContext bi
             }
             else
             {
-                bindingContext.ValidationState.Add(value, new ValidationStateEntry() { SuppressValidation = true });
+                bindingContext.ValidationState.Add(value, new ValidationStateEntry()
+                {
+                    Key = modelName,
+                    SuppressValidation = true
+                });
+
                 bindingContext.ModelState.SetModelValue(
-                    bindingContext.ModelName,
+                    modelName,
                     rawValue: null,
                     attemptedValue: null);
 
                 return ModelBindingResult.Success(bindingContext.ModelName, value);
             }
         }
 
-        private async Task<List<IFormFile>> GetFormFilesAsync(ModelBindingContext bindingContext)
+        private async Task<List<IFormFile>> GetFormFilesAsync(string modelName, ModelBindingContext bindingContext)
         {
             var request = bindingContext.OperationBindingContext.HttpContext.Request;
             var postedFiles = new List<IFormFile>();
             if (request.HasFormContentType)
             {
                 var form = await request.ReadFormAsync();
 
-                // If we're at the top level, then use the FieldName (paramter or property name).
-                // This handles the fact that there will be nothing in the ValueProviders for this parameter
-                // and so we'll do the right thing even though we 'fell-back' to the empty prefix.
-                var modelName = bindingContext.IsTopLevelObject
-                    ? bindingContext.FieldName
-                    : bindingContext.ModelName;
-
                 foreach (var file in form.Files)
                 {
                     ContentDispositionHeaderValue parsedContentDisposition;

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs

@@ -168,11 +168,12 @@ private bool Visit(ModelMetadata metadata, string key, object model)
             }
             else if ((entry != null && entry.SuppressValidation))
             {
-                SuppressValidation(key);
+                // Use the key on the entry, because we might not have entries in model state.
+                SuppressValidation(entry?.Key);
                 return true;
             }
 
-            using (StateManager.Recurse(this, key, metadata, model, strategy))
+            using (StateManager.Recurse(this, key ?? string.Empty, metadata, model, strategy))
             {
                 if (_metadata.IsEnumerableType)
                 {
@@ -271,6 +272,13 @@ private bool VisitChildren(IValidationStrategy strategy)
 
         private void SuppressValidation(string key)
         {
+            if (key == null)
+            {
+                // If the key is null, that means that we shouldn't expect any entries in ModelState for
+                // this value, so there's nothing to do.
+                return;
+            }
+
             var entries = _modelState.FindKeysWithPrefix(key);
             foreach (var entry in entries)
             {

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

@@ -10,7 +10,6 @@
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Internal;
 using Microsoft.AspNetCore.Mvc.Formatters;
-using Microsoft.AspNetCore.Routing;
 using Microsoft.Net.Http.Headers;
 using Moq;
 using Xunit;
@@ -73,9 +72,9 @@ public async Task BindModel_NoInputFormatterFound_SetsModelStateError()
             Assert.False(binderResult.IsModelSet);
             Assert.Null(binderResult.Model);
 
-            // Key is empty because this was a top-level binding.
+            // Key is the fieldName because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
-            Assert.Equal(string.Empty, entry.Key);
+            Assert.Equal("someName", entry.Key);
             Assert.Single(entry.Value.Errors);
         }
 
@@ -165,9 +164,9 @@ public async Task CustomFormatterDeserializationException_AddedToModelState()
             Assert.False(binderResult.IsModelSet);
             Assert.Null(binderResult.Model);
 
-            // Key is empty because this was a top-level binding.
+            // Key is the field name because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
-            Assert.Equal(string.Empty, entry.Key);
+            Assert.Equal("someName", entry.Key);
             var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message;
             Assert.Equal("Your input is bad!", errorMessage);
         }
@@ -200,9 +199,9 @@ public async Task NullFormatterError_AddedToModelState()
             Assert.False(binderResult.IsModelSet);
             Assert.Null(binderResult.Model);
 
-            // Key is empty because this was a top-level binding.
+            // Key is the field name because this was a top-level binding.
             var entry = Assert.Single(bindingContext.ModelState);
-            Assert.Equal(string.Empty, entry.Key);
+            Assert.Equal("someName", entry.Key);
             var errorMessage = Assert.Single(entry.Value.Errors).ErrorMessage;
             Assert.Equal("Unsupported content type 'text/xyz'.", errorMessage);
         }
@@ -267,6 +266,7 @@ private static ModelBindingContext GetBindingContext(
 
             var bindingContext = new ModelBindingContext
             {
+                FieldName = "someName",
                 IsTopLevelObject = true,
                 ModelMetadata = metadataProvider.GetMetadataForType(modelType),
                 ModelName = "someName",

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs

@@ -5,6 +5,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http.Internal;
+using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test
@@ -60,7 +61,8 @@ private static ModelBindingContext GetBindingContext(Type modelType)
                     },
                     ModelBinder = new CancellationTokenModelBinder(),
                     MetadataProvider = metadataProvider,
-                }
+                },
+                ValidationState = new ValidationStateDictionary(),
             };
 
             return bindingContext;

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs

@@ -35,7 +35,7 @@ public async Task FormFileModelBinder_SuppressesValidation()
 
             var entry = bindingContext.ValidationState[result.Model];
             Assert.True(entry.SuppressValidation);
-            Assert.Null(entry.Key);
+            Assert.Equal("file", entry.Key);
             Assert.Null(entry.Metadata);
         }
 
@@ -59,7 +59,7 @@ public async Task FormFileModelBinder_ExpectMultipleFiles_BindSuccessful()
 
             var entry = bindingContext.ValidationState[result.Model];
             Assert.True(entry.SuppressValidation);
-            Assert.Null(entry.Key);
+            Assert.Equal("file", entry.Key);
             Assert.Null(entry.Metadata);
 
             var files = Assert.IsAssignableFrom<IList<IFormFile>>(result.Model);

test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs

@@ -97,17 +97,21 @@ public async Task ModelMetaDataTypeAttribute_InvalidPropertiesAndSubPropertiesOn
             Assert.NotNull(boundPerson);
             Assert.False(modelState.IsValid);
             var modelStateErrors = CreateValidationDictionary(modelState);
-            Assert.Equal("CompanyName cannot be null or empty.", modelStateErrors["CompanyName"]);
-            Assert.Equal("The field Price must be between 20 and 100.", modelStateErrors["Price"]);
+            Assert.Equal("CompanyName cannot be null or empty.", modelStateErrors["Parameter1.CompanyName"]);
+            Assert.Equal("The field Price must be between 20 and 100.", modelStateErrors["Parameter1.Price"]);
             // Mono issue - https://github.com/aspnet/External/issues/19
-            Assert.Equal(PlatformNormalizer.NormalizeContent("The Category field is required."), modelStateErrors["Category"]);
-            Assert.Equal(PlatformNormalizer.NormalizeContent("The Contact Us field is required."), modelStateErrors["Contact"]);
+            Assert.Equal(
+                PlatformNormalizer.NormalizeContent("The Category field is required."),
+                modelStateErrors["Parameter1.Category"]);
+            Assert.Equal(
+                PlatformNormalizer.NormalizeContent("The Contact Us field is required."),
+                modelStateErrors["Parameter1.Contact"]);
             Assert.Equal(
                 PlatformNormalizer.NormalizeContent("The Detail2 field is required."),
-                modelStateErrors["ProductDetails.Detail2"]);
+                modelStateErrors["Parameter1.ProductDetails.Detail2"]);
             Assert.Equal(
                 PlatformNormalizer.NormalizeContent("The Detail3 field is required."),
-                modelStateErrors["ProductDetails.Detail3"]);
+                modelStateErrors["Parameter1.ProductDetails.Detail3"]);
         }
 
         [Fact]
@@ -147,7 +151,7 @@ public async Task ModelMetaDataTypeAttribute_InvalidComplexTypePropertyOnBaseCla
             var modelStateErrors = CreateValidationDictionary(modelState);
             Assert.Equal(
                 PlatformNormalizer.NormalizeContent("The ProductDetails field is required."),
-                modelStateErrors["ProductDetails"]);
+                modelStateErrors["Parameter1.ProductDetails"]);
         }
 
         [Fact]
@@ -187,7 +191,7 @@ public async Task ModelMetaDataTypeAttribute_InvalidClassAttributeOnBaseClass_Ha
             Assert.False(modelState.IsValid);
             var modelStateErrors = CreateValidationDictionary(modelState);
             Assert.Single(modelStateErrors);
-            Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors[""]);
+            Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors["Parameter1"]);
         }
 
         [Fact]
@@ -263,8 +267,10 @@ public async Task ModelMetaDataTypeAttribute_InvalidPropertiesOnDerivedClass_Has
             Assert.False(modelState.IsValid);
             var modelStateErrors = CreateValidationDictionary(modelState);
             Assert.Equal(2, modelStateErrors.Count);
-            Assert.Equal("The field Price must be between 100 and 200.", modelStateErrors["Price"]);
-            Assert.Equal("The field Contact must be a string with a maximum length of 10.", modelStateErrors["Contact"]);
+            Assert.Equal("The field Price must be between 100 and 200.", modelStateErrors["Parameter1.Price"]);
+            Assert.Equal(
+                "The field Contact must be a string with a maximum length of 10.",
+                modelStateErrors["Parameter1.Contact"]);
         }
 
         [Fact]
@@ -304,7 +310,7 @@ public async Task ModelMetaDataTypeAttribute_InvalidClassAttributeOnBaseClassPro
             Assert.False(modelState.IsValid);
             var modelStateErrors = CreateValidationDictionary(modelState);
             Assert.Single(modelStateErrors);
-            Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors[""]);
+            Assert.Equal("Product must be made in the USA if it is not named.", modelStateErrors["Parameter1"]);
         }
 
         [Fact]
@@ -381,7 +387,7 @@ public async Task FromBodyOnActionParameter_EmptyBody_BindsToNullValue()
 
             Assert.True(modelState.IsValid);
             var entry = Assert.Single(modelState);
-            Assert.Empty(entry.Key);
+            Assert.Equal("CustomParameter", entry.Key);
             Assert.Null(entry.Value.RawValue);
         }
 

test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs

@@ -5,6 +5,7 @@
 using System.ComponentModel.DataAnnotations;
 using System.IO;
 using System.Text;
+using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Abstractions;
@@ -1197,7 +1198,7 @@ public async Task FromBody_JToken_ExcludedFromValidation()
 
             // We need to add another model state entry which should get marked as skipped so 
             // we can prove that the JObject was skipped.
-            modelState.SetModelValue("message", "Hello", "Hello");
+            modelState.SetModelValue("CustomParameter.message", "Hello", "Hello");
 
             // Act
             var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext);
@@ -1211,13 +1212,55 @@ public async Task FromBody_JToken_ExcludedFromValidation()
             Assert.True(modelState.IsValid);
             Assert.Equal(2, modelState.Count);
 
-            var entry = Assert.Single(modelState, kvp => kvp.Key == string.Empty);
+            var entry = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter");
             Assert.Equal(ModelValidationState.Valid, entry.Value.ValidationState);
 
-            entry = Assert.Single(modelState, kvp => kvp.Key == "message");
+            entry = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.message");
             Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState);
         }
 
+        // Regression test for https://github.com/aspnet/Mvc/issues/3743
+        //
+        // A cancellation token that's bound with the empty prefix will end up suppressing
+        // the empty prefix. Since the empty prefix is a prefix of everything, this will 
+        // basically result in clearing out all model errors, which is BAD.
+        //
+        // The fix is to treat non-user-input as have a key of null, which means that the MSD
+        // isn't even examined when it comes to supressing validation.
+        [Fact]
+        public async Task CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors()
+        {
+            // Arrange
+            var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value);
+            var parameter = new ParameterDescriptor
+            {
+                Name = "cancellationToken",
+                ParameterType = typeof(CancellationToken)
+            };
+
+            var operationContext = ModelBindingTestHelper.GetOperationBindingContext();
+
+            var httpContext = operationContext.HttpContext;
+            var modelState = operationContext.ActionContext.ModelState;
+
+            // We need to add another model state entry - we want this to be ignored.
+            modelState.SetModelValue("message", "Hello", "Hello");
+
+            // Act
+            var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext);
+
+            // Assert
+            Assert.True(modelBindingResult.IsModelSet);
+            Assert.NotNull(modelBindingResult.Model);
+            Assert.IsType<CancellationToken>(modelBindingResult.Model);
+
+            Assert.False(modelState.IsValid);
+            Assert.Equal(1, modelState.Count);
+
+            var entry = Assert.Single(modelState, kvp => kvp.Key == "message");
+            Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState);
+        }
+
         private static void AssertRequiredError(string key, ModelError error)
         {
             // Mono issue - https://github.com/aspnet/External/issues/19