Default error formatter now returns "Internal server error" unless error is client-aware and safe to report directly to end-users

Author: vladar

UPGRADE.md

@@ -1,15 +1,43 @@
 # Upgrade
 
 ## Upgrade v0.9.x > v0.10.x
-### Deprecated: GraphQL\Utils moved to GraphQL\Utils\Utils
-Old class still exists, but triggers deprecation warning.
+#### Breaking: default error formatting
+By default exceptions thrown in resolvers will be reported with generic message `"Internal server error"`.
+Only exceptions implementing interface `GraphQL\Error\ClientAware` and claiming themselves as `safe` will 
+be reported with full error message.
+
+This is done to avoid information leak in production when unhandled exceptions occur in resolvers 
+(e.g. database connection errors, file access errors, etc).
+
+During development or debugging use `$executionResult->toArray(true)`. It will add `debugMessage` key to 
+each error entry in result. If you also want to add `trace` for each error - pass flags instead:
+
+```
+use GraphQL\Error\FormattedError;
+$debug = FormattedError::INCLUDE_DEBUG_MESSAGE | FormattedError::INCLUDE_TRACE;
+$result = GraphQL::executeAndReturnResult()->toArray($debug);
+```
+
+To change default `"Internal server error"` message to something else, use: 
+```
+GraphQL\Error\FormattedError::setInternalErrorMessage("Unexpected error");
+```
+
+**This change only affects default error reporting mechanism. If you set your own error formatter using 
+`ExecutionResult::setErrorFormatter()` you won't be affected by this change.**
+
+If you are not happy with this change just set [your own error 
+formatter](http://webonyx.github.io/graphql-php/error-handling/#custom-error-formatting).
+
+#### Deprecated: GraphQL\Utils moved to GraphQL\Utils\Utils
+Old class still exists, but triggers deprecation warning when referenced.
 
 ## Upgrade v0.7.x > v0.8.x
 All of those changes apply to those who extends various parts of this library.
 If you only use the library and don't try to extend it - everything should work without breaks.
 
 
-### Breaking: Custom directives handling
+#### Breaking: Custom directives handling
 When passing custom directives to schema, default directives (like `@skip` and `@include`) 
 are not added to schema automatically anymore. If you need them - add them explicitly with 
 your other directives.
@@ -30,15 +58,15 @@ $schema = new Schema([
 ]);
 ```
 
-### Breaking: Schema protected property and methods visibility 
+#### Breaking: Schema protected property and methods visibility 
 Most of the `protected` properties and methods of `GraphQL\Schema` were changed to `private`.
 Please use public interface instead.
 
-### Breaking: Node kind constants
+#### Breaking: Node kind constants
 Node kind constants were extracted from `GraphQL\Language\AST\Node` to 
 separate class `GraphQL\Language\AST\NodeKind`
 
-### Non-breaking: AST node classes renamed
+#### Non-breaking: AST node classes renamed
 AST node classes were renamed to disambiguate with types. e.g.:
 
 ```
@@ -50,7 +78,7 @@ etc.
 Old names are still available via `class_alias` defined in `src/deprecated.php`. 
 This file is included automatically when using composer autoloading.
 
-### Deprecations
+#### Deprecations
 There are several deprecations which still work, but trigger `E_USER_DEPRECATED` when used.
 
 For example `GraphQL\Executor\Executor::setDefaultResolveFn()` is renamed to `setDefaultResolver()`
@@ -61,7 +89,7 @@ but still works with old name.
 There are a few new breaking changes in v0.7.0 that were added to the graphql-js reference implementation 
 with the spec of April2016
 
-### 1. Context for resolver
+#### 1. Context for resolver
 
 You can now pass a custom context to the `GraphQL::execute` function that is available in all resolvers as 3rd argument. 
 This can for example be used to pass the current user etc.
@@ -119,7 +147,7 @@ function resolveMyField($object, array $args, $context, ResolveInfo $info){
 }
 ```
 
-### 2. Schema constructor signature
+#### 2. Schema constructor signature
 
 The signature of the Schema constructor now accepts an associative config array instead of positional arguments:
 
@@ -137,7 +165,7 @@ $schema = new Schema([
 ]);
 ```
 
-### 3. Types can be directly passed to schema
+#### 3. Types can be directly passed to schema
 
 There are edge cases when GraphQL cannot infer some types from your schema. 
 One example is when you define a field of interface type and object types implementing 

docs/error-handling.md

@@ -113,6 +113,28 @@ exceptions thrown by resolvers. To access original exceptions use `$error->getPr
 But note that previous exception is only available for **Execution** errors and will be `null`
 for **Syntax** or **Validation** errors.
 
+For example:
+
+```php
+$result = GraphQL::executeAndReturnResult()
+    ->setErrorFormatter(function(GraphQL\Error\Error $err) {
+        $resolverException = $err->getPrevious();
+
+        if ($resolverException instanceof MyResolverException) {
+            $formattedError = [
+                'message' => $resolverException->getMessage(),
+                'code' => $resolverException->getCode()
+            ];
+        } else {
+            $formattedError = [
+                'message' => $err->getMessage()
+            ];
+        }
+        return $formattedError;
+    })
+    ->toArray();
+```
+
 # Schema Errors
 So far we only covered errors which occur during query execution process. But schema definition can 
 also throw if there is an error in one of type definitions.

src/Error/ClientAware.php

@@ -0,0 +1,17 @@
+<?php
+namespace GraphQL\Error;
+
+/**
+ * Interface ClientAware
+ *
+ * @package GraphQL\Error
+ */
+interface ClientAware
+{
+    /**
+     * Returns true when exception message is safe to be displayed to client
+     *
+     * @return bool
+     */
+    public function isClientSafe();
+}

src/Error/Error.php

@@ -14,7 +14,7 @@
  *
  * @package GraphQL
  */
-class Error extends \Exception implements \JsonSerializable
+class Error extends \Exception implements \JsonSerializable, ClientAware
 {
     /**
      * A message describing the Error for debugging purposes.
@@ -62,6 +62,11 @@ class Error extends \Exception implements \JsonSerializable
      */
     private $positions;
 
+    /**
+     * @var bool
+     */
+    private $isClientSafe;
+
     /**
      * Given an arbitrary Error, presumably thrown while attempting to execute a
      * GraphQL operation, produce a new GraphQLError aware of the location in the
@@ -133,6 +138,22 @@ public function __construct($message, $nodes = null, Source $source = null, $pos
         $this->source = $source;
         $this->positions = $positions;
         $this->path = $path;
+
+        if ($previous instanceof ClientAware) {
+            $this->isClientSafe = $previous->isClientSafe();
+        } else if ($previous === null) {
+            $this->isClientSafe = true;
+        } else {
+            $this->isClientSafe = false;
+        }
+    }
+
+    /**
+     * @return bool
+     */
+    public function isClientSafe()
+    {
+        return $this->isClientSafe;
     }
 
     /**
@@ -190,6 +211,7 @@ public function getLocations()
     /**
      * Returns array representation of error suitable for serialization
      *
+     * @deprecated Use FormattedError::createFromException() instead
      * @return array
      */
     public function toSerializableArray()

src/Error/FormattedError.php

@@ -13,39 +13,84 @@
  */
 class FormattedError
 {
+    const INCLUDE_DEBUG_MESSAGE = 1;
+    const INCLUDE_TRACE = 2;
+
+    private static $internalErrorMessage = 'Internal server error';
+
+    public static function setInternalErrorMessage($msg)
+    {
+        self::$internalErrorMessage = $msg;
+    }
+
     /**
      * @param \Throwable $e
-     * @param $debug
+     * @param bool|int $debug
+     * @param string $internalErrorMessage
      *
      * @return array
      */
-    public static function createFromException($e, $debug = false)
+    public static function createFromException($e, $debug = false, $internalErrorMessage = null)
     {
-        if ($e instanceof Error) {
-            $result = $e->toSerializableArray();
-        } else if ($e instanceof \ErrorException) {
+        Utils::invariant(
+            $e instanceof \Exception || $e instanceof \Throwable,
+            "Expected exception, got %s",
+            Utils::getVariableType($e)
+        );
+
+        $debug = (int) $debug;
+        $internalErrorMessage = $internalErrorMessage ?: self::$internalErrorMessage;
+
+        if ($e instanceof ClientAware) {
+            if ($e->isClientSafe()) {
+                $result = [
+                    'message' => $e->getMessage()
+                ];
+            } else {
+                $result = [
+                    'message' => $internalErrorMessage,
+                    'isInternalError' => true
+                ];
+            }
+        } else {
             $result = [
-                'message' => $e->getMessage(),
+                'message' => $internalErrorMessage,
+                'isInternalError' => true
             ];
+        }
+        if (($debug & self::INCLUDE_DEBUG_MESSAGE > 0) && $result['message'] === $internalErrorMessage) {
+            $result['debugMessage'] = $e->getMessage();
+        }
+
+        if ($e instanceof Error) {
+            $locations = Utils::map($e->getLocations(), function(SourceLocation $loc) {
+                return $loc->toSerializableArray();
+            });
+
+            if (!empty($locations)) {
+                $result['locations'] = $locations;
+            }
+            if (!empty($e->path)) {
+                $result['path'] = $e->path;
+            }
+        } else if ($e instanceof \ErrorException) {
             if ($debug) {
                 $result += [
                     'file' => $e->getFile(),
                     'line' => $e->getLine(),
                     'severity' => $e->getSeverity()
                 ];
             }
-        } else {
-            Utils::invariant(
-                $e instanceof \Exception || $e instanceof \Throwable,
-                "Expected exception, got %s",
-                Utils::getVariableType($e)
-            );
-            $result = [
-                'message' => $e->getMessage()
-            ];
+        } else if ($e instanceof \Error) {
+            if ($debug) {
+                $result += [
+                    'file' => $e->getFile(),
+                    'line' => $e->getLine(),
+                ];
+            }
         }
 
-        if ($debug) {
+        if ($debug & self::INCLUDE_TRACE > 0) {
             $debugging = $e->getPrevious() ?: $e;
             $result['trace'] = static::toSafeTrace($debugging->getTrace());
         }

src/Error/UserError.php

@@ -4,10 +4,17 @@
 /**
  * Class UserError
  *
- * Error that can be safely displayed to client...
+ * Error caused by actions of GraphQL clients. Can be safely displayed to client...
  *
  * @package GraphQL\Error
  */
-class UserError extends InvariantViolation
+class UserError extends \RuntimeException implements ClientAware
 {
+    /**
+     * @return bool
+     */
+    public function isClientSafe()
+    {
+        return true;
+    }
 }

src/Executor/ExecutionResult.php

@@ -2,6 +2,7 @@
 namespace GraphQL\Executor;
 
 use GraphQL\Error\Error;
+use GraphQL\Error\FormattedError;
 
 class ExecutionResult implements \JsonSerializable
 {
@@ -23,7 +24,7 @@ class ExecutionResult implements \JsonSerializable
     /**
      * @var callable
      */
-    private $errorFormatter = ['GraphQL\Error\Error', 'formatError'];
+    private $errorFormatter;
 
     /**
      * @param array $data
@@ -48,14 +49,26 @@ public function setErrorFormatter(callable $errorFormatter)
     }
 
     /**
+     * @param bool|int $debug
      * @return array
      */
-    public function toArray()
+    public function toArray($debug = false)
     {
         $result = [];
 
         if (!empty($this->errors)) {
-            $result['errors'] = array_map($this->errorFormatter, $this->errors);
+            if ($debug) {
+                $errorFormatter = function($e) use ($debug) {
+                    return FormattedError::createFromException($e, $debug);
+                };
+            } else if (!$this->errorFormatter) {
+                $errorFormatter = function($e) {
+                    return FormattedError::createFromException($e, false);
+                };
+            } else {
+                $errorFormatter = $this->errorFormatter;
+            }
+            $result['errors'] = array_map($errorFormatter, $this->errors);
         }
 
         if (null !== $this->data) {

tests/Executor/AbstractPromiseTest.php

@@ -2,6 +2,7 @@
 namespace GraphQL\Tests\Executor;
 
 use GraphQL\Deferred;
+use GraphQL\Error\UserError;
 use GraphQL\Error\Warning;
 use GraphQL\GraphQL;
 use GraphQL\Schema;
@@ -120,7 +121,7 @@ public function testIsTypeOfCanBeRejected()
             'interfaces' => [$PetType],
             'isTypeOf' => function () {
                 return new Deferred(function () {
-                    throw new \Exception('We are testing this error');
+                    throw new UserError('We are testing this error');
                 });
             },
             'fields' => [
@@ -578,7 +579,7 @@ public function testResolveTypeCanBeCaught()
             'name' => 'Pet',
             'resolveType' => function () {
                 return new Deferred(function () {
-                    throw new \Exception('We are testing this error');
+                    throw new UserError('We are testing this error');
                 });
             },
             'fields' => [