Skip to content
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

.NET 5: Null body + content-type with no content-length = validation error even with AllowEmptyInputInBodyModelBinding = true #34980

Open
JohnGalt1717 opened this issue Aug 3, 2021 · 5 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing feature-model-binding help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Milestone

Comments

@JohnGalt1717
Copy link

JohnGalt1717 commented Aug 3, 2021

Describe the bug

Even if you set AllowEmptyInputInBodyModelBinding = true it's basically impossible to have a nullable [FromBody] with no value that works across platforms because it still requires the content-length to be set, which browsers won't let you set, and won't set themselves on a null/empty or even "{}" body.

The standards say that if no body is passed you don't need to send the content-length header (for obvious reasons) thus the current implementation is not following standards and validating AllowEmptyInputInBodyModelBinding against the content-length header if and only if it is set when it should be if it is set to 0 or it doesn't exist.

To Reproduce

public async Task<ActionResult<IEnumerable<SubscriptionDto>>> ListSubscriptions(string? contactId = null, [FromBody] LoadCriteriaDto? criteria = null, CancellationToken cancellationToken = default)

This should allow a null criteria property to be received.

This does work with Android because Android allows setting the content-length header so you can set it to 0 if there is no body and still set the content-type = "application/json". (if you don't set the content-type you get a media type not supported even though the body is null which I believe is a separate bug because if the body is null, then the content-type should be null and it shouldn't be testing/validating it.)

As long as the Content-Length header is set on the request, and the body is null/empty then .net will accept the empty body. However as soon as the Content-Length header is not set but the body is null/empty then AllowEmptyInputInBodyModelBinding doesn't take effect and you get a validation error which is not correct and very problematic because all Chromium/Firefox browsers do not set the Content-Length header in this scenario AND you can't set it yourself because it's a protected header thus there is no way to actually send an empty body from a browser and have .NET accept it as an empty body.

Exceptions (if any)

Invaid json token

Proposed Solution:

  1. if AllowEmptyInputInBodyModelBinding = true, and the body is empty/null/{} and the content-length is not set/null then it should still work and not throw a validation error. (you can make this work by injecting middleware that if it sees no Content-length header, adds it to the request pipeline as a work around for now, but that isn't optimal)
  2. If the body is null, and the content-type header is not set, it shouldn't evaluate the media type because the deserializer is immaterial because there is no body and thus nothing to deserialize and combined with No. 1, should work fine with no content-type, no content-length, no body and AllowEmptyInputInBodyModelBinding = true.
  3. The inline FromBody attribute's ability to allow an empty body should also be updated to the same behavior.

Further technical details

  • ASP.Net core 5.0.302

.NET SDK (reflecting any global.json):
Version: 5.0.302
Commit: c005824e35

Runtime Environment:
OS Name: Windows
OS Version: 10.0.22000
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\5.0.302\

Host (useful for support):
Version: 6.0.0-preview.6.21352.12
Commit: 770d630b28

.NET SDKs installed:
5.0.302 [C:\Program Files\dotnet\sdk]
6.0.100-preview.6.21355.2 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
Microsoft.AspNetCore.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.0-preview.6.21355.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.0-preview.6.21352.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 6.0.0-preview.6.21353.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

VS 2019

@mkArtakMSFT mkArtakMSFT added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-model-binding labels Aug 3, 2021
@adityamandaleeka adityamandaleeka added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Aug 4, 2021
@adityamandaleeka adityamandaleeka added this to the Backlog milestone Aug 4, 2021
@ghost
Copy link

ghost commented Aug 4, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@halter73 halter73 added the feature-minimal-actions Controller-like actions for endpoint routing label Aug 4, 2021
@halter73
Copy link
Member

halter73 commented Aug 4, 2021

We could probably support this by peaking into chunked bodies with BodyReader.ReadAsync() to see if the body is empty before attempting to deserialize as JSON. We would want to do the same thing for optional body parameters in minimal actions (i.e. The new MapGet/MapPost/etc methods that take Delegate instead of RequestDelegate).

@JohnGalt1717
Copy link
Author

@halter73 So there's 2 parts. I think the first part is just if allowemptybody = true, and content-length = 0 or content-length = null OR content-length is not set, it shouldn't validate.

This is the trivial case and to me is a minor change to the validation logic that is in there right now.

The second part that you're referring is that It's trying to decode null in this case, which, again couldn't it just skip that flow if the content header is set to 0 or is null or not set at all? That would be way less costly than peeking at a stream.

@halter73
Copy link
Member

halter73 commented Aug 6, 2021

The problem is that content-length not being set doesn't mean there's no body. If it's a POST and there's no content-length, it might have a HTTP/1.1. chunked request body or use HTTP/2 framing to determine the content length lazily.

If it's HTTP/1.1 and there's no content-length or transfer-encoding header, we might not have to peek, but in some cases we still would.

@JohnGalt1717
Copy link
Author

@halter73 Yuk. Well then shouldn't the flag that is already there for allow empty just mean it out right? Don't check at all unless content-length is set? Other parts of the pipeline will collect it like a non-nullable type on the [FromBody] property, or property on a POST as an exmaple. To me this whole thing seems like a legacy of before nullables and the nullability of the property on the webapi function should control this entirely and be done as part of the pipeline. And if there is nothing coming in, the serializers should all just return null instead of trying to process.

@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
@captainsafia 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 area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc labels Jun 6, 2023
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Aug 24, 2023
@mkArtakMSFT mkArtakMSFT added help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team and removed help wanted Up for grabs. We would accept a PR to help resolve this issue labels Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing feature-model-binding help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team
Projects
None yet
Development

No branches or pull requests

6 participants