Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException

[jcracknell]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

Upgrading from 3.1 to 6.0, and a multipart body exceeding the configured MaxHttpRequestBodySize no longer produces an error response (or a response at all).

Under 3.1 attempting this would produce the trace:

   at Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException.Throw(RequestRejectionReason reason)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.OnReadStarting()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.TryStart()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.ReadAsyncInternal(CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.ReadAsyncInternal(Memory`1 buffer, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.WebUtilities.BufferedReadStream.EnsureBufferedAsync(Int32 minCount, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.WebUtilities.MultipartReaderStream.ReadAsync(Byte[] buffer, Int32 offset, Int32 count, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.WebUtilities.StreamHelperExtensions.DrainAsync(Stream stream, ArrayPool`1 bytePool, Nullable`1 limit, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.WebUtilities.MultipartReader.ReadNextSectionAsync(CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Http.Features.FormFeature.InnerReadFormAsync(CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Mvc.ModelBinding.FormValueProviderFactory.AddValueProviderAsync(ValueProviderFactoryContext context)
   at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.CreateAsync(ActionContext actionContext, IList`1 factories)
   at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.TryCreateAsync(ActionContext actionContext, IList`1 factories)
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext()

Interestingly the log reports:

[2022-03-21 17:26:39.0396][INFO][Microsoft.AspNetCore.Hosting.Diagnostics] Request finished HTTP/1.1 POST http://localhost:57513/api/modules/irams/attachments multipart/form-data;+boundary=---------------------------344584781640975190543136916438 34166185 - 500 - text/plain;+charset=utf-8 90.6786ms
[2022-03-21 17:26:39.0396][DEBUG][Microsoft.AspNetCore.Server.Kestrel.BadRequests] Connection id "0HMGBG5LMMVRR" bad request data: "Request body too large. The max request body size is 33554432 bytes."Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException: Request body too large. The max request body size is 33554432 bytes.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.OnReadStarting()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.TryStartAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.ConsumeAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequestsAsync[TContext](IHttpApplication`1 application)
[2022-03-21 17:26:39.0396][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Connections] Connection id "0HMGBG5LMMVRR" disconnecting.
[2022-03-21 17:26:39.0396][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Connections] Connection id "0HMGBG5LMMVRR" stopped.
[2022-03-21 17:26:39.0396][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets] Connection id "0HMGBG5LMMVRR" resumed.
[2022-03-21 17:26:39.0396][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets] Connection id "0HMGBG5LMMVRR" sending FIN because: "The Socket transport's send loop completed gracefully."

But what I actually get is a closed connection (no response), and a ModelBinding error with a null IFormFile on the server.

This appears to be caught by FormValueProviderFactory per 78a587b02ebce99493a8e9b8c417ecbbbee457ce:

https://github.com/dotnet/aspnetcore/blob/57c3104f0d19bb9e25b8ba75b422e2d6072e1dd5/src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs#L50-L55

Expected Behavior

Any request exceeding MaxRequestBodySize should produce an HTTP 413: Payload too large.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

6.0.102

Anything else?

No response

[jcracknell]

To be clear, the exception handling should be removed as you cannot proceed with modelbinding (or antiforgery validation for that matter) if an IOException has occurred.

The fix should also be backported to 5.x.

[adityamandaleeka]

Triage: the easiest/safest thing to do is probably add another catch in the highlighted code for BadHttpRequestException so as to preserve the original error.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 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.

[halter73]

@jcracknell Is this not producing an empty 500 response? The correct behavior might just be to rethrow the BadHttpRequestException so you get a 413 instead of a 500 response. Is this what you want?

[jcracknell]

@halter73 Ok so there is a response (and a Firefox fetch API bug), but the issue remains that IOExceptions are being suppressed and controller action invocation proceeds even when the request was not fully received.

I'm still very skeptical that you can proceed in the event of an IOException, except in the case where the request was actually fully received and the client hung up.

3.1 + custom BadHttpRequestExceptionMiddleware (3.1 did not automatically set response codes for BadHttpRequestException):

curl -v -X POST http://localhost:57513/api/modules/irams/attachments \
  --cookie ELP.Session=PFBO3TW2XPZUKFVD2DBULST7DYFVTDVIFZ5BNIY \
  --form file=@/home/jcracknell/Downloads/Beyond-All-Reason-1.1392.0.AppImage
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying 127.0.0.1:57513...
* Connected to localhost (127.0.0.1) port 57513 (#0)
> POST /api/modules/irams/attachments HTTP/1.1
> Host: localhost:57513
> User-Agent: curl/7.82.0
> Accept: */*
> Cookie: ELP.Session=PFBO3TW2XPZUKFVD2DBULST7DYFVTDVIFZ5BNIY
> Content-Length: 246963179
> Content-Type: multipart/form-data; boundary=------------------------6ca743bc167bbfd5
> Expect: 100-continue
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 413 Payload Too Large
< Date: Fri, 25 Mar 2022 13:09:30 GMT
< Server: Kestrel
< Content-Length: 0
* HTTP error before end of send, stop sending
<
* Closing connection 0

6.0 without custom BadHttpRequestException middleware (not checking the validation state is my problem):

curl -v -X POST http://localhost:57513/api/modules/irams/attachments \
  --cookie ELP.Session=PFBO3TW2XPZUKFVD2DBULST7DYFVTDVIFZ5BNIY \
  --form file=@/home/jcracknell/Downloads/Beyond-All-Reason-1.1392.0.AppImage
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying 127.0.0.1:57513...
* Connected to localhost (127.0.0.1) port 57513 (#0)
> POST /api/modules/irams/attachments HTTP/1.1
> Host: localhost:57513
> User-Agent: curl/7.82.0
> Accept: */*
> Cookie: ELP.Session=PFBO3TW2XPZUKFVD2DBULST7DYFVTDVIFZ5BNIY
> Content-Length: 246963179
> Content-Type: multipart/form-data; boundary=------------------------43628c3758620eda
> Expect: 100-continue
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 500 Internal Server Error
< Content-Type: text/plain; charset=utf-8
< Date: Fri, 25 Mar 2022 13:24:35 GMT
< Server: Kestrel
< Transfer-Encoding: chunked
* HTTP error before end of send, stop sending
< 
System.NullReferenceException: Object reference not set to an instance of an object.
   at ELP.Modules.EIA.IRAMS.Web.Api.Controllers.AttachmentController.PostAttachment(IFormFile file) in /home/jcracknell/git/elp/src/Modules/EIA/IRAMS/src/Web/Api/Controllers/AttachmentController.cs:line 32
   at lambda_method176(Closure , Object )
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Logged|12_1(ControllerActionInvoker invoker)
   <snip>

HEADERS
=======
Accept: */*
Host: localhost:57513
User-Agent: curl/7.82.0
Content-Type: multipart/form-data; boundary=------------------------43628c3758620eda
Cookie: ELP.Session=PFBO3TW2XPZUKFVD2DBULST7DYFVTDVIFZ5BNIY
Expect: 100-continue
Content-Length: 246963179
* Closing connection 0

Kestrel messages for the same request:

[2022-03-25 09:24:36.8951][INFO][Microsoft.AspNetCore.Hosting.Diagnostics] Request finished HTTP/1.1 POST http://localhost:57513/api/modules/irams/attachments multipart/form-data;+boundary=------------------------43628c3758620eda 246963179 - 500 - text/plain;+charset=utf-8 92.3517ms
[2022-03-25 09:24:36.8951][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets] Connection id "0HMGEC4ESMJTV" received FIN.
[2022-03-25 09:24:36.8951][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets] Connection id "0HMGEC4ESMJTV" sending FIN because: "The client closed the connection."
[2022-03-25 09:24:36.8951][DEBUG][Microsoft.AspNetCore.Server.Kestrel.BadRequests] Connection id "0HMGEC4ESMJTV" bad request data: "Request body too large. The max request body size is 33554432 bytes."Microsoft.AspNetCore.Server.Kestrel.Core.BadHttpRequestException: Request body too large. The max request body size is 33554432 bytes.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.Http1ContentLengthMessageBody.OnReadStarting()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.TryStartAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.MessageBody.ConsumeAsync()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequestsAsync[TContext](IHttpApplication`1 application)
[2022-03-25 09:24:36.8951][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Connections] Connection id "0HMGEC4ESMJTV" disconnecting.
[2022-03-25 09:24:36.8951][DEBUG][Microsoft.AspNetCore.Server.Kestrel.Connections] Connection id "0HMGEC4ESMJTV" stopped.

[jcracknell]

In case anyone is looking for a workaround it is reasonably straightforward to replace all of the form-related IValueProviderFactory implementations with patched versions via MvcOptions.ValueProviderFactories (as the factories are relatively trivial), however the JQueryFormValueProviderFactory depends on internal static helpers.

Alternatively you could wrap the existing factories to unwrap and rethrow ValueProviderExceptions over inner IOExceptions, but doing so alters the stack trace of the IOException.

[ghost]

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.

[chrischu]

We would like to note that one other place where an IOException is caught where it probably shouldn't be is https://github.com/dotnet/aspnetcore/blob/57c3104f0d19bb9e25b8ba75b422e2d6072e1dd5/src/Antiforgery/src/Internal/DefaultAntiforgeryTokenStore.cs#L69-L74

In our case this behavior lead to HTTP 400 (instead of 500) when e.g. the disk is full (IOException "There is not enough space on the disk") when browsing to routes protected by an AntiforgeryToken. Other routes would just skip model binding leading to the model being null.

In either case we think that the correct response would be a 500 error to show that something went wrong on the server.

[LilahTovMoon]

This is still an issue. If I send a request that's too large, I get a screen telling me, "A valid antiforgery token was not provided with the request. Add an antiforgery token, or disable antiforgery validation for this endpoint." This leads a developer to try and debug why the anti forgery token isn't working rather than telling them the issue is that the request is too large.

It'd be a lot better if the developer saw this, giving them the "BadHttpRequestException: Request body too large. The max request body size is 30000000 bytes.":