Don't return server-oriented, implementation-leaking error messages for client errors

[cmeeren]

Product

Hot Chocolate

Is your feature request related to a problem?

Consider for example this error:

https://github.com/ChilliCream/graphql-platform/blob/7d21adab765fffad291f22255db1102fed39234e/src/HotChocolate/Core/src/Execution/Properties/Resources.resx#L202

Any client can trivially produce this error by e.g. supplying an invalid value for a GUID field, which results in this message:

"Unable to convert the value of the argument id to System.Guid. Check if the requested type is correct or register a custom type converter."

There are two problems with this:

• The message, with its mention of registering custom type converters, is oriented toward the server's developers, not clients
• The message, in printing the fully qualified type name, leaks implementation details. With custom types here, an attacker could get some information about the structure of the codebase, possibly making inferences allowing them to more effectively hack the system.

The solution you'd like

Messages that are oriented toward API clients and which do not leak implementation details.

[cmeeren]

Note that the type name in this specific case is also present in extensions.

[cmeeren]

Another thing: I see in ExecutionDiagnosticEventListener that this error triggers ResolverError. Shouldn't it instead trigger ValidationErrors?

Triggering ResolverError is cumbersome for us because we assume ResolveError indicates a server error in the resolver, and are logging that as errors, but client errors should not be logged as errors since the server is not at fault.

[michaelstaib]

ResolveError is correct as it is happening during execution. The GraphQL spec distinguishes very clearly here. The error is also thrown by the resolver context.

[michaelstaib]

I will have to have a look at error cases a bit on this one. This is kind of a catch clause at the end. We can make the error less telling but at dev time it should be as it is today. On production however the error could be more generic.

[cmeeren]

On production however the error could be more generic.

That would be great!

ResolveError is correct as it is happening during execution. The GraphQL spec distinguishes very clearly here. The error is also thrown by the resolver context.

Sure! 👍 Then I merely wish for a reliable way to distinguish between server and client errors in ResolverError, so that I can log server errors properly while not getting spammed with client errors.