Always throw RequestError with useful message when clients provide an invalid JSON body (#916)

Co-authored-by: Benedikt Franke <[email protected]>

Author: Frits van Campen

CHANGELOG.md

@@ -18,6 +18,7 @@ You can find and compare releases at the [GitHub release page](https://github.co
 - Throw `SerializationError` over client safe `Error` when failing to serialize leaf types
 - Move debug entries in errors under `extensions` key
 - Use native PHP types for `Schema` and `SchemaConfig`
+- Always throw `RequestError` with useful message when clients provide an invalid JSON body
 
 ### Added
 

src/Server/Helper.php

@@ -90,18 +90,9 @@ public function parseHttpRequest(?callable $readRawBodyFn = null)
                 $rawBody    = $readRawBodyFn === null
                     ? $this->readRawBody()
                     : $readRawBodyFn();
-                $bodyParams = json_decode($rawBody, true);
+                $bodyParams = $this->decodeJson($rawBody);
 
-                if (json_last_error() !== JSON_ERROR_NONE) {
-                    throw new RequestError('Could not parse JSON: ' . json_last_error_msg());
-                }
-
-                if (! is_array($bodyParams)) {
-                    throw new RequestError(
-                        'GraphQL Server expects JSON object or array, but got ' .
-                        Utils::printSafeJson($bodyParams)
-                    );
-                }
+                $this->assertJsonObjectOrArray($bodyParams);
             } elseif (stripos($contentType, 'application/x-www-form-urlencoded') !== false) {
                 $bodyParams = $_POST;
             } elseif (stripos($contentType, 'multipart/form-data') !== false) {
@@ -530,22 +521,9 @@ public function parsePsrRequest(RequestInterface $request)
             } elseif (stripos($contentType[0], 'application/json') !== false) {
                 $bodyParams = $request instanceof ServerRequestInterface
                     ? $request->getParsedBody()
-                    : json_decode((string) $request->getBody(), true);
-
-                if ($bodyParams === null) {
-                    throw new InvariantViolation(
-                        $request instanceof ServerRequestInterface
-                         ? 'Expected to receive a parsed body for "application/json" PSR-7 request but got null'
-                         : 'Expected to receive a JSON array in body for "application/json" PSR-7 request'
-                    );
-                }
+                    : $this->decodeJson((string) $request->getBody());
 
-                if (! is_array($bodyParams)) {
-                    throw new RequestError(
-                        'GraphQL Server expects JSON object or array, but got ' .
-                        Utils::printSafeJson($bodyParams)
-                    );
-                }
+                $this->assertJsonObjectOrArray($bodyParams);
             } else {
                 parse_str((string) $request->getBody(), $bodyParams);
 
@@ -564,6 +542,36 @@ public function parsePsrRequest(RequestInterface $request)
         );
     }
 
+    /**
+     * @return mixed
+     *
+     * @throws RequestError
+     */
+    protected function decodeJson(string $rawBody)
+    {
+        $bodyParams = json_decode($rawBody, true);
+
+        if (json_last_error() !== JSON_ERROR_NONE) {
+            throw new RequestError('Expected JSON object or array for "application/json" request, but failed to parse because: ' . json_last_error_msg());
+        }
+
+        return $bodyParams;
+    }
+
+    /**
+     * @param mixed $bodyParams
+     *
+     * @throws RequestError
+     */
+    protected function assertJsonObjectOrArray($bodyParams): void
+    {
+        if (! is_array($bodyParams)) {
+            throw new RequestError(
+                'Expected JSON object or array for "application/json" request, got: ' . Utils::printSafeJson($bodyParams)
+            );
+        }
+    }
+
     /**
      * Converts query execution result to PSR-7 response.
      *

tests/Server/RequestParsingTest.php

@@ -401,42 +401,39 @@ public function testFailsParsingInvalidRawJsonRequestRaw(): void
         $body = 'not really{} a json';
 
         $this->expectException(RequestError::class);
-        $this->expectExceptionMessage('Could not parse JSON: Syntax error');
+        $this->expectExceptionMessage('Expected JSON object or array for "application/json" request, but failed to parse because: Syntax error');
         $this->parseRawRequest('application/json', $body);
     }
 
     public function testFailsParsingInvalidRawJsonRequestPsr(): void
     {
         $body = 'not really{} a json';
 
-        $this->expectException(InvariantViolation::class);
-        $this->expectExceptionMessage('Expected to receive a JSON array in body for "application/json" PSR-7 request');
+        $this->expectException(RequestError::class);
+        $this->expectExceptionMessage('Expected JSON object or array for "application/json" request, but failed to parse because: Syntax error');
         $this->parsePsrRequest('application/json', $body);
     }
 
     public function testFailsParsingNonPreParsedPsrRequest(): void
     {
-        try {
-            $this->parsePsrRequest('application/json', json_encode(null));
-            self::fail('Expected exception not thrown');
-        } catch (InvariantViolation $e) {
-            // Expecting parsing exception to be thrown somewhere else:
-            self::assertSame(
-                'Expected to receive a JSON array in body for "application/json" PSR-7 request',
-                $e->getMessage()
-            );
-        }
+        $this->expectException(RequestError::class);
+        $this->expectExceptionMessage('Expected JSON object or array for "application/json" request, got: null');
+        $this->parsePsrRequest('application/json', json_encode(null));
+    }
+
+    public function testFailsParsingInvalidEmptyJsonRequestPsr(): void
+    {
+        $this->expectException(RequestError::class);
+        $this->expectExceptionMessage('Expected JSON object or array for "application/json" request, but failed to parse because: Syntax error');
+        $this->parsePsrRequest('application/json', '');
     }
 
-    /**
-     * There is no equivalent for psr request, because it should throw
-     */
     public function testFailsParsingNonArrayOrObjectJsonRequestRaw(): void
     {
         $body = '"str"';
 
         $this->expectException(RequestError::class);
-        $this->expectExceptionMessage('GraphQL Server expects JSON object or array, but got "str"');
+        $this->expectExceptionMessage('Expected JSON object or array for "application/json" request, got: "str"');
         $this->parseRawRequest('application/json', $body);
     }
 
@@ -445,7 +442,7 @@ public function testFailsParsingNonArrayOrObjectJsonRequestPsr(): void
         $body = '"str"';
 
         $this->expectException(RequestError::class);
-        $this->expectExceptionMessage('GraphQL Server expects JSON object or array, but got "str"');
+        $this->expectExceptionMessage('Expected JSON object or array for "application/json" request, got: "str"');
         $this->parsePsrRequest('application/json', $body);
     }