C# error exposed in error message

Description

We use this library in a production environment which recently had a pen test against it. One of the observations was that when an array was passed to a field resolver with a query parameter of type int, it exposed the following code-level error message with code 'INVALID_VALUE'.

Variable '$id' is invalid. Unable to convert 'System.Collections.Generic.List`1[System.Object]'

This information could then be used to determine the underlying codebase which could then be used for targeted attacks.

Steps to reproduce

Have a field resolver where an input value is of type 'Int'
Execute a query against the field resolver using a tool like Insomnia, Postman or Curl where instead of an int an array is passed

{
  "operationName":"GetPlan",
  "variables":{"id": [1,2]},
  "query":"query GetPlan($id: Int!) { planById(id: $id) {id}}"
}

Expected result

Any errors that could potentially expose C# level errors should be omitted, either always or via a config variable that can be set for production environments.

Actual result

The following environment variable was exposed

Variable '$id' is invalid. Unable to convert 'System.Collections.Generic.List`1[System.Object]'

Environment

.NET - 8
GraphQL.NET - 7.7.2
MacOS or Docker container

Check the Processing Errors documentation.

You can also handle these processing exceptions by setting a delegate within the ExecutionOptions.UnhandledExceptionDelegate property. Within the delegate you can log the error message and stack trace for debugging needs. You can also override the generic error message with a more specific message, wrap or replace the exception with your own ExecutionError class, and/or set the codes and data as necessary. Note that if ThrowOnUnhandledException is true, the UnhandledExceptionDelegate will not be called.

There are also a few examples of how to perform the configuration.

The UnhandledExceptionDelegate doesn't seem to be called in this situation with ThrowOnUnhandledException turned off. My worry is there are other areas that aren't sanitised that could easily be missed.

It looks like this error in particular is occurring at

graphql-dotnet/src/GraphQL/Validation/ValidationContext.cs

Line 311 in 49f24b5

    
           throw new InvalidVariableError(this, variableDef, variableName, $"Unable to convert '{value}' to '{scalarGraphType.Name}'", ex);

This is actually an input error, due to the fact that IdGraphType cannot parse a list. See here:

graphql-dotnet/src/GraphQL/Types/Scalars/IdGraphType.cs

Lines 47 to 62 in 49f24b5

    
           public override object? ParseValue(object? value) => value switch 
        
           { 
        
               string _ => value, 
        
               int _ => value, 
        
               long _ => value, 
        
               Guid _ => value, 
        
               null => null, 
        
               byte _ => value, 
        
               sbyte _ => value, 
        
               short _ => value, 
        
               ushort _ => value, 
        
               uint _ => value, 
        
               ulong _ => value, 
        
               BigInteger _ => value, 
        
               _ => ThrowValueConversionError(value) 
        
           };

So ParseValue runs ThrowValueConversionError, which throws the exception you showed above.

The exception message should be updated so that ToString isn't called for unknown types such as arrays and objects.

@BottlecapDave I looked into this further; you are correct, although ParseValue throws an error, ValidationContext.cs:311 rethrows a generic error which should not contain C#-specific information.

	public override object? ParseValue(object? value) => value switch
	{
	string _ => value,
	int _ => value,
	long _ => value,
	Guid _ => value,
	null => null,
	byte _ => value,
	sbyte _ => value,
	short _ => value,
	ushort _ => value,
	uint _ => value,
	ulong _ => value,
	BigInteger _ => value,
	_ => ThrowValueConversionError(value)
	};