Invalid client body encoding results in 500 response, while it should be 4** Error

[sobanieca]

Describe the bug

When client sends request with failed encoding 500 Exception is returned, but it's client fault and I believe it should be 400 Bad Request instead.

To Reproduce

Download solution from https://github.com/sobanieca/AspNetCoreEncodingIssue Open in Visual Studio Run WebApplication1 (ensure it listens on localhost:5000) Run Post-Request.ps1 powershell attached (ensure Powershell ISE working directory is the same as ps1 file, so it can read Request.json file)

Expected behavior

I would expect API to return 400 Bad Request, as client is sending request with invalid encoding

Additional context

Inside Post-Request.ps1 script (line 12) one can see fix that client can apply to fix API issue

Exception that is being thrown:

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1]
      An unhandled exception has occurred while executing the request.
System.Text.DecoderFallbackException: Unable to translate bytes [B2] at index 23 from specified code page to Unicode.
   at System.Text.DecoderExceptionFallbackBuffer.Throw(Byte[] bytesUnknown, Int32 index)
   at System.Text.DecoderExceptionFallbackBuffer.Fallback(Byte[] bytesUnknown, Int32 index)
   at System.Text.DecoderFallbackBuffer.InternalFallback(Byte[] bytes, Byte* pBytes, Char*& chars)
   at System.Text.UTF8Encoding.GetChars(Byte* bytes, Int32 byteCount, Char* chars, Int32 charCount, DecoderNLS baseDecoder)
   at System.Text.DecoderNLS.GetChars(Byte[] bytes, Int32 byteIndex, Int32 byteCount, Char[] chars, Int32 charIndex, Boolean flush)
   at System.Text.DecoderNLS.GetChars(Byte[] bytes, Int32 byteIndex, Int32 byteCount, Char[] chars, Int32 charIndex)
   at Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.ReadIntoBuffer()
   at Microsoft.AspNetCore.WebUtilities.HttpRequestStreamReader.Read(Char[] buffer, Int32 index, Int32 count)
   at Newtonsoft.Json.JsonTextReader.ReadData(Boolean append, Int32 charsRequired)
   at Newtonsoft.Json.JsonTextReader.ParseValue()
   at Newtonsoft.Json.JsonTextReader.Read()
   at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Formatters.JsonInputFormatter.ReadRequestBodyAsync(InputFormatterContext context, Encoding encoding)
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder.BindModelAsync(ModelBindingContext bindingContext)
   at Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, Object value)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

[Tratcher]

Note the repro uses a JSON request body with ANSII (?) encoding. I don't know what the default encoding is for HttpRequestStreamReader, but it's likely UTF-8.

A 500 means there was an unhandled exception while processing the application. The server / error middleware doesn't know the context of the exception so all as treated as 500s.

The only component in this stack that would have sufficient context would be BodyModelBinder or JsonInputFormatter. It would know it was currently parsing the request body and any errors could be converted to 400's.

@mkArtakMSFT

[mkArtakMSFT]

Thanks for contacting us, @sobanieca. Would you be interested in submitting a PR with the fix?

[sobanieca]

@mkArtakMSFT I'd love to submit a PR! However, since I'm not familiar with AspNetCore internals it may take me some time... So if anyone can fix this issue before me that's fine ;)

[sobanieca]

I have managed to find some time and tried to fix this issue. However, I guess I will need some help here :) I have created PR and included my comments there: https://github.com/aspnet/Mvc/pull/8719. One remark: I believe I was able to reproduce similar issue with XmlFormatter. However, error was a bit different, so not sure if it's similar problem. I will try to investigate it further and create separate issue for this if necessary. But first - JSON issue ;)

[johngallardo]

It appears that an attempted fix for this issue hasn't been merged. What is the status and the recommendation for services that are attempting to avoid 500s in this case? I've seen installing an ExceptionFilter for DecoderFallbackException, but that feels a bit heavy handed, since we really only want to trap this exception when it originates during processing of the client provided data.

[BrennanConroy]

Looks like this behavior has improved by default as we use System.Text.Json which throws a JsonException in this case and is handled at https://github.com/dotnet/aspnetcore/blob/3e354f3591a854b498a90097bae6d6207871c4a9/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs#L78 this will result in the Model not being valid and your API being called.

The JsonException.InnerException.InnerException is a DecoderFallbackException so we might want to consider special casing that and returning a 400 instead as well as logging a nice error message.

Failed to decode the HTTP request body content using the selected character encoding - {EncodingName}. {Exception.Message}

Also, we sould update the NewtonsoftJsonInputFormatter with the same fix, or alternatively put some shared logic in TextInputFormatter.ReadRequestBodyAsync and throw the DecoderFallbackException from SystemTextJsonInputFormatter.

[ghost]

Thanks for contacting us. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. 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.

[vchirikov]

It also happens with a custom type in query string + binding via IParsable/ISpanParsable the result is 500 with exception Microsoft.AspNetCore.Http.BadHttpRequestException: Failed to bind parameter

[ps-weber]

This bug has been open for a while, but I still think its relevant. Some use-cases such as JsonPatch still require Newtonsoft.Json, and Powershell makes it really easy to accidentally mess up the encoding.

[nonconformee]

Thanks for contacting us. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. 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.

Can't be that hard to implement something like this:

builder.Services.Configure<RouteHandlerOptions>(o => o.Use400InsteadOf500StatusCode = true);