Aspnetcore: No handling for bad request when sent multipart form data request

[rosieks]

Describe the bug

If I send multipart/form-data request which has invalid headers then I receive 500 error. I would rather expect 400 as the request is invalid. In my case if content type doesn't have boundary or if there is no content disposition header

To Reproduce

Steps to reproduce the behavior:

1. Using this version of ASP.NET Core '2.2.0'
2. Run this code: https://gist.github.com/rosieks/c26c64ee1f0eb19efb07e5783909c20b
3. With these arguments: Just open '/api/values' in the browser
4. See error

Expected behavior

Right now I receive error 500, while I would expect that incorrect request will be handled by ASP.NET Core and returned as 400 - Bad Request.

Additional context

Add any other context about the problem here.
Include the output of dotnet --info

.NET Core SDK (reflecting any global.json):
Version: 2.2.203
Commit: e5bab63eca

Runtime Environment:
OS Name: Windows
OS Version: 10.0.16299
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\2.2.203\

Host (useful for support):
Version: 2.2.4
Commit: f95848e524

.NET Core SDKs installed:
1.1.0 [C:\Program Files\dotnet\sdk]
2.0.0 [C:\Program Files\dotnet\sdk]
2.0.2 [C:\Program Files\dotnet\sdk]
2.0.3 [C:\Program Files\dotnet\sdk]
2.1.2 [C:\Program Files\dotnet\sdk]
2.1.4 [C:\Program Files\dotnet\sdk]
2.1.100 [C:\Program Files\dotnet\sdk]
2.1.101 [C:\Program Files\dotnet\sdk]
2.1.103 [C:\Program Files\dotnet\sdk]
2.1.200 [C:\Program Files\dotnet\sdk]
2.1.201 [C:\Program Files\dotnet\sdk]
2.1.202 [C:\Program Files\dotnet\sdk]
2.1.300 [C:\Program Files\dotnet\sdk]
2.1.402 [C:\Program Files\dotnet\sdk]
2.1.403 [C:\Program Files\dotnet\sdk]
2.1.500 [C:\Program Files\dotnet\sdk]
2.1.503 [C:\Program Files\dotnet\sdk]
2.1.505 [C:\Program Files\dotnet\sdk]
2.1.602 [C:\Program Files\dotnet\sdk]
2.2.106 [C:\Program Files\dotnet\sdk]
2.2.203 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 1.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 1.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.0.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
https://aka.ms/dotnet-download

[mkArtakMSFT]

@pranavkm do we return 500 in 3.0 too?

[pranavkm]

Most likely if the FormReader throws as it did before.

[jaceks2106]

@pranavkm I investigated this issue deeper and actually it is thrown at FormFeature.GetBoundary().

[mkArtakMSFT]

@rosieks, would you be interested in sending us a PR for this? we'd happily consider it.

[rosieks]

Yes, I can send you a PR for that

[pranavkm]

I reopened @rosieks PR so that we can discuss this. We didn't do a very good job justifying why it wasn't correct.

1) In the PR that was sent, the FormValueProviderFactory adds a ModelState error when reading the form fails. This seems like a fairly low cost solution to fixing the unwanted 500s. However, having a value provider produce a ModelState error is very unusual and not something we've done previously. Value providers do not know what model is being bound, so the associated key would is always going to be incorrect. Associating it with the empty key doesn't seem like the right thing to do, but I could be mistaken.

2) Using ModelState to convey a malformed request payload is in line with what's done with Json and Xml formatters. One way to do this is have the FormFileValueProvider throw an exception that the built in model binders understand and use that to convey a malformed request. https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinder.cs#L98.

3) Yet another option might be to have a resource filter that reads the form content and decides to 400 the request if the form is malformed. IMO, this seems the most straightforward of the 3. However, it's different from how formatting works, as also figure out what customization and extensibility do we offer here.

@rynowak what do you think?

[rynowak]

I'd suggest that we throw a reasonable exception from the value provider - and handle it here:

• https://github.com/aspnet/AspNetCore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs#L58
• https://github.com/aspnet/AspNetCore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs#L62

This is similar to what we do formatters, as well as failed conneg. Right now value providers are an extensibility point that doesn't have a way to signal failure.

The other option is would be to add properties/stuff to ValueProviderFactoryContext so we could do this without an exception. This would require some new plubing to handle and would obsolete CompositeValueProvider.CreateAsync since we wouldn't call it anymore.

Yet another option might be to have a resource filter that reads the form content and decides to 400 the request if the form is malformed.

We don't need to this, we already read the form eagerly in MVC. This is the way we can guarantee that it's ready asynchronously.

Value providers do not know what model is being bound, so the associated key would is always going to be incorrect.

I think the problem with model state is unavoidable. ModelState having key => value where the key is always a property is a flaw because we have no way to indicate a global error.