Report good error messages when JSON fails to deserialize

[DaveVdE]

When the JsonInputFormatter detects an error in deserialising JSON (e.g. required properties) the ModelState.IsValid property is to false and there are ModelError instances to be found, but their ErrorMessage property is empty while their Exception has a perfectly good error message to use.

[javiercn]

@dougbu I believe a model state error contains either an error message or an exception but not both, normally validators add model state errors when they find an invalid property value in an object and exceptions in the case something gets thrown. You don't want to mix those two up as you want to prevent returning call stacks as part of your error messages.

[dougbu]

@DaveVdE to add to what @javiercn said: MVC special-cases a few Exceptions e.g. FormatException and uses specific error messages when adding them to the ModelStateDictionary. But sending Exception.Message to clients exposes too much information about the site's infrastructure and is not something MVC does.

[DaveVdE]

I agree with all you said, but JSON formatting errors are especially interesting to the client since they have to fix the JSON. I'm currently doing something akin to this:

                var errorMessages = context.ModelState.Values.SelectMany(value => value.Errors).Select(error => error.Exception?.Message ?? error.ErrorMessage);
                var message = string.Join("\n", errorMessages);

                context.Result = new BadRequestErrorMessageResult(message);

which gives me:

{
"Message": "Required property 'created' not found in JSON. Path '', line 4, position 1. Could not convert string to DateTimeOffset: 2016-09-0. Path 'created', line 3, position 23."
}

because if I just use a BadRequestObjectResult and pass the ModelState, what comes out is not very useful:

{"":["The input was not valid."],"created":["The input was not valid."]}

[rynowak]

@blowdart @JamesNK - I think this topic has come up a few times and I don't recall the answer, how much of a security problem is it to put JSON.NET's exception messages in the errors returned to clients.

[blowdart]

I guess it depends what's in them. For example would the values sent appear? In which case, oh heck no, because someone, somewhere is going to send something sensitive. Better to log errors server side, and return a correlation ID or something of that ilk.

[DaveVdE]

Well in my case it's a client sending an API request. I'd like to send back errors from JSON.NET to help in getting it running, instead of leaving the client in the dark.

I'm fine as it is, I've worked around it. I just felt the ModelState errors was stuff to send to the client.

[blowdart]

If it were opt-in then I'd be o with it @rynowak, just not the default. And of course commented with a "Here be dragons" type comment.

[DaveVdE]

So should I close this issue?

[dougbu]

@DaveVdE don't close the issue. I am pretty sure we're not tracking the feature @blowdart and @rynowak discuss above anywhere else.

[Eilon]

@JamesNK any thoughts on this? We're thinking that perhaps Json.NET could have some new interface, such as ISafeExceptionMessage that has a string SafeExceptionMessage { get; } property that is guaranteed to have a "safe" error message - that is, an error message that would never contain any sensitive data.

Or does Json.NET already have a mechanism to extract safe information from general parsing exceptions, etc.?

[rynowak]

/cc @JamesNK

[JamesNK]

There are a lot of throw statements in Json.NET and doubling the number of error messages sounds like a lot of work.

[rynowak]

I think a less wacky idea would be for us to handle JsonReaderException and either get agreement that those error messages are safe - OR make our own generic error message and use the line/col information.

The main issue that our users are running into right now is that it's non trivial to send an error back to the client when the JSON is invalid. Us doing either of the above would meet that requirement.

[SteveSandersonMS]

Implemented in #7015

[bindu24]

Can we now return good error messages instead of the exception message where sensitive information is exposed