Allowing ErrorHandler to remove sensitive information

[T0bst3r]

Hey there dear team

I hope this is the right way to be doing this, if not, or if I missed out on important information, please let me know.

While developing a dart gRPC server for a client application I noticed that unhandled exceptions within a RPC cause a GrpcError of type INTERNAL to be returned to the client.

While I am aware, that the right path of action is to assure no unhandled Exceptions be thrown, eventually a bug will find it's way.

Receiving a message like gRPC Error (code: 13, codeName: INTERNAL, message: Exception: SECRET_KEY not defined, details: [], rawResponse: null, trailers: {}) on client side raises two issues:

• This could reveal sensitive information to an attacker
• This doesn't reveal a lot of information when debugging

After looking through the code I've decided that using the existing infrastructure would probably be the fastest and most versatile approach. Therefore I propose expanding the existing errorHandler function definition by an optional return parameter of GrpcError and updating the call of the errorHandler within the _sendError function to override the initial error IF a new one is defined.

This allows for full backwards compatibility (As existing errorHandlers would not have a return value and therefore would not override the existing GrpcError), but also allows for more customisation through the handler.

A simple handler like (error, trace) => error.code == 13 ? grpc.GrpcError.internal('Lorem Ipsum') : error // 13 => internal error would allow to remove all sensitive information of an exception in a production environment and would instead return this GrpcError: gRPC Error (code: 13, codeName: INTERNAL, message: Lorem Ipsum, details: [], rawResponse: null, trailers: {})

While a Handler like: (error, trace) => error.code == 13 ? grpc.GrpcError.internal(error.message, null, null, {'stack trace' : trace.toString()}) : error allows for the stack trace to be added in dev environments and in turn a GrpcError like this one is returned to the client:

gRPC Error (code: 13, codeName: INTERNAL, message: Exception: SECRET_KEY not defined, details: [], rawResponse: null, trailers: {stack trace: 
#0      envGetOrThrow.<anonymous closure> (...)
#1      DotEnv.getOrElse (package:dotenv/src/dotenv.dart:52:43)
#2      ...

(Although for this I had to manipulate files a little bit more, because the stack trace is only added to _sendError by about a third of calls thus far, but this is easily added thanks to the dart 3.0 addition of Records. See: #666 (comment))

In my eyes the only downside of my approach is: The default is only as secure as the current solution and not more secretive.

TL;DR: slightly expanding the current functionalities of the errorHandler allows for greater customisability with increased security or increased debugability at no cost and full backwards compatibility.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: T0bst3r (9c0cfd9)

[T0bst3r]

Hey there, it has been a while, is there anyone that could have a look at this? I don't want to roll the current error feedback to production and overriding the classes locally isn't really an option either. Not a permanent one anyways

[mosuem]

This would be a breaking API change, as GrpcErrorHandler is part of the public API. So this PR would need a major version rev and changelog entries. What is the relation to #666?

[T0bst3r]

Thank you for taking your time and having a look at this.

I was trying to argue my case that it is non breaking because a function implicitly declared will at most give a warning, how ever then I noticed, that a function explicitly declared with a void return type can not be inserted. (I don't get why a function not returning anything doesn't fit the criteria of a function that returns nothing or a specific object, but that's a topic for a different discussion I think)

Relation to #666 is, that I noticed when trying to give more feedback, that the stack trace was missing when an exception was thrown inside an interceptor or during metadata handling. This is because the _onMetadata and _applyInterceptors functions return the Error as a value and don't rethrow it. The records type added in Dart 3 allow for these functions to also return the stack trace which can then be passed on to the error handler. The changes complement each other well, but are technically independent from one another, hence why two pull requests.

[T0bst3r]

Is there a way to define the GrpcErrorHandler as accepting of both void and GrpcError? return types, another way to make the change non breaking, or what would the next steps be there?