New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception thrown if no body is included when a multipart Content-Type is specified #5631

Closed
vaindil opened this Issue Dec 21, 2016 · 25 comments

Comments

Projects
None yet
@vaindil

vaindil commented Dec 21, 2016

(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 vaindil changed the title from Exception thrown if no body is included when an IFormFile is expected to Exception thrown if no body is included when a multipart/form-data Content-Type is specified Dec 22, 2016

@vaindil

This comment has been minimized.

vaindil commented Dec 22, 2016

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.

@vaindil vaindil changed the title from Exception thrown if no body is included when a multipart/form-data Content-Type is specified to Exception thrown if no body is included when a multipart Content-Type is specified Dec 22, 2016

@Eilon

This comment has been minimized.

Member

Eilon commented Jan 11, 2017

@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

This comment has been minimized.

Member

Tratcher commented Jan 11, 2017

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

This comment has been minimized.

Member

Eilon commented Jan 11, 2017

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

This comment has been minimized.

Member

rynowak commented Jan 11, 2017

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

@Eilon

This comment has been minimized.

Member

Eilon commented Jan 12, 2017

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

@Tratcher

This comment has been minimized.

Member

Tratcher commented Jan 12, 2017

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

@Eilon Eilon added this to the Backlog milestone Jan 30, 2017

@rynowak

This comment has been minimized.

Member

rynowak commented Jul 18, 2017

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

This comment has been minimized.

Member

Tratcher commented Jul 18, 2017

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

This comment has been minimized.

ignas-sakalauskas-cko commented Dec 6, 2017

Are there any plans to fix this issue?

@Eilon

This comment has been minimized.

Member

Eilon commented Dec 7, 2017

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

@maximebousquet

This comment has been minimized.

maximebousquet commented May 29, 2018

Hi, any update on this ?

@sepehr1014

This comment has been minimized.

sepehr1014 commented Jul 27, 2018

Still present on 2.1. Any updates on this issue?

@pranavkm

This comment has been minimized.

Member

pranavkm commented Sep 7, 2018

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

This comment has been minimized.

Member

dougbu commented Sep 7, 2018

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

@Tratcher

This comment has been minimized.

Member

Tratcher commented Sep 7, 2018

@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

This comment has been minimized.

chrischu commented Sep 7, 2018

@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

This comment has been minimized.

Member

Tratcher commented Sep 8, 2018

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

@chrischu

This comment has been minimized.

chrischu commented Sep 8, 2018

@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

This comment has been minimized.

drauch commented Sep 8, 2018

@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

This comment has been minimized.

Member

Tratcher commented Sep 10, 2018

@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

This comment has been minimized.

chrischu commented Sep 10, 2018

@Tratcher Oh now I see where the confusion stems from (sorry about that!), I reported a bug here (#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

This comment has been minimized.

drauch commented Sep 10, 2018

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

@pranavkm

This comment has been minimized.

Member

pranavkm commented Sep 11, 2018

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 added a commit to aspnet/HttpAbstractions that referenced this issue Sep 12, 2018

pranavkm added a commit to aspnet/HttpAbstractions that referenced this issue Sep 12, 2018

pranavkm added a commit to aspnet/HttpAbstractions that referenced this issue Sep 12, 2018

pranavkm added a commit to aspnet/HttpAbstractions that referenced this issue Sep 12, 2018

@pranavkm pranavkm self-assigned this Sep 12, 2018

@pranavkm pranavkm modified the milestones: Backlog, 2.2.0-preview3 Sep 12, 2018

pranavkm added a commit to aspnet/HttpAbstractions that referenced this issue Sep 13, 2018

Return FormCollection.Empty when Content-Length is 0 (#1038)
* Return FormCollection.Empty when Content-Length is 0

Fixes aspnet/Mvc#5631
@pranavkm

This comment has been minimized.

Member

pranavkm commented Sep 13, 2018

Fixed in aspnet/HttpAbstractions@d0ddb06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment