From 74eb152351458a1bb5d9489a146bdc619a4fffaf Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 10 Jun 2021 16:45:10 -0700 Subject: [PATCH] Make early errors more consistent and more 4xxy 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. --- CHANGELOG.md | 1 + package-lock.json | 6 +- .../src/__tests__/azureFunctionApollo.test.ts | 24 +++++- .../src/azureFunctionApollo.ts | 2 +- .../src/__tests__/runHttpQuery.test.ts | 2 +- .../apollo-server-core/src/requestPipeline.ts | 2 +- .../apollo-server-core/src/runHttpQuery.ts | 11 ++- .../apollo-server-express/src/ApolloServer.ts | 19 +++++ .../src/__tests__/ApolloServer.test.ts | 15 ++++ .../src/__tests__/fastifyApollo.test.ts | 2 +- .../src/__tests__/hapiApollo.test.ts | 2 +- .../src/index.ts | 83 ++++++++++++++++++- .../src/__tests__/koaApollo.test.ts | 12 ++- packages/apollo-server-micro/package.json | 1 + .../apollo-server-micro/src/ApolloServer.ts | 49 ++++++----- .../src/__tests__/microApollo.test.ts | 12 +-- .../apollo-server-micro/src/microApollo.ts | 27 +++--- 17 files changed, 216 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c22860dc258..74eaad70042 100644 --- 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 index adcb0982e29..ff528feb2aa 100644 --- a/package-lock.json +++ b/package-lock.json @@ -21727,7 +21727,8 @@ "@hapi/accept": "^5.0.2", "apollo-server-core": "file:../apollo-server-core", "apollo-server-types": "file:../apollo-server-types", - "micro": "^9.3.4" + "micro": "^9.3.4", + "type-is": "^1.6.18" }, "devDependencies": { "apollo-server-integration-testsuite": "file:../apollo-server-integration-testsuite" @@ -27258,7 +27259,8 @@ "apollo-server-core": "file:../apollo-server-core", "apollo-server-integration-testsuite": "file:../apollo-server-integration-testsuite", "apollo-server-types": "file:../apollo-server-types", - "micro": "^9.3.4" + "micro": "^9.3.4", + "type-is": "^1.6.18" } }, "apollo-server-plugin-base": { diff --git a/packages/apollo-server-azure-functions/src/__tests__/azureFunctionApollo.test.ts b/packages/apollo-server-azure-functions/src/__tests__/azureFunctionApollo.test.ts index ee19097c910..bec421a7324 100644 --- 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 index 63f6be52740..3feccbaa389 100644 --- 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 index 7c23dec789c..62db5c5a853 100644 --- 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 index 4b9b5dd2879..4f73e140373 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -189,7 +189,7 @@ export async function processGraphQLRequest( } 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 index c8a38a94ff6..1f97180aebf 100644 --- a/packages/apollo-server-core/src/runHttpQuery.ts +++ b/packages/apollo-server-core/src/runHttpQuery.ts @@ -202,10 +202,15 @@ export async function processHTTPRequest( 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 index cc76020229e..75a16f49188 100644 --- 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 index 54d63614e07..dfea55e0f69 100644 --- 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 index 3f291339f9a..7cb688dbeb5 100644 --- 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 index 795c9409767..a1bb2cf9f73 100644 --- 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 index 3f8cbebeae4..d2503c8868d 100644 --- 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 index d42e89b27b2..ac4247f1649 100644 --- 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,12 +27,12 @@ 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.', ); @@ -34,5 +40,5 @@ describe('koaApollo', () => { }); 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 index 5cf04992a77..0b3927d8eec 100644 --- 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": { diff --git a/packages/apollo-server-micro/src/ApolloServer.ts b/packages/apollo-server-micro/src/ApolloServer.ts index 78430f93e38..4706d8f7deb 100644 --- a/packages/apollo-server-micro/src/ApolloServer.ts +++ b/packages/apollo-server-micro/src/ApolloServer.ts @@ -11,6 +11,7 @@ export interface ServerRegistration { path?: string; disableHealthCheck?: boolean; onHealthCheck?: (req: MicroRequest) => Promise; + __testing__microSuppressErrorLog?: boolean; } export class ApolloServer extends ApolloServerBase { @@ -28,6 +29,7 @@ export class ApolloServer extends ApolloServerBase { path, disableHealthCheck, onHealthCheck, + __testing__microSuppressErrorLog, }: ServerRegistration = {}) { this.assertStarted('createHandler'); @@ -36,26 +38,35 @@ export class ApolloServer extends ApolloServerBase { const landingPage = this.getLandingPage(); return async (req: MicroRequest, res: ServerResponse) => { - if ( - await this.handleHealthCheck({ - req, - res, - disableHealthCheck, - onHealthCheck, - }) - ) { - return; - } - if ( - landingPage && - this.handleGraphqlRequestsWithLandingPage({ req, res, landingPage }) - ) { - return; - } - if (await this.handleGraphqlRequestsWithServer({ req, res })) { - return; + try { + if ( + await this.handleHealthCheck({ + req, + res, + disableHealthCheck, + onHealthCheck, + }) + ) { + return; + } + if ( + landingPage && + this.handleGraphqlRequestsWithLandingPage({ req, res, landingPage }) + ) { + return; + } + if (await this.handleGraphqlRequestsWithServer({ req, res })) { + return; + } + send(res, 404, null); + } catch (errorObj) { + if (!__testing__microSuppressErrorLog) { + throw errorObj; + } + // Like Micro's sendError but without the logging. + const statusCode = errorObj.statusCode || errorObj.status; + send(res, statusCode || 500, errorObj.stack); } - send(res, 404, null); }; } diff --git a/packages/apollo-server-micro/src/__tests__/microApollo.test.ts b/packages/apollo-server-micro/src/__tests__/microApollo.test.ts index 5442b36ee08..6c4d44d30d6 100644 --- a/packages/apollo-server-micro/src/__tests__/microApollo.test.ts +++ b/packages/apollo-server-micro/src/__tests__/microApollo.test.ts @@ -12,17 +12,19 @@ async function createApp(options: CreateAppOptions = {}) { (options.graphqlOptions as Config) || { schema: Schema }, ); await server.start(); - return micro(server.createHandler()); + return micro( + server.createHandler({ __testing__microSuppressErrorLog: true }), + ); } -describe('microApollo', function() { - it('should throw an error if called without a schema', function() { +describe('microApollo', function () { + it('should throw an error if called without a schema', function () { expect(() => new ApolloServer(undefined as any)).toThrow( 'ApolloServer requires options.', ); }); }); -describe('integration:Micro', function() { - testSuite({createApp}); +describe('integration:Micro', function () { + testSuite({ createApp, integrationName: 'micro' }); }); diff --git a/packages/apollo-server-micro/src/microApollo.ts b/packages/apollo-server-micro/src/microApollo.ts index 5704c512e17..3e7ad6db1d2 100644 --- a/packages/apollo-server-micro/src/microApollo.ts +++ b/packages/apollo-server-micro/src/microApollo.ts @@ -6,6 +6,7 @@ import { import { send, json, RequestHandler } from 'micro'; import url from 'url'; import { IncomingMessage, ServerResponse } from 'http'; +import typeis from 'type-is'; import { MicroRequest } from './types'; import { ValueOrPromise } from 'apollo-server-types'; @@ -16,7 +17,10 @@ export interface MicroGraphQLOptionsFunction { } // Utility function used to set multiple headers on a response object. -function setHeaders(res: ServerResponse, headers: Record): void { +function setHeaders( + res: ServerResponse, + headers: Record, +): void { Object.entries(headers).forEach(([header, value]) => { res.setHeader(header, value); }); @@ -39,21 +43,22 @@ export function graphqlMicro( } const graphqlHandler = async (req: MicroRequest, res: ServerResponse) => { - let query: any; - try { - query = - req.method === 'POST' - ? req.filePayload || (await json(req)) - : url.parse(req.url!, true).query; - } catch (error) { - // Do nothing; `query` stays `undefined` - } + const contentType = req.headers['content-type']; + const query = + req.method === 'POST' + ? req.filePayload || + (contentType && + req.headers['content-length'] && + req.headers['content-length'] !== '0' && + typeis.is(contentType, 'application/json') && + (await json(req))) + : url.parse(req.url!, true).query; try { const { graphqlResponse, responseInit } = await runHttpQuery([req, res], { method: req.method!, options, - query, + query: query as any, request: convertNodeHttpToRequest(req), }); setHeaders(res, responseInit.headers!);