JsonInputFromatter validation issue

[ArtemAstashkin]

Is this a Bug or Feature request?:

Bug

Steps to reproduce (preferably a link to a GitHub repo with a repro project):

1. You have an action in a Controller
2. The action has a requestModel with array of integers
3. You send a request to the action with invalid model - for example array with string.Empty instead integers
4. You expect to receive BadRequest, but you will receive Http Ok after a long period of time

I've attached the repo to reproduce this issue https://github.com/ArtemAstashkin/JsonInputFormatterTest

1. The repo has 2 projects (Web and Web.Tests). Web has HomeController and Post action with RequestModel

    public class RequestModel
    {
        public string Key { get; set; }

        [FromBody]
        [Required]
        public PostRequestModelBody Body { get; set; }
    }

2. The Web.Tests project has only one specific test to reproduce the issue

        [Fact]
        public async Task ShouldReturnBadRequest()
        {
            var itemIds = Enumerable.Repeat(string.Empty, 621).ToArray();
            var body = JSON.SerializeDynamic(new { ItemIds = itemIds });

            var request = new HttpRequestMessage
            {
                Method = HttpMethod.Post,
                RequestUri = new Uri("/api/home?key=test", UriKind.Relative),
                Content = new StringContent(body, Encoding.UTF8, "application/json"),
            };

            var sut = await Sut.AsyncInstance;
            var response = await sut.SendAsync(request);

            Assert.Equal(
                expected: HttpStatusCode.BadRequest,
                actual: response.StatusCode);
        }

The test will fail. But, if you change 621 to 199 it will pass.

Description of the problem:

The issue is related to the JsonInputFormatter which tries to mark validation errors as handled and validate all items in arrays and all properties in a model. But it take a lot of time. Moreover it has a limit of exceptions (200 as default). Eventually if somebody sends invalid model with 600 string.Empty (for example, 201 - enough) in array instead of integers, the formatter will spend a lot of time to parse it without any expected result - because it will fail after 200th error and return status OK instead of BadRequest.

It has 2 problems:

1. validation doesn't work properly
2. performance issue - formatter spends a lot of time to try to parse all invalid values so a malefactor can cause serious performance issues on online high-load services

Version of Microsoft.AspNetCore.Mvc or Microsoft.AspNetCore.App or Microsoft.AspNetCore.All:

2.1.1

[mkArtakMSFT]

Thanks for contacting us, @ArtemAstashkin.
@dougbu, can you please look into this? Thanks!

[dougbu]

validation doesn't work properly

More that input formatting intentionally doesn't fill the ModelStateDictionary with numerous (and in this case redundant) errors. To see all of the errors, set context.ModelState.MaxValidationDepth to a higher number e.g. 1000 in an IResourceFilter implementation. It seems the 200 default is too low for your scenario.

I suspect this would worsen performance slightly.

performance issue

Please define "spends a lot of time". In any case, we would either need to abort input formatting early (which conflicts with your first concern) or make Exception handling faster in .NET Core to speed this case up.

---

FYI 2.1.1 is rather old. The current patch version of Microsoft.AspNetCore.All and Microsoft.AspNetCore.App is 2.1.6.

[ArtemAstashkin]

200 default is too low for your scenario.

I think the limit does not matter. If it exists somebody who knows about it will be able to generate unexpected request and damage a service.

I suspect this would worsen performance slightly

No, in my case it takes several hundreds of milliseconds to parse this request for the first time and several dozens for the next times. For example 300-500 ms and 30-60 ms which seems too much.

Please define "spends a lot of time". In any case, we would either need to abort input formatting early (which conflicts with your first concern) or make Exception handling faster in .NET Core to speed this case up.

I'm sure it is not significant to get all errors. I'm sure the better (faster, and easiest) case - to break the validation on the first error, no to try to collect hundreds.
In my case I have replaced the JsonInputFromatter with custom formatter based in Jil JsonDeserializer and it made serialization dramatically faster - 3-5 ms for any cases - much less than previous numbers! It will break serialization on the first error and return BadRequest as fast as it can.

FYI 2.1.1 is rather old.

It does not depend on the version - all current versions are the same according to this behavior.

[dougbu]

@ArtemAstashkin you have said

validation doesn't work properly

Do you mean the reported errors are incomplete? Or, is this a reiteration of your complaint that input formatting does not stop after the first error is encountered?

[ArtemAstashkin]

I mean that instead of BadRequest status we receive HttpStatusCode.Ok despite invalid model, without any validation errors.

[dougbu]

I mean that instead of BadRequest status we receive HttpStatusCode.Ok despite invalid model, without any validation errors.

That's due to the code in your action:

if (this.ModelState.IsValid)
{
    return this.Ok();
}

[ArtemAstashkin]

Yes, it is related to the realization.
Although BadRequest is a common result for an invalid request I only wanted to simplify the example and emphasize that ModelState can be incorrect in some cases in other words contains unexpected state.

[dougbu]

@ArtemAstashkin I see now that this issue is a duplicate of aspnet/Mvc#7992. That bug was fixed in ASP.NET Core 2.2.0 (with aspnet/Mvc@f2608c2ff4). Suggest upgrading your application to 2.2.1, the latest 2.2.x release.