Mvc: Exception thrown if no body is included when a multipart Content-Type is specified

[vaindil]

_(Sorry, not sure if this belongs in this repo or another one. I also opened aspnet/HttpAbstractions#747.)_

Typical behavior when a parameter is not included in a request is to make that parameter null. With the below code, however, I'm getting an exception if the request body is blank. The exception happens before the controller is called. If the body isn't blank but an invalid parameter is specified, the code performs as expected--the controller is reached but the relevant parameter is null.

IOException: Unexpected end of Stream, the content may have already been read by another component.

Function:

[HttpPost]
public async Task<IActionResult> Upload(IFormFile image)
{
    // body isn't necessary
}

Request body is empty, exception is thrown whether Content-Type is specified or not.

Accept: */*
Accept-Encoding: gzip, deflate
Authorization: Bearer <token here>
Cache-Control: no-cache
Connection: keep-alive
Content-Length: 0
Content-Type: multipart/form-data

Stack trace:

System.IO.IOException: Unexpected end of Stream, the content may have already been read by another component. 
   at Microsoft.AspNetCore.WebUtilities.MultipartReaderStream.<ReadAsync>d__36.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.WebUtilities.StreamHelperExtensions.<DrainAsync>d__3.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.WebUtilities.MultipartReader.<ReadNextSectionAsync>d__20.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Http.Features.FormFeature.<InnerReadFormAsync>d__18.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.ModelBinding.FormValueProviderFactory.<AddValueProviderAsync>d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.<CreateAsync>d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.DefaultControllerArgumentBinder.<BindArgumentsCoreAsync>d__6.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeNextResourceFilter>d__22.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Rethrow(ResourceExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeAsync>d__20.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.<Invoke>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware`1.<Invoke>d__18.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.<Invoke>d__7.MoveNext()

[vaindil]

Updated the title, discovered this has to do with Content-Type being multipart, not with the expected controller parameter. In this case a body is required per the spec.

The body must then contain one or more "body parts," each preceded by an encapsulation boundary, and the last one followed by a closing boundary.

Since the body format is invalid it wouldn't make sense for the request to reach the controller, but I think the framework should catch this and return a 400 instead of causing an exception and returning a 500.

[Eilon]

@Tratcher would it make sense for MVC's FormValueProviderFactory to have a new API on FormFeature (or related class) to have some kind of TryRead(...) so that it can detect an invalid multipart form (or other invalid request) and return a 400? MVC could certainly just try to catch exceptions and return a 400, but that is somewhat dubious.

[Tratcher]

That's pretty messy, we'd have to duplicate the entire parsing call stack with Try* methods, and you may still get IOExceptions from the underlying Stream. Exceptions are hard to avoid here and somebody has to catch them.

[Eilon]

Ah, that's unfortunate. We'll have to see if it's safe to catch exceptions here - that is, I want to make sure we don't inadvertently hide bugs that cause other "random" exceptions, and also that we don't inadvertently catch exceptions from user code.

@Tratcher any idea how safe it is to just try catch around the whole call? Does it throw only for "bad request" scenarios?

[rynowak]

Typically we'd want to do something make sure the parser is going to throw a format exception, and then only catch those.

[Eilon]

Right, the question is whether the parser can make a guarantee around that... not sure how "defensively" it's written.

[Tratcher]

It will definitely throw an IOException :-)
It also throws InvalidDataException and Cancellation exceptions

[rynowak]

Any idea what we could do to resolve this issue? We haven't had any other hits on this, and it's not clear what the action item from us would be here.

[Tratcher]

We've missed the chance to change the exception types for 2.0. You're left trying to catch what it currently throws and go from there. The most confusing is the reported IOException as it's ambiguous why the body ended unexpectedly. E.g. end of stream or client disconnect. You could try sending a 400 for both and just let the client disconnect scenario fail (silently).

[ignas-sakalauskas-cko]

Are there any plans to fix this issue?

[Eilon]

@ignas-sakalauskas-cko not at this time because we don't have a design for how to fix this.

[maximebousquet]

Hi, any update on this ?

[sepehr1014]

Still present on 2.1. Any updates on this issue?

[pranavkm]

cc @chrischu

Out of curiosity, how do you get in to this scenario? I tried this in a browser, and I get an empty multipart segment, but the boundaries are present which produces a non-zero content-length:

Content-Length: 188
Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryhUyOS5r5YPM6mvSa

[dougbu]

@pranavkm in the original description, it's the whole request body and not a specific part that's empty.

[Tratcher]

@dougbu understood. The question is what client generated such a request. It's questionable if it's even valid to send Content-Length: 0 and Content-Type: multipart/form-data together.

[chrischu]

@pranavkm The newest version of Postman does the request like this, I'm not sure about older versions, I just know that the old Chrome App version of it definitely doesn't exhibit that behavior (it sends a request just containing the form data boundary).

[Tratcher]

That seems more like a bug in Postman. It doesn't even give a 'boundary'.

[chrischu]

@Tratcher No, it does define a boundary (in the content-type header), but since there is no content there is also no need to put a boundary up, right?

[drauch]

@Tratcher :

That seems more like a bug in Postman. It doesn't even give a 'boundary'.

Stop it right there, that's a dangerous path : ) Regardless of the user input - the system should not run into a 5xx internal server error. A 500 triggers operations teams to investigate.
A malformed HTTP request should not crash a web server. Malformed input should never crash the target system in my opinion.

[Tratcher]

@chrischu do you have different headers than those shown in the original post?

@drauch Understood, the discussion of weather or not it's valid pertains to how we'd address it, not if we'd address it. If it really is valid to send a request like that we would handle it silently and you'd end up with an empty Form on the request. If it isn't valid then a 4xx response would be more appropriate and we'd additionally ask Postman to fix it.

And no a 5xx response does not qualify as a server crash, the server is still operating fine, only the request was rejected. 4xx just means we understood what was wrong with the request and decided it was worth the effort to give you a more specific response. These only get added for common errors where it's obvious that the client was at fault and how.

[chrischu]

@Tratcher Oh now I see where the confusion stems from (sorry about that!), I reported a bug here (https://github.com/aspnet/Mvc/issues/8414) and in the reported bug I witnessed headers that included the boundary but the problem still occurred.

We now built a workaround the problem by using a decorator for IControllerArgumentBinder that just sets all arguments to null when controllerContext.HttpContext.Request.ContentLength == 0. This works fine as we then get a BadRequest result that reports missing [Required] properties.

[drauch]

@chrischu : that workaround only works for 1.1.x, there is no IControllerArgumentBinder in 2.1.x.

[pranavkm]

The plan here is to return an empty form if the Content-Length is 0. This would only affect non-chunked requests. Chunked requests would be left as-is so would likely continue to fail in a similar way.

[pranavkm]

Fixed in https://github.com/aspnet/HttpAbstractions/commit/d0ddb068bea92f75fa0c783e97065ecbe3b2e04c