Make early errors more consistent and more 4xxy (#5305)

There are a lot of reasons that a supposedly GraphQL request might not make it to requestPipeline: missing body, bad JSON, bad content-type, etc. Previously, many of these outcomes resulted in 5xx errors despite them being client errors (which should be 4xx), and their handling was pretty inconsistent across the integrations. Part of the confusion was a particular error message which combined "you seem to have set up your server wrong by not setting up body parsing properly" with "there was a problem parsing this request". The former is reasonable to be 500 but the latter is much more likely to be what's actually happening (you have to actively disable body parsing to make that happen!). And the recommendation was ungrammatical and express-specific despite being in apollo-server-core. But the good news is that for some reason the Express body-parser unconditionally sets `req.body` to at least `{}` before checking `content-type` (see https://github.com/expressjs/body-parser/blob/480b1cfe29af19c070f4ae96e0d598c099f42a12/lib/types/json.js#L105-L117) so we can move the "500 if body not parsed" from `apollo-server-core` to `apollo-server-express` and directly detect whether `req.body` is empty. This is a backwards-incompatible change, so we can finally implement it as v3 is imminent. The PR consists of: - Core - Make an error about needing a query be clear that it needs to be a non-empty query (the error could fire before if `query` was provided yet empty). - Make "POST body missing" error 400 instead of 500, include that it could be bad content-type or empty JSON object, and don't suggest (with poor grammar) the use of body-parser (as described above). Also show this error if httpRequest.query is a string or Buffer (this can happen with Azure Functions on JSON parse failures). - Express: Check for a sign that body-parser hasn't been applied here instead of in Core and provide a more targeted body-parser-suggesting 500. - Micro: Only parse JSON for `content-type: application/json` (like all other integrations). - Azure Functions - 400 instead of 500 for missing body - Make test more realistic on parser failures; see Core change for where this matters. Fixes #1841 (thanks to @nihalgonsalves for a good set of test cases). Fixes #4165.
apollographql · Jun 15, 2021 · 46d9329 · 46d9329
1 parent 5da3668
commit 46d9329
Show file tree

Hide file tree

Showing 17 changed files with 216 additions and 54 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -99,6 +99,7 @@ Certain undersupported and underused Apollo Server features have been removed in
   - Removed the `playground` option provided to the `ApolloServer` constructor. You can customize GraphQL Playground or enable it in a production environment by installing the new `ApolloServerPluginLandingPageGraphQLPlayground` plugin.
   - To disable GraphQL Playground, either install the new `ApolloServerPluginLandingPageDisabled` plugin or install any other plugin that implements `renderLandingPage`.
   - Apollo Server packages no longer export `defaultPlaygroundOptions`, `PlaygroundConfig`, or `PlaygroundRenderPageOptions`. By default, no GraphQL Playground settings are overridden, including the endpoint, which now defaults to `window.location.href` (with most query parameters removed). This means you typically don't have to manually configure the endpoint.
+- Bad request errors (invalid JSON, missing body, etc) are more consistent across integrations and consistently return 4xx status codes instead of sometimes returning 5xx status codes.
 
 #### Changes to Node.js framework integrations
 

diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/apollo-server-azure-functions/src/__tests__/azureFunctionApollo.test.ts b/packages/apollo-server-azure-functions/src/__tests__/azureFunctionApollo.test.ts
@@ -6,6 +6,7 @@ import testSuite, {
 import { Config } from 'apollo-server-core';
 import url from 'url';
 import { IncomingMessage, ServerResponse } from 'http';
+import typeis from 'type-is';
 
 const createAzureFunction = async (options: CreateAppOptions = {}) => {
   const server = new ApolloServer(
@@ -25,13 +26,28 @@ const createAzureFunction = async (options: CreateAppOptions = {}) => {
     req.on('data', (chunk) => (body += chunk));
     req.on('end', () => {
       const urlObject = url.parse(req.url!, true);
+      const contentType = req.headers['content-type'];
       const request = {
         method: req.method,
-        body: body && JSON.parse(body),
+        body,
         path: req.url,
         query: urlObject.query,
         headers: req.headers,
       };
+      if (
+        body &&
+        contentType &&
+        req.headers['content-length'] &&
+        req.headers['content-length'] !== '0' &&
+        typeis.is(contentType, 'application/json')
+      ) {
+        try {
+          request.body = JSON.parse(body);
+        } catch (e) {
+          // Leaving body as string seems to be what Azure Functions does.
+          // https://github.com/Azure/azure-functions-host/blob/ba408f522b59228f7fcf9c64223d2ef109ca810d/src/WebJobs.Script.Grpc/MessageExtensions/GrpcMessageConversionExtensions.cs#L251-L264
+        }
+      }
       const context = {
         done(error: any, result: any) {
           if (error) throw error;
@@ -51,7 +67,11 @@ const createAzureFunction = async (options: CreateAppOptions = {}) => {
 };
 
 describe('integration:AzureFunctions', () => {
-  testSuite({ createApp: createAzureFunction, serverlessFramework: true });
+  testSuite({
+    createApp: createAzureFunction,
+    serverlessFramework: true,
+    integrationName: 'azure-functions',
+  });
 
   it('can append CORS headers to GET request', async () => {
     const server = new ApolloServer({ schema: Schema });

diff --git a/packages/apollo-server-azure-functions/src/azureFunctionApollo.ts b/packages/apollo-server-azure-functions/src/azureFunctionApollo.ts
@@ -28,7 +28,7 @@ export function graphqlAzureFunction(
     if (request.method === 'POST' && !request.body) {
       callback(null, {
         body: 'POST body missing.',
-        status: 500,
+        status: 400,
       });
       return;
     }

diff --git a/packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts b/packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts
@@ -48,7 +48,7 @@ describe('runHttpQuery', () => {
             errors: [
               {
                 message:
-                  'GraphQL operations must contain a `query` or a `persistedQuery` extension.',
+                  'GraphQL operations must contain a non-empty `query` or a `persistedQuery` extension.',
                 extensions: { code: 'INTERNAL_SERVER_ERROR' },
               },
             ],

diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts
@@ -189,7 +189,7 @@ export async function processGraphQLRequest<TContext>(
   } else {
     return await sendErrorResponse(
       new GraphQLError(
-        'GraphQL operations must contain a `query` or a `persistedQuery` extension.',
+        'GraphQL operations must contain a non-empty `query` or a `persistedQuery` extension.',
       ),
     );
   }

diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts
@@ -202,10 +202,15 @@ export async function processHTTPRequest<TContext>(
 
   switch (httpRequest.method) {
     case 'POST':
-      if (!httpRequest.query || Object.keys(httpRequest.query).length === 0) {
+      if (
+        !httpRequest.query ||
+        typeof httpRequest.query === 'string' ||
+        Buffer.isBuffer(httpRequest.query) ||
+        Object.keys(httpRequest.query).length === 0
+      ) {
         throw new HttpQueryError(
-          500,
-          'POST body missing. Did you forget use body-parser middleware?',
+          400,
+          'POST body missing, invalid Content-Type, or JSON object has no keys.',
         );
       }
 

diff --git a/packages/apollo-server-express/src/ApolloServer.ts b/packages/apollo-server-express/src/ApolloServer.ts
@@ -131,6 +131,25 @@ export class ApolloServer extends ApolloServerBase {
         return;
       }
 
+      if (!req.body) {
+        // The json body-parser *always* sets req.body to {} if it's unset (even
+        // if the Content-Type doesn't match), so if it isn't set, you probably
+        // forgot to set up body-parser.
+        res.status(500);
+        if (bodyParserConfig === false) {
+          res.send(
+            '`res.body` is not set; you passed `bodyParserConfig: false`, ' +
+              'but you still need to use `body-parser` middleware yourself.',
+          );
+        } else {
+          res.send(
+            '`res.body` is not set even though Apollo Server installed ' +
+              "`body-parser` middleware; this shouldn't happen!",
+          );
+        }
+        return;
+      }
+
       runHttpQuery([], {
         method: req.method,
         options: () => this.createGraphQLServerOptions(req, res),

diff --git a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts
@@ -168,6 +168,21 @@ describe('apollo-server-express', () => {
       });
     });
 
+    it('gives helpful error if body is not parsed', async () => {
+      const { server, httpServer } = await createServer(
+        {
+          typeDefs,
+          resolvers,
+        },
+        { bodyParserConfig: false },
+      );
+
+      await request(httpServer)
+        .post(server.graphqlPath)
+        .send({ query: '{hello}' })
+        .expect(500, /need to use `body-parser`/);
+    });
+
     describe('healthchecks', () => {
       it('creates a healthcheck endpoint', async () => {
         const { httpServer } = await createServer({

diff --git a/packages/apollo-server-fastify/src/__tests__/fastifyApollo.test.ts b/packages/apollo-server-fastify/src/__tests__/fastifyApollo.test.ts
@@ -37,5 +37,5 @@ describe('fastifyApollo', () => {
 });
 
 describe('integration:Fastify', () => {
-  testSuite({createApp, destroyApp});
+  testSuite({ createApp, destroyApp, integrationName: 'fastify' });
 });
diff --git a/packages/apollo-server-hapi/src/__tests__/hapiApollo.test.ts b/packages/apollo-server-hapi/src/__tests__/hapiApollo.test.ts
@@ -34,5 +34,5 @@ describe('integration:Hapi', () => {
     await new Promise(resolve => app.close(resolve));
   }
 
-  testSuite({createApp, destroyApp});
+  testSuite({createApp, destroyApp, integrationName: 'hapi'});
 });
diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts
@@ -220,10 +220,12 @@ export default ({
   createApp,
   destroyApp,
   serverlessFramework,
+  integrationName,
 }: {
   createApp: CreateAppFunc;
   destroyApp?: DestroyAppFunc;
   serverlessFramework?: boolean;
+  integrationName?: string;
 }) => {
   describe('apolloServer', () => {
     let app: any;
@@ -253,12 +255,85 @@ export default ({
         });
       });
 
-      it('throws an error if POST body is missing', async () => {
+      it('throws an error if POST body is empty', async () => {
         app = await createApp();
-        const req = request(app).post('/graphql').send();
+        const req = request(app).post('/graphql').type('text/plain').send('  ');
         return req.then((res) => {
-          expect(res.status).toEqual(500);
-          expect((res.error as HTTPError).text).toMatch('POST body missing.');
+          expect(res.status).toEqual(400);
+          expect((res.error as HTTPError).text).toMatch('POST body missing');
+        });
+      });
+
+      it('throws an error if POST body is missing even with content-type', async () => {
+        app = await createApp();
+        const req = request(app)
+          .post('/graphql')
+          .type('application/json')
+          .send();
+        return req.then((res) => {
+          expect(res.status).toEqual(400);
+          expect((res.error as HTTPError).text).toMatch(
+            integrationName === 'fastify'
+              ? "Body cannot be empty when content-type is set to 'application/json'"
+              : 'POST body missing',
+          );
+        });
+      });
+
+      it('throws an error if invalid content-type', async () => {
+        app = await createApp();
+        const req = request(app)
+          .post('/graphql')
+          .type('text/plain')
+          .send(
+            JSON.stringify({
+              query: 'query test{ testString }',
+            }),
+          );
+        return req.then((res) => {
+          expect(res.status).toEqual(400);
+          expect((res.error as HTTPError).text).toMatch('invalid Content-Type');
+        });
+      });
+
+      it('throws an error if POST operation is missing', async () => {
+        app = await createApp();
+        const req = request(app).post('/graphql').send({});
+        return req.then((res) => {
+          expect(res.status).toEqual(400);
+          expect((res.error as HTTPError).text).toMatch('has no keys');
+        });
+      });
+
+      it('throws an error if POST operation is empty', async () => {
+        app = await createApp();
+        const req = request(app).post('/graphql').send({ query: '' });
+        return req.then((res) => {
+          expect(res.status).toEqual(400);
+          expect((res.error as HTTPError).text).toMatch('non-empty `query`');
+        });
+      });
+
+      it('throws an error if POST JSON is malformed', async () => {
+        app = await createApp();
+        const req = request(app)
+          .post('/graphql')
+          .type('application/json')
+          .send('{foo');
+        return req.then((res) => {
+          expect(res.status).toEqual(400);
+          expect((res.error as HTTPError).text).toMatch(
+            integrationName === 'hapi'
+              ? 'Invalid request payload JSON format'
+              : integrationName === 'micro'
+              ? 'Invalid JSON'
+              : integrationName === 'azure-functions'
+              ? // This is not really the right message but AF does its parsing
+                // outside of our handlers and getting it actually right was too
+                // much of a pain.
+                'POST body missing, invalid Content-Type, or JSON object has no keys.'
+              : 'Unexpected token f',
+          );
         });
       });
 

diff --git a/packages/apollo-server-koa/src/__tests__/koaApollo.test.ts b/packages/apollo-server-koa/src/__tests__/koaApollo.test.ts
@@ -8,6 +8,12 @@ async function createApp(options: CreateAppOptions = {}) {
   const Koa = require('koa');
   const { ApolloServer } = require('../ApolloServer');
   const app = new Koa();
+  // Let's have errors be exposed to "users" instead of logged
+  // since it's a pain for us to check logs generically in this
+  // test suite.
+  app.on('error', (e: any) => {
+    e.expose = true;
+  });
 
   const server = new ApolloServer(
     (options.graphqlOptions as Config) || { schema: Schema },
@@ -21,18 +27,18 @@ async function destroyApp(app: any) {
   if (!app || !app.close) {
     return;
   }
-  await new Promise(resolve => app.close(resolve));
+  await new Promise((resolve) => app.close(resolve));
 }
 
 describe('koaApollo', () => {
   const { ApolloServer } = require('../ApolloServer');
-  it('throws error if called without schema', function() {
+  it('throws error if called without schema', function () {
     expect(() => new ApolloServer(undefined as any)).toThrow(
       'ApolloServer requires options.',
     );
   });
 });
 
 describe('integration:Koa', () => {
-  testSuite({createApp, destroyApp});
+  testSuite({ createApp, destroyApp });
 });
diff --git a/packages/apollo-server-micro/package.json b/packages/apollo-server-micro/package.json
@@ -27,6 +27,7 @@
     "@hapi/accept": "^5.0.2",
     "apollo-server-core": "file:../apollo-server-core",
     "apollo-server-types": "file:../apollo-server-types",
+    "type-is": "^1.6.18",
     "micro": "^9.3.4"
   },
   "devDependencies": {