From 503ac4619ab87ed634c841ec5b71b226c9d3a5cf Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Fri, 31 Aug 2018 17:45:41 +0200 Subject: [PATCH 1/5] Check if an exception is internal before rethrowing --- src/Error/FormattedError.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Error/FormattedError.php b/src/Error/FormattedError.php index 25d364d..705d822 100644 --- a/src/Error/FormattedError.php +++ b/src/Error/FormattedError.php @@ -235,7 +235,9 @@ class FormattedError $debug = (int) $debug; - if ($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) { + $isInternal = ! $e instanceof ClientAware || ! $e->isClientSafe(); + + if (($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) && $isInternal) { if (! $e instanceof Error) { throw $e; } @@ -245,8 +247,6 @@ class FormattedError } } - $isInternal = ! $e instanceof ClientAware || ! $e->isClientSafe(); - if (($debug & Debug::INCLUDE_DEBUG_MESSAGE) && $isInternal) { // Displaying debugMessage as a first entry: $formattedError = ['debugMessage' => $e->getMessage()] + $formattedError; From 326e0b47192f848363274b007b426a2f393e3b27 Mon Sep 17 00:00:00 2001 From: spawnia Date: Mon, 3 Sep 2018 21:18:04 +0200 Subject: [PATCH 2/5] Add Debug::RETHROW_UNSAFE_EXCEPTIONS flag This allows for a more granular default for which Exceptions are thrown, taking the settings made through the ClientAware interface into account. - Add docs - Add a test case - Differentiate clearly from other test --- docs/error-handling.md | 7 +++-- src/Error/Debug.php | 1 + src/Error/FormattedError.php | 14 +++++++--- tests/Server/QueryExecutionTest.php | 30 ++++++++++++++-------- tests/Server/ServerTestCase.php | 40 ++++++++++++++++++++++++++--- 5 files changed, 73 insertions(+), 19 deletions(-) diff --git a/docs/error-handling.md b/docs/error-handling.md index 8582949..17b0a2a 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -84,7 +84,7 @@ To change default **"Internal server error"** message to something else, use: GraphQL\Error\FormattedError::setInternalErrorMessage("Unexpected error"); ``` -#Debugging tools +# Debugging tools During development or debugging use `$result->toArray(true)` to add **debugMessage** key to each formatted error entry. If you also want to add exception trace - pass flags instead: @@ -116,7 +116,7 @@ This will make each error entry to look like this: ]; ``` -If you prefer first resolver exception to be re-thrown, use following flags: +If you prefer the first resolver exception to be re-thrown, use following flags: ```php toArray($debug); ``` +If you only want to re-throw Exceptions that are not marked as safe through the `ClientAware` interface, use +the flag `Debug::RETHROW_UNSAFE_EXCEPTIONS`. + # Custom Error Handling and Formatting It is possible to define custom **formatter** and **handler** for result errors. diff --git a/src/Error/Debug.php b/src/Error/Debug.php index 6e98087..a48b81f 100644 --- a/src/Error/Debug.php +++ b/src/Error/Debug.php @@ -12,4 +12,5 @@ class Debug const INCLUDE_DEBUG_MESSAGE = 1; const INCLUDE_TRACE = 2; const RETHROW_INTERNAL_EXCEPTIONS = 4; + const RETHROW_UNSAFE_EXCEPTIONS = 8; } diff --git a/src/Error/FormattedError.php b/src/Error/FormattedError.php index 705d822..fcf0914 100644 --- a/src/Error/FormattedError.php +++ b/src/Error/FormattedError.php @@ -235,9 +235,7 @@ class FormattedError $debug = (int) $debug; - $isInternal = ! $e instanceof ClientAware || ! $e->isClientSafe(); - - if (($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) && $isInternal) { + if ($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) { if (! $e instanceof Error) { throw $e; } @@ -247,7 +245,15 @@ class FormattedError } } - if (($debug & Debug::INCLUDE_DEBUG_MESSAGE) && $isInternal) { + $isUnsafe = ! $e instanceof ClientAware || ! $e->isClientSafe(); + + if(($debug & Debug::RETHROW_UNSAFE_EXCEPTIONS) && $isUnsafe){ + if ($e->getPrevious()) { + throw $e->getPrevious(); + } + } + + if (($debug & Debug::INCLUDE_DEBUG_MESSAGE) && $isUnsafe) { // Displaying debugMessage as a first entry: $formattedError = ['debugMessage' => $e->getMessage()] + $formattedError; } diff --git a/tests/Server/QueryExecutionTest.php b/tests/Server/QueryExecutionTest.php index 0b7d473..f443d9e 100644 --- a/tests/Server/QueryExecutionTest.php +++ b/tests/Server/QueryExecutionTest.php @@ -1,11 +1,9 @@ [ - 'fieldWithException' => null, + 'fieldWithSafeException' => null, 'f1' => 'f1' ], 'errors' => [ [ 'message' => 'This is the exception we want', - 'path' => ['fieldWithException'], + 'path' => ['fieldWithSafeException'], 'trace' => [] ] ] @@ -86,6 +84,18 @@ class QueryExecutionTest extends ServerTestCase $result = $this->executeQuery($query)->toArray(); $this->assertArraySubset($expected, $result); } + + public function testRethrowUnsafeExceptions() : void + { + $this->config->setDebug(Debug::RETHROW_UNSAFE_EXCEPTIONS); + $this->expectException(UnsafeException::class); + + $this->executeQuery(' + { + fieldWithUnsafeException + } + ')->toArray(); + } public function testPassesRootValueAndContext() : void { @@ -240,7 +250,7 @@ class QueryExecutionTest extends ServerTestCase 'query' => '{invalid}' ], [ - 'query' => '{f1,fieldWithException}' + 'query' => '{f1,fieldWithSafeException}' ] ]; @@ -405,7 +415,7 @@ class QueryExecutionTest extends ServerTestCase 'query' => '{invalid}' ], [ - 'query' => '{f1,fieldWithException}' + 'query' => '{f1,fieldWithSafeException}' ], [ 'query' => ' @@ -427,7 +437,7 @@ class QueryExecutionTest extends ServerTestCase [ 'data' => [ 'f1' => 'f1', - 'fieldWithException' => null + 'fieldWithSafeException' => null ], 'errors' => [ ['message' => 'This is the exception we want'] @@ -581,7 +591,7 @@ class QueryExecutionTest extends ServerTestCase return ['test' => 'formatted']; }); - $result = $this->executeQuery('{fieldWithException}'); + $result = $this->executeQuery('{fieldWithSafeException}'); $this->assertFalse($called); $formatted = $result->toArray(); $expected = [ @@ -620,7 +630,7 @@ class QueryExecutionTest extends ServerTestCase ]; }); - $result = $this->executeQuery('{fieldWithException,test: fieldWithException}'); + $result = $this->executeQuery('{fieldWithSafeException,test: fieldWithSafeException}'); $this->assertFalse($called); $formatted = $result->toArray(); diff --git a/tests/Server/ServerTestCase.php b/tests/Server/ServerTestCase.php index e76d8f4..5620337 100644 --- a/tests/Server/ServerTestCase.php +++ b/tests/Server/ServerTestCase.php @@ -3,6 +3,7 @@ namespace GraphQL\Tests\Server; use GraphQL\Deferred; +use GraphQL\Error\ClientAware; use GraphQL\Error\UserError; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; @@ -34,10 +35,16 @@ abstract class ServerTestCase extends TestCase return $info->fieldName; } ], - 'fieldWithException' => [ + 'fieldWithSafeException' => [ 'type' => Type::string(), - 'resolve' => function($root, $args, $context, $info) { - throw new UserError("This is the exception we want"); + 'resolve' => function() { + throw new UserError('This is the exception we want'); + } + ], + 'fieldWithUnsafeException' => [ + 'type' => Type::string(), + 'resolve' => function() { + throw new UnsafeException('This exception should not be shown to the user'); } ], 'testContextAndRootValue' => [ @@ -92,3 +99,30 @@ abstract class ServerTestCase extends TestCase return $schema; } } + +class UnsafeException extends \Exception implements ClientAware +{ + /** + * Returns true when exception message is safe to be displayed to a client. + * + * @api + * @return bool + */ + public function isClientSafe() + { + return false; + } + + /** + * Returns string describing a category of the error. + * + * Value "graphql" is reserved for errors produced by query parsing or validation, do not use it. + * + * @api + * @return string + */ + public function getCategory() + { + return 'unsafe'; + } +} From c4fe304efe403e1ec69bd58ba5e7d916a977b42f Mon Sep 17 00:00:00 2001 From: spawnia Date: Mon, 3 Sep 2018 21:43:02 +0200 Subject: [PATCH 3/5] Fix coding style --- src/Error/FormattedError.php | 2 +- tests/Server/QueryExecutionTest.php | 14 ++++++------ tests/Server/ServerTestCase.php | 35 ++++------------------------- tests/Server/Unsafe.php | 30 +++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 39 deletions(-) create mode 100644 tests/Server/Unsafe.php diff --git a/src/Error/FormattedError.php b/src/Error/FormattedError.php index fcf0914..8ca886a 100644 --- a/src/Error/FormattedError.php +++ b/src/Error/FormattedError.php @@ -247,7 +247,7 @@ class FormattedError $isUnsafe = ! $e instanceof ClientAware || ! $e->isClientSafe(); - if(($debug & Debug::RETHROW_UNSAFE_EXCEPTIONS) && $isUnsafe){ + if (($debug & Debug::RETHROW_UNSAFE_EXCEPTIONS) && $isUnsafe) { if ($e->getPrevious()) { throw $e->getPrevious(); } diff --git a/tests/Server/QueryExecutionTest.php b/tests/Server/QueryExecutionTest.php index 334d5b9..5a76f46 100644 --- a/tests/Server/QueryExecutionTest.php +++ b/tests/Server/QueryExecutionTest.php @@ -19,6 +19,7 @@ use GraphQL\Validator\Rules\CustomValidationRule; use GraphQL\Validator\ValidationContext; use function count; use function sprintf; +use Unsafe; class QueryExecutionTest extends ServerTestCase { @@ -94,21 +95,20 @@ class QueryExecutionTest extends ServerTestCase 'errors' => [ [ 'message' => 'This is the exception we want', - 'path' => ['fieldWithSafeException'], - 'trace' => [] - ] - ] + 'trace' => [], + ], + ], ]; $result = $this->executeQuery($query)->toArray(); $this->assertArraySubset($expected, $result); } - + public function testRethrowUnsafeExceptions() : void { $this->config->setDebug(Debug::RETHROW_UNSAFE_EXCEPTIONS); - $this->expectException(UnsafeException::class); + $this->expectException(Unsafe::class); $this->executeQuery(' { @@ -487,7 +487,7 @@ class QueryExecutionTest extends ServerTestCase [ 'data' => [ 'f1' => 'f1', - 'fieldWithSafeException' => null + 'fieldWithSafeException' => null, ], 'errors' => [ ['message' => 'This is the exception we want'], diff --git a/tests/Server/ServerTestCase.php b/tests/Server/ServerTestCase.php index 7c87258..90f6849 100644 --- a/tests/Server/ServerTestCase.php +++ b/tests/Server/ServerTestCase.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace GraphQL\Tests\Server; use GraphQL\Deferred; -use GraphQL\Error\ClientAware; use GraphQL\Error\UserError; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\Type; @@ -15,6 +14,7 @@ use function trigger_error; use const E_USER_DEPRECATED; use const E_USER_NOTICE; use const E_USER_WARNING; +use Unsafe; abstract class ServerTestCase extends TestCase { @@ -46,13 +46,13 @@ abstract class ServerTestCase extends TestCase 'type' => Type::string(), 'resolve' => function() { throw new UserError('This is the exception we want'); - } + }, ], 'fieldWithUnsafeException' => [ 'type' => Type::string(), 'resolve' => function() { - throw new UnsafeException('This exception should not be shown to the user'); - } + throw new Unsafe('This exception should not be shown to the user'); + }, ], 'testContextAndRootValue' => [ 'type' => Type::string(), @@ -108,30 +108,3 @@ abstract class ServerTestCase extends TestCase return $schema; } } - -class UnsafeException extends \Exception implements ClientAware -{ - /** - * Returns true when exception message is safe to be displayed to a client. - * - * @api - * @return bool - */ - public function isClientSafe() - { - return false; - } - - /** - * Returns string describing a category of the error. - * - * Value "graphql" is reserved for errors produced by query parsing or validation, do not use it. - * - * @api - * @return string - */ - public function getCategory() - { - return 'unsafe'; - } -} diff --git a/tests/Server/Unsafe.php b/tests/Server/Unsafe.php new file mode 100644 index 0000000..d33a908 --- /dev/null +++ b/tests/Server/Unsafe.php @@ -0,0 +1,30 @@ + Date: Mon, 3 Sep 2018 22:12:03 +0200 Subject: [PATCH 4/5] Add namespace to class --- tests/Server/QueryExecutionTest.php | 1 - tests/Server/ServerTestCase.php | 1 - tests/Server/Unsafe.php | 2 ++ 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Server/QueryExecutionTest.php b/tests/Server/QueryExecutionTest.php index 5a76f46..d816bce 100644 --- a/tests/Server/QueryExecutionTest.php +++ b/tests/Server/QueryExecutionTest.php @@ -19,7 +19,6 @@ use GraphQL\Validator\Rules\CustomValidationRule; use GraphQL\Validator\ValidationContext; use function count; use function sprintf; -use Unsafe; class QueryExecutionTest extends ServerTestCase { diff --git a/tests/Server/ServerTestCase.php b/tests/Server/ServerTestCase.php index 90f6849..431d28d 100644 --- a/tests/Server/ServerTestCase.php +++ b/tests/Server/ServerTestCase.php @@ -14,7 +14,6 @@ use function trigger_error; use const E_USER_DEPRECATED; use const E_USER_NOTICE; use const E_USER_WARNING; -use Unsafe; abstract class ServerTestCase extends TestCase { diff --git a/tests/Server/Unsafe.php b/tests/Server/Unsafe.php index d33a908..5e08918 100644 --- a/tests/Server/Unsafe.php +++ b/tests/Server/Unsafe.php @@ -1,5 +1,7 @@ Date: Mon, 3 Sep 2018 22:46:14 +0200 Subject: [PATCH 5/5] More coding style -.- Can this be done automatically? --- tests/Server/ServerTestCase.php | 4 ++-- tests/Server/Unsafe.php | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/Server/ServerTestCase.php b/tests/Server/ServerTestCase.php index 431d28d..4ad9698 100644 --- a/tests/Server/ServerTestCase.php +++ b/tests/Server/ServerTestCase.php @@ -43,13 +43,13 @@ abstract class ServerTestCase extends TestCase ], 'fieldWithSafeException' => [ 'type' => Type::string(), - 'resolve' => function() { + 'resolve' => function () { throw new UserError('This is the exception we want'); }, ], 'fieldWithUnsafeException' => [ 'type' => Type::string(), - 'resolve' => function() { + 'resolve' => function () { throw new Unsafe('This exception should not be shown to the user'); }, ], diff --git a/tests/Server/Unsafe.php b/tests/Server/Unsafe.php index 5e08918..2eb3742 100644 --- a/tests/Server/Unsafe.php +++ b/tests/Server/Unsafe.php @@ -1,5 +1,7 @@