Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Report good error messages when JSON fails to deserialize #4862

Closed
@DaveVdE

Description

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.

Activity

javiercn

javiercn commented on Jun 14, 2016

@javiercn
Member

@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

dougbu commented on Jun 14, 2016

@dougbu
Member

@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

DaveVdE commented on Jun 14, 2016

@DaveVdE
Author

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

rynowak commented on Jun 14, 2016

@rynowak
Member

@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

blowdart commented on Jun 14, 2016

@blowdart
Member

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

DaveVdE commented on Jun 14, 2016

@DaveVdE
Author

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

blowdart commented on Jun 14, 2016

@blowdart
Member

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

DaveVdE commented on Jun 15, 2016

@DaveVdE
Author

So should I close this issue?

dougbu

dougbu commented on Jun 15, 2016

@dougbu
Member

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

added this to the Backlog milestone on Oct 4, 2016
Eilon

Eilon commented on Oct 4, 2016

@Eilon
Member

@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

rynowak commented on Jul 17, 2017

@rynowak
Member
JamesNK

JamesNK commented on Jul 17, 2017

@JamesNK
Member

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

rynowak

rynowak commented on Jul 17, 2017

@rynowak
Member

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.

changed the title JsonInputFormatter: errors in model state do not have an ErrorMessage. Report good error messages when JSON fails to deserialize on Sep 7, 2017

21 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions

    Report good error messages when JSON fails to deserialize · Issue #4862 · aspnet/Mvc