Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException #40810

jcracknell · 2022-03-21T22:01:13Z

Is there an existing issue for this?

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 78a587b:

aspnetcore/src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs

Lines 50 to 55 in 57c3104

    
           catch (IOException ex) 
        
           { 
        
               // ReadFormAsync can throw IOException if the client disconnects. 
        
               // Wrap it in a ValueProviderException that the CompositeValueProvider special cases. 
        
               throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex); 
        
           }

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

The text was updated successfully, but these errors were encountered:

jcracknell · 2022-03-23T13:47:33Z

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 · 2022-03-23T20:10:49Z

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 · 2022-03-24T18:37:38Z

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 · 2022-03-24T18:39:16Z

@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 · 2022-03-25T13:31:30Z

@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 · 2022-04-20T17:12:00Z

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 · 2022-10-11T19:16:49Z

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 · 2023-01-04T13:51:58Z

We would like to note that one other place where an IOException is caught where it probably shouldn't be is

aspnetcore/src/Antiforgery/src/Internal/DefaultAntiforgeryTokenStore.cs

Lines 69 to 74 in 57c3104

    
           catch (IOException ex) 
        
           { 
        
               // Reading the request body (which happens as part of ReadFromAsync) may throw an exception if a client disconnects. 
        
               // Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure. 
        
               throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex); 
        
           }

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 · 2024-10-29T16:23:46Z

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.":

jcracknell changed the title ~~No response for multipart body exceeding MaxHttpRequestBodySize~~ No response for multipart body exceeding MaxRequestBodySize Mar 21, 2022

TanayParikh added the area-runtime label Mar 22, 2022

rafikiassumani-msft added this to the .NET 7 Planning milestone Mar 24, 2022

jcracknell changed the title ~~No response for multipart body exceeding MaxRequestBodySize~~ Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException Mar 25, 2022

rafikiassumani-msft modified the milestones: .NET 7 Planning, .NET 8 Planning Oct 11, 2022

captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023

captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024

LilahTovMoon linked a pull request Oct 29, 2024 that will close this issue

don't throw a misleading exception upon a BadHttpRequestException #58688

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException #40810

Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException #40810

jcracknell commented Mar 21, 2022

jcracknell commented Mar 23, 2022

adityamandaleeka commented Mar 23, 2022

ghost commented Mar 24, 2022

halter73 commented Mar 24, 2022

jcracknell commented Mar 25, 2022 •

edited

Loading

jcracknell commented Apr 20, 2022 •

edited

Loading

ghost commented Oct 11, 2022

chrischu commented Jan 4, 2023

LilahTovMoon commented Oct 29, 2024

Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException #40810

Modelbinding/AntiForgery validation suppresses IOException/BadHttpRequestException #40810

Comments

jcracknell commented Mar 21, 2022

Is there an existing issue for this?

Describe the bug

Expected Behavior

Steps To Reproduce

Exceptions (if any)

.NET Version

Anything else?

jcracknell commented Mar 23, 2022

adityamandaleeka commented Mar 23, 2022

ghost commented Mar 24, 2022

halter73 commented Mar 24, 2022

jcracknell commented Mar 25, 2022 • edited Loading

jcracknell commented Apr 20, 2022 • edited Loading

ghost commented Oct 11, 2022

chrischu commented Jan 4, 2023

LilahTovMoon commented Oct 29, 2024

jcracknell commented Mar 25, 2022 •

edited

Loading

jcracknell commented Apr 20, 2022 •

edited

Loading