apollo-engine-reporting: tweaks to maskErrorDetails and rewriteError

[glasser]

This reimplements engine.maskErrorDetails in terms of engine.rewriteError, simplifying the code path around it. It also allows rewriteError implementors slightly more control over the error they produce.

Two behavior changes:

• If the error returned from the engine.rewriteError hook has an extensions property, that property will be used instead of the original error's extensions. We now document that changes to most other GraphQLError fields by engine.rewriteError are ignored.
• The engine.maskErrorDetails option, deprecated by engine.rewriteError in v2.5.0, now behaves a bit more like the new option: while all error messages will be redacted, they will still show up on the appropriate nodes in a trace.

[glasser]

While I think I'd like to see this merged (and am planning to build my federated metrics change on top of this, because it refactors where this code is called from) I am also raising this PR basically to try to help me understand some of the choices made in #2618

I don't understand this code in rewriteError:

      return new GraphQLError(
        rewrittenError.message,
        err.nodes,
        err.source,
        err.positions,
        err.path,
        err.originalError,
        err.extensions,
      );

This raises two questions for me:

(a) Why exactly do we have an API that's allowed to return an arbitrary error, and yet if it's a GraphQLError (the expected case?) it's not allowed to change anything but the message? I'm assuming part of the point here is that it should be OK for a rewriteError to return new GraphQLError('some new message') without having to remember to copy over the path and ensure it ends up in the right place, which seems reasonable (although all the examples encourage mutation instead), and most of the fields here don't seem too sensitive. At first glance originalError seems sensitive but it's a non-enumerable property so it's not actually reported.

But... extensions definitely seems like a property that could have arbitrary sensitive data on it! Not letting rewriteErrors sanitize extensions seems pretty harsh. While the docs and examples for rewriteError are great, I don't see anything that explains that changes to non-message properties will be ignored! Was it intentional that rewriteErrors shouldn't be able to rewrite extensions?

(b) The new feature is documented as being a replacement for maskErrorDetails, but you can't actually implement the behavior of maskErrorDetails in terms of rewriteError. Notably, maskErrorDetails errors don't have a path, location, extensions, etc, whereas rewriteError return values do. Was this an intentional change (we think those fields aren't sensitive)? It seems odd to make a new feature that doesn't provide the value of the old feature — unless we think the behavior of the old feature isn't valuable, in which case changing the old feature's behavior like I do here seems reasonable?

[glasser]

I guess there's a tension between "want people to be be able to keep the context without having to write it" (especially for path!) and "want people to be able to delete stuff".

What if at the very least, extensions was loaded from rewrittenError.extensions || err.extensions, and you could set extensions to {} on your new object if you wanted to (effectively) drop all extensions?

[abernix]

I guess there's a tension between "want people to be be able to keep the context without having to write it" (especially for path!) and "want people to be able to delete stuff".

Yup! Generally, it was believed that users will want to mask sensitive things, or selectively not report an error.

The new feature is documented as being a replacement for maskErrorDetails, but you can't actually implement the behavior of maskErrorDetails

I think the lack of a more correct maskErrorDetails implementation was part of its immediate demise (rewriteError was actually supposed to come out in the version immediately after it surfaced). Further, I believe that maskErrorDetails may have resulted in the errors appearing in the wrong place in the trace tree entirely — i.e. through because of its loss the path? The overall idea of just masking all errors in such a generic way wasn't incredibly useful, overall, hence rewriteError.

As you found with originalError, it doesn't make its way to transmission, which is why we didn't copy that property over from the error returned from rewriteError, but the extensions bit seems like an oversight on my part and I think your suggestion to rewrittenError.extensions || err.extensions and provide that option is correct.

I don't see anything that explains that changes to non-message properties will be ignored!

That is an important detail and should be clarified!

unless we think the behavior of the old feature isn't valuable, in which case changing the old feature's behavior like I do here seems reasonable?

Seems fine to me.

[abernix]

We should!

[glasser]

Will fix with the || solution.

[abernix]

We had been reproducing exactly what the previous generation of maskErrorDetails had done. I would be surprised if anyone is actually using this option in its current state, to be honest. I don't think it was ever documented, and the one person who I know asked for it claimed it missed the mark.

[abernix]

Yeah, the original implementation just didn't have this extra detail, I think?

[glasser]

Further, I believe that maskErrorDetails may have resulted in the errors appearing in the wrong place in the trace tree entirely

OK, my main goal here was verifying that we consider this to be a bug and not a feature. Thanks!

[abernix]

Follow-up commits look good. Free to merge to release-2.7.0!