Stop leaking exception information in errors

[piotrblasiak]

It seems like the following extensions are added to errors by default:

"extensions": {
  "exception": "java.lang.NullPointerException",
  "classification": "DataFetchingException",
  "code": "null-pointer"
}

I think this should be considered information leakage and improper error handling. See OWASP: https://owasp.org/www-community/Improper_Error_Handling

I think the @errorcode annotation is useful, because it lets GraphQL clients show actionable errors, but I think it is worth arguing that only very specific exceptions should have error codes. NPE is usually not something the end user can do anything about and should not have a code.

[t1]

There's sometimes a difficult balance between security and usability.

Maybe we should not generate the exception nor the code extensions for exceptions from the java.lang package? Other packages? This should probably be a config option.

Or we could generate a code with a generic value like internal-server-error or so?

[piotrblasiak]

In my opinion, the default config should follow the graphql specification and not include any extensions at all.
I agree that theres a difficult balance between security and usability, but the default should be security - at least in production mode. You could include all those extensions in dev mode if you really want but that would also make people wonder what happened with them in production if they rely on them.

[t1]

I fully second that security goes first. Could you describe in more detail, what you consider a better alternative?

[piotrblasiak]

I think implementing #299 (being able to add extensions yourself), and removing all default error codes and other error extensions would be my preferred alternative. No extensions should be added by the graphql implementation by default :)

[phillip-kruger]

I agree that we should not show the error details in the extensions by default. I suggest we add a config to switch this feature on (like it is currently) and off, and making the default off. w.d.y.t ?

[piotrblasiak]

Yes. I think that would be nice - as there might be some people relying on that functionality.

[phillip-kruger]

Yea, so for those, it will be a breaking change (in the sense that they will have to add a config) but I think that is the correct default. I'll make a change a.s.a.p

[piotrblasiak]

Another thing that I think is potentially dangerous is that the exception message is always exposed as the error message. Consider if the database would be down, and the exception would contain authentication information - that would then be leaked to the client.

[phillip-kruger]

That can be controlled. By default, for RuntimeExceptions, it will show a generic server error message, and for Checked Exception, it will show the exception message. But that is configurable. See https://download.eclipse.org/microprofile/microprofile-graphql-1.0.4-RC1/microprofile-graphql-spec-1.0.4-RC1.html#_server_errors

Note that the black/white list naming has changed and will be available in an upcoming MP version, but is already supported in SmallRye (use mp.graphql.hideErrorMessage and mp.graphql.showErrorMessage) See https://github.com/smallrye/smallrye-graphql/blob/main/server/implementation-cdi/src/main/java/io/smallrye/graphql/cdi/config/GraphQLConfig.java

[piotrblasiak]

Ok, is there some standard convention that I am not aware of that says that checked exceptions should always be safe to show to the end-user? For example, I just got this:

Validation error of type WrongType: argument 'id' with value 'StringValue{value='uZmcZ-obPREWGetAoXxOZ0Q'}' is not a valid 'Unknown Scalar Type [com.myapp.domain.model.GraphId]' @ 'model'

That error message is neither secure nor helpful to the end-user.

But perhaps this is a discussion to be had in the MP group?

[phillip-kruger]

You can hide it if you prefer. Also note that you can use bean validation, that might give you a better message on validation

[piotrblasiak]

@phillip-kruger ok, how would you do that in quarkus?

I tried setting the 2 application.properties that I could think of:

mp.graphql.showErrorMessage=none
quarkus.smallrye-graphql.showErrorMessage=none

And I still got the leaky error message above.

[phillip-kruger]

There is no quarkus property for this (we can maybe add that).
The property will work the same as the on as described in the spec.

So the mp.graphql.showErrorMessage property takes a list of Exceptions (full package and class name) that usually are hidden but you want it shown (So a RuntimeException)

The mp.graphql.hideErrorMessage does the opposite to Checked Exceptions.

[piotrblasiak]

Oh ok. So there is no way to hide all exception messages besides those that I whitelist?
Basically, I don't trust any exceptions to expose their message.

[phillip-kruger]

Well, checked exception is usually a fairly specific Exception that you throw, and are the only ones that show the message. You can try to add java.lang.Exception, I can not remember if this will include all subtypes. @t1 might remember.

[piotrblasiak]

How about errors like this?

The field at path '/model/creationDate' was declared as a non null type, but the code involved in retrieving data has wrongly returned a null value. The graphql specification requires that the parent field be set to null, or if that is non nullable that it bubble up null to its parent and so on. The non-nullable type is 'DateTime' within parent type 'Model'

Not trying to be a pain in the ass, but I hope you see my point - plenty of internal error messages reaching the client and even if these are not dangerous to show there is no way to know if this is user friendly to show to the end-user, or if we should show some generic error message instead.

[phillip-kruger]

That looks like errors from graphql-java ? We can maybe discuss to add those (validation) error to the hide list by default. But that will have to happen on spec (MicroProfile) level

[phillip-kruger]

Also, try adding Bean validation to those methods. That might help...

[phillip-kruger]

Can you share the full response of your example above ? What is the details in the extension ?

[piotrblasiak]

I think me not having bean validation yet in this case helped to show a potential problem - that unexpected error messages are being sent :)

Here is the full response:

{"errors":[{"message":"The field at path '/model/creationDate' was declared as a non null type, but the code involved in retrieving data has wrongly returned a null value.  The graphql specification requires that the parent field be set to null, or if that is non nullable that it bubble up null to its parent and so on. The non-nullable type is 'DateTime' within parent type 'Model'","path":["model","creationDate"],"extensions":{"classification":"NullValueInNonNullableField"}}],"data":{"model":null}}

It is likely that graphql-java is returning all sorts of error messages. I had to do this in my other code to make sure only the messages I trust are being sent:

public class ExceptionWhileDataFetching implements GraphQLError {

    private final List<Object> path;
    private final Throwable exception;
    private final List<SourceLocation> locations;
    private final String reference;

    public ExceptionWhileDataFetching(final ResultPath path, final Throwable exception, final SourceLocation sourceLocation, final String reference) {

        this.path = assertNotNull(path).toList();
        this.exception = assertNotNull(exception);
        this.locations = Collections.singletonList(sourceLocation);
        this.reference = reference;
    }

    @Override
    public String getMessage() {
        return exception instanceof UserFriendlyException ? ((UserFriendlyException) exception).getUserFriendlyMessage() : null;
    }

[phillip-kruger]

I think me not having bean validation yet in this case helped to show a potential problem - that unexpected error messages are being sent :)

^^^ Was trying to help you with a workaround

This Error comes from graphql-java, so we would need a clean way to control these, and that does not exist at the moment.
Some Errors are not tied to Exceptions, so we can not control them with the logic above.

We would need to look into how we can make this extendable .... Thanks :)

Maybe open a separate issue for this. (graphql-java errors) as this issue is for the extension switch as discussed earlier

[piotrblasiak]

Ah yeah thanks - I appreciate the workaround 🙏

Isn't this a proper thread for this issue? I think the issue for extension switching is discussed in #299

[phillip-kruger]

I based it on: #772 (comment)

I think we have multiple issues,

1. the ability to turn of the exception details we add to extensions. (this issue)
2. the ability to contribute (key-value) to the extensions (Add generic ability to add extension key-value for Errors #299)
3. the ability to control graphql-java errors (new issue)

[t1]

is there some standard convention that I am not aware of that says that checked exceptions should always be safe to show to the end-user?

This convention is from the Jakarta EE tradition. Checked exceptions are considered to be application exceptions a.k.a. business exceptions, in contrast to technical exceptions. There is one famous exception to this rule: IOExceptions. But I hardly see checked exceptions being used any more... it's just too troublesome to pass them through several layers and they are generally not usable in functional APIs, e.g. Streams.

You can show/hide messages by configuring a super class, so mp.graphql.hideErrorMessage=java.lang.Exception should hide the message from all checked exceptions.

I think we can completely remove the exception annotation. Clients should to be concerned about this.

Maybe we should just use the same config option pair to show/hide the message and code extensions by default. I.e. the default would be to hide all RuntimeExceptions but to show all checked exceptions (maybe we should treat an IOException like a RuntimeException). If an exception uses an @Code annotation, this extension is added, if the class isn't a subclass of any class in mp.graphql.hideErrorMessage.

Maybe we should rename the config options to mp.graphql.hide/showErrorDetails?

What do you think?

[piotrblasiak]

Yes I think that could make sense.

[phillip-kruger]

I think there is still a valid case to make that even for Checked exceptions, we should not (by default) add the the stuff we do in extensions. The use should opt-in for this. I made a PR that will hide all our extensions by default, with the option to opt-in per field.