JSON "null" request body rejected as an empty request body #41002

senbar · 2022-04-01T13:05:56Z

JSON "null" response body rejected as an empty response body

I have added check if request body has 0 bytes; If yes then body was actually empty and proper error message is returned; If not then json contained null and different message is returned.

Should I create new error message for this null case in resources? I have changed it to ValueMustNotBeNullAccessor but it doesn't seem appropriate since it was for a bit different cases elsewhere.

Fixes #40415

remove null coalescing since bodylength is null for null value in json

senbar · 2022-04-06T08:52:06Z

I think this should do it. If anyone has better idea how to check whether request was empty or with null value please let me know. Also the error message is up for debate I dont think the one I used is clear at all

src/Mvc/Mvc.Core/src/ModelBinding/Binders/BodyModelBinder.cs

brunolins16 · 2022-04-07T16:42:57Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/BodyModelBinder.cs

- bindingContext.ModelState.AddModelError(modelBindingKey, message);
+
+ // If request content length is 0 then request had empty body, if not null was provided in json
+ if (httpContext.Request.ContentLength == 0)


I don't feel that this is the right place to do this. Maybe an option is change the json-based inpur formatters to AddModelError and return Failure instead

aspnetcore/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

Line 206 in 1c5da27

return InputFormatterResult.NoValue();

aspnetcore/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs

Line 111 in 1c5da27

return InputFormatterResult.NoValue();

Please check if we do have any side effects with this change.

If we do need to detect the presence of a request body, we should use the feature. See: https://github.com/dotnet/aspnetcore/pull/39917/files#diff-f137ebb8705f77c3c09cf21ca959b40735794ec71b8fdec0c7d4f0a0b3a7fb34R106

Thanks for feedback! I agree with suggestions and am working on them

senbar · 2022-04-24T20:03:37Z

So I think there should be some sort of ModelState enum in InputFormatterResult class indicating whether model is set null or undefined.

senbar · 2022-04-27T18:33:46Z

I made commit with changes that I think would improve API here. I have added enum describing state of model binding (set/null/empty body). All the logic checking if model is in these states is broken so don't look into that. I haven't made any api changes proposal yet so I dont entirely know how the process goes. That's why I would appreciate here feedback if this is the way I should finish this issue.

brunolins16 · 2022-04-27T19:01:02Z

I made commit with changes that I think would improve API here. I have added enum describing state of model binding (set/null/empty body). All the logic checking if model is in these states is broken so don't look into that. I haven't made any api changes proposal yet so I dont entirely know how the process goes. That's why I would appreciate here feedback if this is the way I should finish this issue.

@senbar That is an interesting change, however, as you mentioned that will be a large change to how the states are checked. Since the goal is to just provide an more descriptive error message, for JSON "null", but keep rejecting the request as before. I still feel you could try something easier.

If you take a look where the ReadRequestBodyAsync is called (here), it is only called when the body is detected that means the SystemTextJsonInputFormatter or NewtonsoftJsonInputFormatter will only do the serialization when a body is detected. With that, both formatters will do the serialization and check if the model is null and report back the NoValue state, that is where I feel we can do better, something like

if (model == null && !context.TreatEmptyInputAsDefaultValue)
{
    if ( [check for JSON-encoded value "null"]) 
    { 
        bindingContext.ModelState.AddModelError(context.ModelName, "The new message");
        return InputFormatterResult.Failure();
    } 

    // Some nonempty inputs might deserialize as null, for example whitespace. 
    // The upstream BodyModelBinder needs to
    // be notified that we don't regard this as a real input so it can register
    // a model binding error.
    return InputFormatterResult.NoValue();
}

This reverts commit c960a75.

senbar · 2022-05-05T17:33:03Z

To check if request is null here would need reading Request.Body stream in

if (model == null && !context.TreatEmptyInputAsDefaultValue)
{
        var bodyStreamReader = new StreamReader(httpContext.Request.Body);
        bodyStreamReader.BaseStream.Seek(0, SeekOrigin.Begin);
        string rawJson = await bodyStreamReader.ReadToEndAsync();
        if(rawJson=="null")
        {
            context.ModelState.TryAddModelError(context.ModelName, "null value not supported");
            return InputFormatterResult.Failure();
        }

        // Some nonempty inputs might deserialize as null, for example whitespace,
        // or the JSON-encoded value "null". The upstream BodyModelBinder needs to
        // be notified that we don't regard this as a real input so it can register
        // a model binding error.
        return InputFormatterResult.NoValue();
}

which doesn't work because Request.Body stream is not seekable in general. Any ideas what should I do instead? Buffering whole body stream would have big performance impact.

halter73 · 2022-05-05T20:59:04Z

With that, both formatters will do the serialization and check if the model is null and report back the NoValue state, that is where I feel we can do better, something like

if (model == null && !context.TreatEmptyInputAsDefaultValue)
{
    if ( [check for JSON-encoded value "null"]) 
    { 
        bindingContext.ModelState.AddModelError(context.ModelName, "The new message");
        return InputFormatterResult.Failure();
    } 

    // Some nonempty inputs might deserialize as null, for example whitespace. 
    // The upstream BodyModelBinder needs to
    // be notified that we don't regard this as a real input so it can register
    // a model binding error.
    return InputFormatterResult.NoValue();
}

I'm not sure we want to report failure at such a low layer. After all, null is one of the "nonempty inputs might deserialize as null" that the BodyModelBinder needs to know about. I don't think that either !context.TreatEmptyInputAsDefaultValue nor EmptyBodyBehavior.Disallow implies that a null literal should be rejected outright. It should be if the parameter is non-nullable, but we don't know that at the InputFormatter layer do we?

We probably should be changing this to return InputFormatterResult.Success(null); for null literals instead of InputFormatterResult.NoValue(); regardless of TreatEmptyInputAsDefaultValue because it's not truly empty.

It's a literal default value, and the whole problem here seems to be that both empty input and null literals are being collapsed into a single NoValue() when TreatEmptyInputAsDefaultValue is false. It's fine for these values to be collapsed when TreatEmptyInputAsDefaultValue is true, because that's what you asked for, but not the other way around.

I don't think it's necessary to check if there was an empty request body or null because the method you're modifying, ReadRequestBodyAsync, is only called when the body is non-empty (or at least we think it will be) that what the canHaveBody logic I linked to earlier was doing. It was deciding whether to call ReadRequestBodyAsync or return NoValue() immediately:

aspnetcore/src/Mvc/Mvc.Core/src/Formatters/InputFormatter.cs

Lines 102 to 116 in 3921939

 var canHaveBody = context.HttpContext.Features.Get<IHttpRequestBodyDetectionFeature>()?.CanHaveBody; 

 // In case the feature is not registered 

 canHaveBody ??= context.HttpContext.Request.ContentLength != 0; 

 if (canHaveBody is false) 

 { 

 if (context.TreatEmptyInputAsDefaultValue) 

 { 

 return InputFormatterResult.SuccessAsync(GetDefaultValueForType(context.ModelType)); 

 } 

 return InputFormatterResult.NoValueAsync(); 

 } 

 return ReadRequestBodyAsync(context);

And if the canHaveBody logic was wrong, and we thought there was going to be a body but it turned out to be empty, JsonSerializer.DeserializeAsync() in SystemTextJsonInputFormatter will throw a JsonException which will always be turned into a Failure().

Unfortunately, Json.NET turns an empty body into null when you try to deserialize. Again the canHaveBody checks should prevent this from happening for most empty bodies but not all. I assume we copied the if (model == null && !context.TreatEmptyInputAsDefaultValue) logic from the NewtonsoftJsonInputFormatter to the SystemTextJsonInputFormatter because it makes very little sense in the latter case where it will only return null for a null literal and nothing else.

@brunolins16 Is there anything wrong with just removing the if (model == null && !context.TreatEmptyInputAsDefaultValue) branch entirely SystemTextJsonInputFormatter and always return InputFormatterResult.Success(model); when JsonSerializer.DeserializeAsync() doesn't throw?

I imagine we'll have to be more careful with the Json.NET input formatter for back compat. It's always reliably treated null and empty bodies as exactly the same because they were indistinguishable from just the call to Deserialize. We could in theory try to test if any bytes were actually read from the body stream after the fact to determine the difference, but that would be a breaking change.

senbar · 2022-05-05T22:52:49Z

So isn't this a problem with entire handling of model binding of "null" json on nullable parameter with TreatEmptyInputAsDefaultValue==true? If we get "null" json it gets deserialized to null model which returns
NoValue from formatter. This in turn leads to model binding error since:

// If the input formatter gives a "no value" result, that's always a model state error,

From what I understand TreatEmptyInputAsDefaultValue refers to empty request body not json "null" literal right?

It should be if the parameter is non-nullable, but we don't know that at the InputFormatter layer do we?

But we do know the type of deserialized object since we are getting it from context.ModelType for deserialization in InputFormatter right? Also the same comment I quoted earlier states:

// If the input formatter gives a "no value" result, that's always a model state error,
// because BodyModelBinder implicitly regards input as being required for model binding.
// If instead the input formatter wants to treat the input as optional, it must do so by
// returning InputFormatterResult.Success(defaultForModelType), because input formatters
// are responsible for choosing a default value for the model type.

So I think it is implied that formatter should be handling dealing with optional arguments and stuff like that.

As for checking if json is null literal I was thinking about the case described in the comment:
// Some nonempty inputs might deserialize as null, for example whitespace, (...)
so I wanted to check if the input we got is actually a null literal or some whitespace. From what I can see know whitespace actually makes deserialization throw "The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. Path: $ | LineNumber: 0 | BytePositionInLine: 1." . Since the comment made it clear there are some inputs being regarded as null I wanted to make sure the json is null literal (which I am not entirely sure how to do since request stream is of course non seekable).

brunolins16 · 2022-05-05T23:33:53Z

So isn't this a problem with entire handling of model binding of "null" json on nullable parameter with TreatEmptyInputAsDefaultValue==true?

I do not think that the condition is wrong just because we did some improvements (.NET 7) in how we are treating the AllowEmptyBody based on the Nullabity in I feel only very specific scenarios will allow empty body and not null or vice versa.

@brunolins16 Bruno Lins de Oliveira FTE Is there anything wrong with just removing the if (model == null && !context.TreatEmptyInputAsDefaultValue) branch entirely SystemTextJsonInputFormatter and always return InputFormatterResult.Success(model); when JsonSerializer.DeserializeAsync() doesn't throw?

@halter73 As we discussed, "null" and empty body are different, however, MVC treat them as the same for STJ for a long time and change might cause some breaking change. We changed to make the empty body better and most of the cases when a "null" is accepted will be covered by this new mechanism, as example:

public ActionResult([FromBody] Contact? contact) {}

or

public ActionResult([FromBody] Contact contact = null) {}

Until .NET 6 they used to be blocked for both (null and empty body) because we were not using the optionality to allow them. Now, .NET 7, both actions will be accepted (null and empty body).

Since the mechanism will behave differently, as I mentioned, not change it right now might be the best and going back to the original idea of just update the error message to differentiate between Empty Body and null is a less breaking change.

Since the comment made it clear there are some inputs being regarded as null I wanted to make sure the json is null literal (which I am not entirely sure how to do since request stream is of course non seekable).

I think the comment about whitespace is not true, as you mentioned, and the only situation where we will get a null value from the System.Text.Json will be when we have a literal null, so i feel we can safe say (only for System.Text.Json) this:

if (model == null && !context.TreatEmptyInputAsDefaultValue)
{
            context.ModelState.TryAddModelError(context.ModelName, "null value not supported");
            return InputFormatterResult.Failure();
}

That will produce a better error message, without change the current behavior. And the new mechanism or the user option to allow empty body will be used to decide if the failure will happen or we will treat as Success binding.

Also, I think we should keep the NewtonsoftJsonInputFormatter as-is for now.

@halter73 anything to add based on our discussion?
@senbar is that make sense for you?

senbar · 2022-05-06T08:38:22Z

I am not sure if I understand correctly how it deals with nullable types. When I run test with (/contact is simple endpoint with Contact class object argument from body):

 var response = await Client.PostAsJsonAsync<Contact>("/contact", default);

model binding returns error even though Contact is nullable and null literal was provided.
The reason is I as I mentioned above: model gets set to null and SystemTextJsonInputFormatter returns NoValue which binder interprets as an error. Isn't this incorrect or do I misunderstand behaviour we are going for?

brunolins16 · 2022-05-06T14:04:48Z

I am not sure if I understand correctly how it deals with nullable types. When I run test with (/contact is simple endpoint with Contact class object argument from body):
 var response = await Client.PostAsJsonAsync<Contact>("/contact", default);
model binding returns error even though Contact is nullable and null literal was provided. The reason is I as I mentioned above: model gets set to null and SystemTextJsonInputFormatter returns NoValue which binder interprets as an error. Isn't this incorrect or do I misunderstand behaviour we are going for?

We changed the behavior in .NET 7 and a nullable Contact will set the TreatEmptyInputAsDefaultValue as true. So, a null literal and empty body will be accepted.

remove logic from BodyModelBinder; Fix messgaes on ApiBehaviortest

senbar · 2022-05-18T07:54:19Z

@brunolins16 I have made suggested changes to SytemTextJsonInputFormatter and added tests for it.
The question is should I do something with ApiBehaviorTest since it currently relies on SystemTextJsonInputFormatter being used (my null prarameter test would fail if Json.net was used). This may not be the best practise in functional test.

Edit: I see some tests are failing, will fix that shortly

brunolins16 · 2022-05-18T15:32:05Z

@brunolins16 I have made suggested changes to SytemTextJsonInputFormatter and added tests for it. The question is should I do something with ApiBehaviorTest since it currently relies on SystemTextJsonInputFormatter being used (my null prarameter test would fail if Json.net was used). This may not be the best practise in functional test

Let me do some investigation and will reply to you soon. Thanks for your patience and contribution.

brunolins16 · 2022-05-18T20:45:32Z

@senbar I just took a look and i think the easiest way is to add the new Functional Tests to https://github.com/brunolins16/aspnetcore/blob/main/src/Mvc/test/Mvc.FunctionalTests/SystemTextJsonInputFormatterTest.cs , instead APIBehaviorTests, similar to what we have here:

https://github.com/brunolins16/aspnetcore/blob/e9b9fc0c533b120ce5c0317ee8e3a56aca26d37d/src/Mvc/test/Mvc.FunctionalTests/NewtonsoftJsonInputFormatterTest.cs#L35

brunolins16 · 2022-05-18T20:50:12Z

src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs

- // be notified that we don't regard this as a real input so it can register
- // a model binding error.
- return InputFormatterResult.NoValue();
+ context.ModelState.TryAddModelError(context.ModelName, "null value not supported");


What do you think about something like The supplied value 'null' is invalid or The value 'null' is invalid?

brunolins16 · 2022-05-18T20:53:16Z

src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs

@@ -86,6 +86,28 @@ public async Task ReadAsync_SingleError()
 });
 }

+ [Fact]
+ public async Task ReadAsync_WithJsonNullLiteralWithoutTreatEmptyInputAsDefault_ShouldSetNullValueNotSupportedError()


Maybe you could add a test with treatEmptyInputAsDefaultValue = true?

brunolins16 · 2022-05-18T20:54:41Z

src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs

+ var httpContext = GetHttpContext(contentBytes);
+
+ var formatterContext = CreateInputFormatterContext(typeof(int?), httpContext);
+ // TreatEmptyInputAsDefaultValue is set to false so even if type is nullable we should gget 'null value not supported'


Suggested change

// TreatEmptyInputAsDefaultValue is set to false so even if type is nullable we should gget 'null value not supported'

// TreatEmptyInputAsDefaultValue is set to false so even if type is nullable the formatter should report a failure.

brunolins16 · 2022-05-18T21:01:40Z

src/Mvc/test/Mvc.FunctionalTests/ApiBehaviorTest.cs

+ }
+
+ [Fact]
+ public virtual async Task ActionWithNonOptionalParameterReturnsBodyCannotBeEmpty_WhenInvokedWithEmptyBody()


Suggested change

public virtual async Task ActionWithNonOptionalParameterReturnsBodyCannotBeEmpty_WhenInvokedWithEmptyBody()

public Task JsonInputFormatter_ReturnsBadRequest_ForEmptyRequestBodyAction()

As mentioned, maybe you should move it to https://github.com/brunolins16/aspnetcore/blob/main/src/Mvc/test/Mvc.FunctionalTests/SystemTextJsonInputFormatterTest.cs

brunolins16 · 2022-05-18T21:03:41Z

src/Mvc/test/Mvc.FunctionalTests/ApiBehaviorTest.cs

@@ -82,6 +83,73 @@ public virtual async Task ActionsReturnBadRequest_WhenModelStateIsInvalid()
 }
 }

+ [Fact]
+ public virtual async Task ActionWithNonOptionalParameterReturnsNullIsNotPermited_WhenInvokedWithNullParameter()


Suggested change

public virtual async Task ActionWithNonOptionalParameterReturnsNullIsNotPermited_WhenInvokedWithNullParameter()

public Task JsonInputFormatter_ReturnsBadRequest_ForLiteralNull()

senbar · 2022-06-10T12:33:41Z

Sorry have been busy last weeks, I will be finishing PR soon

brunolins16 · 2022-08-01T20:35:20Z

Sorry have been busy last weeks, I will be finishing PR soon

@senbar are you still planning to work on it? Can I help you on anything?

senbar · 2022-08-03T08:52:41Z

Yeah sorry aobut that I was changing job lately and was swamped with work. I will try to finish it next two weeks. As for help I think I got it it should be pretty straightforward, if there are any problems I will let You know, thanks

brunolins16 · 2022-08-03T15:09:30Z

Don't worry about it, take your time. I was just checking if I could do anything to help.

ghost · 2022-12-19T00:00:58Z

Hi @senbar.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

add check if body contains bytes add tests

f68b095

senbar requested a review from a team as a code owner April 1, 2022 13:05

ghost added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member labels Apr 1, 2022

senbar marked this pull request as draft April 1, 2022 13:13

fix broken tests for error message change;

42fb020

remove null coalescing since bodylength is null for null value in json

senbar marked this pull request as ready for review April 6, 2022 10:14

TanayParikh added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Apr 6, 2022

TanayParikh requested review from rafikiassumani-msft and brunolins16 April 6, 2022 16:59

brunolins16 reviewed Apr 7, 2022

View reviewed changes

src/Mvc/Mvc.Core/src/ModelBinding/Binders/BodyModelBinder.cs Outdated Show resolved Hide resolved

brunolins16 reviewed Apr 7, 2022

View reviewed changes

mkArtakMSFT assigned rafikiassumani-msft Apr 11, 2022

rafikiassumani-msft requested a review from a team April 11, 2022 17:13

senbar requested a review from a team as a code owner April 27, 2022 18:28

Change of api proposal with broken changes

c960a75

senbar requested a review from brunolins16 April 27, 2022 18:34

Revert "Change of api proposal with broken changes"

173dd0d

This reverts commit c960a75.

halter73 changed the title ~~JSON "null" response body rejected as an empty response body~~ JSON "null" request body rejected as an empty request body May 5, 2022

add tests for nul value not supported message on InputFormatterTest;

15cefd4

remove logic from BodyModelBinder; Fix messgaes on ApiBehaviortest

senbar force-pushed the JSON_null_body branch from 632881f to 15cefd4 Compare May 18, 2022 06:37

brunolins16 reviewed May 18, 2022

View reviewed changes

captainsafia added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Dec 6, 2022

ghost added the stale Indicates a stale issue. These issues will be closed automatically soon. label Dec 19, 2022

ghost closed this Dec 23, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON "null" request body rejected as an empty request body #41002

JSON "null" request body rejected as an empty request body #41002

senbar commented Apr 1, 2022

senbar commented Apr 6, 2022

brunolins16 Apr 7, 2022

halter73 Apr 11, 2022 •

edited

Loading

senbar Apr 13, 2022 •

edited

Loading

senbar commented Apr 24, 2022

senbar commented Apr 27, 2022

brunolins16 commented Apr 27, 2022

senbar commented May 5, 2022

halter73 commented May 5, 2022 •

edited

Loading

senbar commented May 5, 2022

brunolins16 commented May 5, 2022 •

edited

Loading

senbar commented May 6, 2022 •

edited

Loading

brunolins16 commented May 6, 2022

senbar commented May 18, 2022 •

edited

Loading

brunolins16 commented May 18, 2022

brunolins16 commented May 18, 2022

brunolins16 May 18, 2022

brunolins16 May 18, 2022

brunolins16 May 18, 2022

brunolins16 May 18, 2022

brunolins16 May 18, 2022

brunolins16 May 18, 2022

senbar commented Jun 10, 2022

brunolins16 commented Aug 1, 2022

senbar commented Aug 3, 2022

brunolins16 commented Aug 3, 2022

ghost commented Dec 19, 2022

	// TreatEmptyInputAsDefaultValue is set to false so even if type is nullable we should gget 'null value not supported'
	// TreatEmptyInputAsDefaultValue is set to false so even if type is nullable the formatter should report a failure.

	public virtual async Task ActionWithNonOptionalParameterReturnsBodyCannotBeEmpty_WhenInvokedWithEmptyBody()
	public Task JsonInputFormatter_ReturnsBadRequest_ForEmptyRequestBodyAction()

JSON "null" request body rejected as an empty request body #41002

JSON "null" request body rejected as an empty request body #41002

Conversation

senbar commented Apr 1, 2022

senbar commented Apr 6, 2022

brunolins16 Apr 7, 2022

Choose a reason for hiding this comment

halter73 Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

senbar Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

senbar commented Apr 24, 2022

senbar commented Apr 27, 2022

brunolins16 commented Apr 27, 2022

senbar commented May 5, 2022

halter73 commented May 5, 2022 • edited Loading

senbar commented May 5, 2022

brunolins16 commented May 5, 2022 • edited Loading

senbar commented May 6, 2022 • edited Loading

brunolins16 commented May 6, 2022

senbar commented May 18, 2022 • edited Loading

brunolins16 commented May 18, 2022

brunolins16 commented May 18, 2022

brunolins16 May 18, 2022

Choose a reason for hiding this comment

brunolins16 May 18, 2022

Choose a reason for hiding this comment

brunolins16 May 18, 2022

Choose a reason for hiding this comment

brunolins16 May 18, 2022

Choose a reason for hiding this comment

brunolins16 May 18, 2022

Choose a reason for hiding this comment

brunolins16 May 18, 2022

Choose a reason for hiding this comment

senbar commented Jun 10, 2022

brunolins16 commented Aug 1, 2022

senbar commented Aug 3, 2022

brunolins16 commented Aug 3, 2022

ghost commented Dec 19, 2022

halter73 Apr 11, 2022 •

edited

Loading

senbar Apr 13, 2022 •

edited

Loading

halter73 commented May 5, 2022 •

edited

Loading

brunolins16 commented May 5, 2022 •

edited

Loading

senbar commented May 6, 2022 •

edited

Loading

senbar commented May 18, 2022 •

edited

Loading