diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormValueProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormValueProvider.cs index 6a07ba4f67..278407db2d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormValueProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormValueProvider.cs @@ -13,7 +13,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// public class FormValueProvider : BindingSourceValueProvider, IEnumerableValueProvider { - private readonly CultureInfo _culture; private readonly IFormCollection _values; private PrefixContainer _prefixContainer; @@ -40,10 +39,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } _values = values; - _culture = culture; + Culture = culture; } - public CultureInfo Culture => _culture; + public CultureInfo Culture { get; } protected PrefixContainer PrefixContainer { @@ -83,6 +82,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(key)); } + if (key.Length == 0) + { + // Top level parameters will fall back to an empty prefix when the parameter name does not + // appear in any value provider. This would result in the parameter binding to a form parameter + // with a empty key (e.g. Request body looks like "=test") which isn't a scenario we want to support. + // Return a "None" result in this event. + return ValueProviderResult.None; + } + var values = _values[key]; if (values.Count == 0) { @@ -90,7 +98,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } else { - return new ValueProviderResult(values, _culture); + return new ValueProviderResult(values, Culture); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/JQueryValueProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/JQueryValueProvider.cs index 0789e6b50b..581b18112c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/JQueryValueProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/JQueryValueProvider.cs @@ -79,6 +79,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// public override ValueProviderResult GetValue(string key) { + if (key == null) + { + throw new ArgumentNullException(nameof(key)); + } + if (_values.TryGetValue(key, out var values) && values.Count > 0) { return new ValueProviderResult(values, Culture); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/QueryStringValueProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/QueryStringValueProvider.cs index e59048c542..9094a62dda 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/QueryStringValueProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/QueryStringValueProvider.cs @@ -13,7 +13,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// public class QueryStringValueProvider : BindingSourceValueProvider, IEnumerableValueProvider { - private readonly CultureInfo _culture; private readonly IQueryCollection _values; private PrefixContainer _prefixContainer; @@ -40,10 +39,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } _values = values; - _culture = culture; + Culture = culture; } - public CultureInfo Culture => _culture; + public CultureInfo Culture { get; } protected PrefixContainer PrefixContainer { @@ -83,6 +82,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(key)); } + if (key.Length == 0) + { + // Top level parameters will fall back to an empty prefix when the parameter name does not + // appear in any value provider. This would result in the parameter binding to a query string + // parameter with a empty key (e.g. /User?=test) which isn't a scenario we want to support. + // Return a "None" result in this event. + return ValueProviderResult.None; + } + var values = _values[key]; if (values.Count == 0) { @@ -90,7 +98,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } else { - return new ValueProviderResult(values, _culture); + return new ValueProviderResult(values, Culture); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/RouteValueProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/RouteValueProvider.cs index ee1335711b..66e1ea1c2e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/RouteValueProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/RouteValueProvider.cs @@ -85,6 +85,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(key)); } + if (key.Length == 0) + { + // Top level parameters will fall back to an empty prefix when the parameter name does not + // appear in any value provider. This would result in the parameter binding to a route value + // an empty key which isn't a scenario we want to support. + // Return a "None" result in this event. + return ValueProviderResult.None; + } + if (_values.TryGetValue(key, out var value)) { var stringValue = value as string ?? Convert.ToString(value, Culture) ?? string.Empty; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs index 611acad7f1..66d87b5f06 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CompositeValueProviderTest.cs @@ -5,6 +5,8 @@ using System; using System.Collections.Generic; using System.Globalization; using System.Linq; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Internal; using Microsoft.Extensions.Primitives; using Moq; using Xunit; @@ -38,9 +40,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Dictionary values, CultureInfo culture) { - var emptyValueProvider = - new JQueryFormValueProvider(bindingSource, new Dictionary(), culture); - var valueProvider = new JQueryFormValueProvider(bindingSource, values, culture); + var emptyValueProvider = new QueryStringValueProvider(bindingSource, new QueryCollection(), culture); + var valueProvider = new FormValueProvider(bindingSource, new FormCollection(values), culture); return new CompositeValueProvider() { emptyValueProvider, valueProvider }; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/EnumerableValueProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/EnumerableValueProviderTest.cs index 19c3d37407..6d586a80d9 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/EnumerableValueProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/EnumerableValueProviderTest.cs @@ -235,6 +235,23 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal(ValueProviderResult.None, result); } + [Fact] + public virtual void GetValue_EmptyKey() + { + // Arrange + var store = new Dictionary(BackingStore) + { + { string.Empty, "some-value" }, + }; + var valueProvider = GetEnumerableValueProvider(BindingSource.Query, store, culture: null); + + // Act + var result = valueProvider.GetValue(string.Empty); + + // Assert + Assert.Equal(ValueProviderResult.None, result); + } + [Fact] public virtual void FilterInclude() { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryFormValueProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryFormValueProviderTest.cs index 608180ac3b..4676530e5f 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryFormValueProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryFormValueProviderTest.cs @@ -31,5 +31,22 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding // Assert Assert.Null(result); } + + [Fact] + public override void GetValue_EmptyKey() + { + // Arrange + var store = new Dictionary(BackingStore) + { + { string.Empty, "some-value" }, + }; + var valueProvider = GetEnumerableValueProvider(BindingSource.Query, store, culture: null); + + // Act + var result = valueProvider.GetValue(string.Empty); + + // Assert + Assert.Equal("some-value", (string)result); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryQueryStringValueProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryQueryStringValueProviderTest.cs index 830ceac1ab..8d9b795769 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryQueryStringValueProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/JQueryQueryStringValueProviderTest.cs @@ -34,5 +34,22 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding // Assert Assert.Null(result); } + + [Fact] + public override void GetValue_EmptyKey() + { + // Arrange + var store = new Dictionary(BackingStore) + { + { string.Empty, "some-value" }, + }; + var valueProvider = GetEnumerableValueProvider(BindingSource.Query, store, culture: null); + + // Act + var result = valueProvider.GetValue(string.Empty); + + // Assert + Assert.Equal("some-value", (string)result); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs index 0999087bec..62f3c3eb9f 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs @@ -152,6 +152,39 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } + [Fact] + public async Task BindParameter_WithEmptyQueryStringKey_DoesNotGetBound() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor + { + Name = "Parameter1", + BindingInfo = new BindingInfo(), + + ParameterType = typeof(string) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?=someValue"); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + + // ModelBindingResult + Assert.False(modelBindingResult.IsModelSet); + + // ModelState + Assert.True(modelState.IsValid); + Assert.Empty(modelState.Keys); + } + [Fact] [ReplaceCulture("en-GB", "en-GB")] public async Task BindDecimalParameter_WithData_GetsBound()