Remove over-share in error causing user data leaking into logs

[dustyholmes-wf]

I discovered that the toString of this error produces content based on the json that was being deserialized. Any logging mechanism that uses the toString has the opportunity to then place the contents of that data into the logging system.

This is far too easy to do by accident and I believe that separating the json from the error it produces helps while not reducing the comprehension of the error significantly.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ahmednfwela]

I think it's better to leave the old behavior but hide it under a flag

[dustyholmes-wf]

I think it's better to leave the old behavior but hide it under a flag

I was thinking about this, I restored the json value but did not print it. This allows developers to inspect the error in more detail.

[ahmednfwela]

I like this, the previous behavior of printing the src json was unreadable anyway since most json objects transferred over APIs are long

[dustyholmes-wf]

I signed it! (My company, Workiva, signed it!)

No dice: system's changed.

[dave26199]

Thanks! I am just now on vacation for a week, will take a look when back. Apologies for the delay :)

[trentgrover-wf]

fixed the CLA issue

[ahmednfwela]

@dustyholmes-wf can you mark it as ready for review ?

[davidmorgan]

@dustyholmes-wf

Thanks for the PR.

There seems to be a problem with the commits, one of them partially reverts the other, so not all the changes show up here: https://github.com/google/built_value.dart/pull/1256/files

I like the idea, thank you, a few suggestions:

• Let's keep the signature of DeserializationError compatible by making json an optional named parameter
• Make the error message a bit shorter still, I think the words "json content" can be removed

I'm about to go on vacation for another week--sorry again--but will check in on this and do a built_value release when back. (From 14th August). Thanks!

[dustyholmes-wf]

@davidmorgan I've updated this to remove that extra noise from the error message. The second commit was intended to revert some of the changes. The end result is that the Error object technically contains the actual JSON but does not print it when you toString the error. This should help developers identify what is going on when debugging, but won't make it easy to accidentally write the potentially sensitive content out.

Given all of that, the signature for the error remains the same. It still accepts a json.

[davidmorgan]

@dustyholmes-wf got it, thanks--I got confused about the progression.

Could you please also add to the class comment something like:

/// [Error] conveying why deserialization failed.
///
/// The Object that failed to deseralize is included as [json] but is not displayed
/// in the error message to prevent accidental inclusion in logs.
class DeserializationError extends Error {

And if you like, add yourself to AUTHORS.

And, looks like the test needs updating?

Then I think we're good to merge.

Thanks :) and thanks for your patience.

[dustyholmes-wf]

@davidmorgan Ok, I think I got that all.

[davidmorgan]

Looks good, thanks!