dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.06k stars 9.9k forks source link

Model binding fails with FormatException if dictionary key is numeric type and value is missing from POST #35594

Open karlra opened 3 years ago

karlra commented 3 years ago

Describe the bug

In ASP.net, it is possible to POST a dictionary in a form submission in the following format:

<input type="text" name="sortOrder[319]" value="0" />
<input type="text"  name="sortOrder[320]" value="10" />

This will be bound correctly to a controller action like this:

public async Task<IActionResult> TagDetails(Dictionary<int, int> sortOrder)

However, if the sortOrder field is completely missing from the POST, such as is very likely to happen if this data comes from a database and the list to sort is empty, the controller action crashes with a validation error IF the key in the dictionary is a numeric type like int/long/float/decimal OR a field that requires parsing, like DateTime.

If we attempt to map it to a Dictionary<DateTime, string> we see the error as "FormatException: The string 'submit' was not recognized as a valid DateTime. There is an unknown word starting at index '0'.". For numbers, the error is a similar parsing error. Depending on what the form looks like the name of the field it attempts to parse ("submit") in the example above is different, so it happens regardless of whether this list is the only thing on the form or not.

It seems that ASP.net is attempting to over-parse this, because surely it should be attempting to parse only named parameters if it's a form being posted? If the type is int[] or List or similar it works fine (the parameter becomes null, as expected).

To Reproduce

Repro attached for ASP.net Core 5.

dictionary_parse_bug.zip

karlra commented 3 years ago

In order to work around this, I attempted to change the type to Dictionary<string, int> which doesn't crash, but instead it binds the entire form to the Dictionary?

image

I am using the example from https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0 (scroll down to the Dictionaries section), and it seems like that example also crashes if selectedCourses is missing from the POST.

pranavkm commented 2 years ago

Model binding will fallback to an empty prefix if it does not find any values that matches the parameter's prefix - https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0#prefix--parameter-name. In your case, since no keys with the prefix sortOrder were present, model binding falls back to the empty prefix at which point every field is a candidate to be bound.

You can limit what prefixes are allowed to be bound by specifying it on the parameter - https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding?view=aspnetcore-5.0#custom-prefix.

Also just to be certain, I would expect binding to add the format exception as a a validation error to the ModelStateDictionary in and not throw an exception in this case. Is that not the case?

karlra commented 2 years ago

It doesn't even enter the controller action but crashes before.

pranavkm commented 2 years ago

Ooh that's a good find. It's weird that format exceptions from dictionaries are rethrown:

An unhandled exception has occurred while executing the request.
      System.FormatException: Input string was not in a correct format.
         at System.Number.ThrowOverflowOrFormatException(ParsingStatus status, TypeCode type)
         at System.Number.ParseInt32(ReadOnlySpan`1 value, NumberStyles styles, NumberFormatInfo info)
         at System.Int32.Parse(String s, NumberStyles style, IFormatProvider provider)
         at System.ComponentModel.Int32Converter.FromString(String value, NumberFormatInfo formatInfo)
         at System.ComponentModel.BaseNumberConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value)
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingHelper.ConvertSimpleType(Object value, Type destinationType, CultureInfo culture)
         at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingHelper.UnwrapPossibleArrayType(Object value, Type destinationType, CultureInfo culture)
         at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingHelper.ConvertTo(Object value, Type type, CultureInfo culture)
         at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DictionaryModelBinder`2.BindModelAsync(ModelBindingContext bindingContext)
         at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value, Object container)
         at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext()
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
         at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)

FYI @dougbu in case this stacktrace rings a bell.

dougbu commented 2 years ago

Doesn't ring a bell but we have lots of try / catch blocks in other model binders around xyz.ConvertTo(...) calls. We probably aren't testing Dictionary<{numeric type}> enough.

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

anibal-acosta commented 2 years ago

Same problem here (.net core 6.0 razor page), I have this dictionary [BindProperty] public Dictionary<Int32, String> Relations { get; set; }

When user POST and there are no "Relations" sent the page throw exception Invalid Input format string.

So, I decide to add a hidden field and works <input type="hidden" name="Relations[0]" value="0" />

and in the Post method I check that dictionary key is not 0 in the iteration.

but definitely is a bug, dictionary should be null when no data with that name is present in the POST

GonziHere commented 2 years ago

I have the same issue. I've tried [CanBeNull] and it didn't help. Also, it took significant time to find the issue, since error is somewhat cryptic.

My workaround is this:

@if (Model.MyArray.Count == 0)
{
    <input type="hidden" asp-for="@Model.MyArray" value="null" />
}

With combination of [CanBeNull] (maybe not necessary).

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.