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
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 commentedon Jun 14, 2016
@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 commentedon Jun 14, 2016
@DaveVdE to add to what @javiercn said: MVC special-cases a few
Exception
s e.g.FormatException
and uses specific error messages when adding them to theModelStateDictionary
. But sendingException.Message
to clients exposes too much information about the site's infrastructure and is not something MVC does.DaveVdE commentedon Jun 14, 2016
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:
which gives me:
because if I just use a
BadRequestObjectResult
and pass the ModelState, what comes out is not very useful:rynowak commentedon Jun 14, 2016
@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 commentedon Jun 14, 2016
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 commentedon Jun 14, 2016
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 commentedon Jun 14, 2016
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 commentedon Jun 15, 2016
So should I close this issue?
dougbu commentedon Jun 15, 2016
@DaveVdE don't close the issue. I am pretty sure we're not tracking the feature @blowdart and @rynowak discuss above anywhere else.
Eilon commentedon Oct 4, 2016
@JamesNK any thoughts on this? We're thinking that perhaps Json.NET could have some new interface, such as
ISafeExceptionMessage
that has astring 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 commentedon Jul 17, 2017
/cc @JamesNK
JamesNK commentedon Jul 17, 2017
There are a lot of throw statements in Json.NET and doubling the number of error messages sounds like a lot of work.
rynowak commentedon Jul 17, 2017
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.
21 remaining items