From e0f7052fb1f6bb0952df06e3729bd4d4b376ae61 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Fri, 29 Jun 2018 10:36:52 -0700 Subject: [PATCH] Errors Lifecycle: user extensions > engine reporting > formatError (#1272) * enable willSendResponse to return a modified response * add formatError as an extension that wraps engine reporting * ensure that formatError once on every error path * move old formatError express tests into integration suite * add error lifecycle with minimal engine reporting check * increase granularity of formatError test * return 400 error for GraphQL error created by context * add check for internal server error for errors thrown in context * comment about context error status code --- .../apollo-server-core/src/ApolloServer.ts | 16 +- packages/apollo-server-core/src/formatters.ts | 29 +++ .../apollo-server-core/src/runHttpQuery.ts | 53 +++-- packages/apollo-server-core/src/runQuery.ts | 7 +- .../src/apolloServerHttp.test.ts | 68 ------ .../src/ApolloServer.test.ts | 1 - .../package.json | 4 + .../src/ApolloServer.ts | 201 +++++++++++++++++- .../src/index.ts | 116 ++++++++++ packages/graphql-extensions/src/index.ts | 18 +- 10 files changed, 416 insertions(+), 97 deletions(-) create mode 100644 packages/apollo-server-core/src/formatters.ts diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index a23a3ca60c8..b888dbb2b89 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -39,6 +39,8 @@ import { FileUploadOptions, } from './types'; +import { FormatErrorExtension } from './formatters'; + import { gql } from './index'; const NoIntrospection = (context: ValidationContext) => ({ @@ -185,11 +187,23 @@ export class ApolloServerBase { // or cacheControl. this.extensions = []; + // Error formatting should happen after the engine reporting agent, so that + // engine gets the unmasked errors if necessary + if (this.requestOptions.formatError) { + this.extensions.push( + () => + new FormatErrorExtension( + this.requestOptions.formatError, + this.requestOptions.debug, + ), + ); + } + if (engine || (engine !== false && process.env.ENGINE_API_KEY)) { this.engineReportingAgent = new EngineReportingAgent( engine === true ? {} : engine, ); - // Let's keep this extension first so it wraps everything. + // Let's keep this extension second so it wraps everything, except error formatting this.extensions.push(() => this.engineReportingAgent.newExtension()); } diff --git a/packages/apollo-server-core/src/formatters.ts b/packages/apollo-server-core/src/formatters.ts new file mode 100644 index 00000000000..8e708b41393 --- /dev/null +++ b/packages/apollo-server-core/src/formatters.ts @@ -0,0 +1,29 @@ +import { GraphQLExtension, GraphQLResponse } from 'graphql-extensions'; +import { formatApolloErrors } from 'apollo-server-errors'; + +export class FormatErrorExtension extends GraphQLExtension { + private formatError: Function; + private debug: boolean; + + public constructor(formatError: Function, debug: boolean = false) { + super(); + this.formatError = formatError; + this.debug = debug; + } + + public willSendResponse(o: { + graphqlResponse: GraphQLResponse; + }): void | { graphqlResponse: GraphQLResponse } { + if (o.graphqlResponse.errors) { + return { + graphqlResponse: { + ...o.graphqlResponse, + errors: formatApolloErrors(o.graphqlResponse.errors, { + formatter: this.formatError, + debug: this.debug, + }), + }, + }; + } + } +} diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts index ddfc1442fc2..762c7914668 100644 --- a/packages/apollo-server-core/src/runHttpQuery.ts +++ b/packages/apollo-server-core/src/runHttpQuery.ts @@ -69,18 +69,23 @@ export class HttpQueryError extends Error { } } -function throwHttpGraphQLError( - statusCode, - errors: Array, - optionsObject, +/** + * If optionsObject is specified, then the errors array will be formatted + */ +function throwHttpGraphQLError( + statusCode: number, + errors: Array, + optionsObject?: Partial, ) { throw new HttpQueryError( statusCode, prettyJSONStringify({ - errors: formatApolloErrors(errors, { - debug: optionsObject.debug, - formatter: optionsObject.formatError, - }), + errors: optionsObject + ? formatApolloErrors(errors, { + debug: optionsObject.debug, + formatter: optionsObject.formatError, + }) + : errors, }), true, { @@ -302,7 +307,17 @@ export async function runHttpQuery( context = await context(); } catch (e) { e.message = `Context creation failed: ${e.message}`; - throwHttpGraphQLError(500, [e], optionsObject); + // For errors that are not internal, such as authentication, we + // should provide a 400 response + if ( + e.extensions && + e.extensions.code && + e.extensions.code !== 'INTERNAL_SERVER_ERROR' + ) { + throwHttpGraphQLError(400, [e], optionsObject); + } else { + throwHttpGraphQLError(500, [e], optionsObject); + } } } else { // Always clone the context if it's not a function, because that preserves @@ -402,16 +417,23 @@ export async function runHttpQuery( throw e; } + // This error will be uncaught, so we need to wrap it and treat it as an + // internal server error return { - errors: formatApolloErrors([e], { - formatter: optionsObject.formatError, - debug: optionsObject.debug, - }), + errors: formatApolloErrors([e], optionsObject), }; } }) as Array }>>; - const responses = await Promise.all(requests); + let responses; + try { + responses = await Promise.all(requests); + } catch (e) { + if (e.name === 'HttpQueryError') { + throw e; + } + throwHttpGraphQLError(500, [e], optionsObject); + } const responseInit: ApolloServerHttpResponse = { headers: { @@ -449,7 +471,8 @@ export async function runHttpQuery( // This code is run on parse/validation errors and any other error that // doesn't reach GraphQL execution if (graphqlResponse.errors && typeof graphqlResponse.data === 'undefined') { - throwHttpGraphQLError(400, graphqlResponse.errors as any, optionsObject); + // don't include optionsObject, since the errors have already been formatted + throwHttpGraphQLError(400, graphqlResponse.errors as any); } const stringified = prettyJSONStringify(graphqlResponse); diff --git a/packages/apollo-server-core/src/runQuery.ts b/packages/apollo-server-core/src/runQuery.ts index dc75e98ecab..030e839d437 100644 --- a/packages/apollo-server-core/src/runQuery.ts +++ b/packages/apollo-server-core/src/runQuery.ts @@ -159,7 +159,6 @@ function doRunQuery(options: QueryOptions): Promise { }), ], { - formatter: options.formatError, debug, }, ); @@ -199,7 +198,6 @@ function doRunQuery(options: QueryOptions): Promise { fromGraphQLError(err, { errorClass: ValidationError }), ), { - formatter: options.formatError, debug, }, ); @@ -250,7 +248,6 @@ function doRunQuery(options: QueryOptions): Promise { if (result.errors) { response.errors = formatApolloErrors([...result.errors], { - formatter: options.formatError, debug, }); } @@ -277,8 +274,8 @@ function doRunQuery(options: QueryOptions): Promise { throw err; }) .then(graphqlResponse => { - extensionStack.willSendResponse({ graphqlResponse }); + const response = extensionStack.willSendResponse({ graphqlResponse }); requestDidEnd(); - return graphqlResponse; + return response.graphqlResponse; }); } diff --git a/packages/apollo-server-express/src/apolloServerHttp.test.ts b/packages/apollo-server-express/src/apolloServerHttp.test.ts index 5da4869efcc..08f8255a6c0 100644 --- a/packages/apollo-server-express/src/apolloServerHttp.test.ts +++ b/packages/apollo-server-express/src/apolloServerHttp.test.ts @@ -417,74 +417,6 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => { expect(response.status).to.equal(400); }); - it('allows for custom error formatting to sanitize', async () => { - const app = express(); - - app.use('/graphql', bodyParser.json()); - app.use( - '/graphql', - graphqlExpress({ - schema: TestSchema, - formatError(error) { - return { message: 'Custom error format: ' + error.message }; - }, - }), - ); - - const response = await request(app) - .post('/graphql') - .send({ - query: '{thrower}', - }); - - expect(response.status).to.equal(200); - expect(JSON.parse(response.text)).to.deep.equal({ - data: null, - errors: [ - { - message: 'Custom error format: Throws!', - }, - ], - }); - }); - - it('allows for custom error formatting to elaborate', async () => { - const app = express(); - - app.use('/graphql', bodyParser.json()); - app.use( - '/graphql', - graphqlExpress({ - schema: TestSchema, - formatError(error) { - return { - message: error.message, - locations: error.locations, - stack: 'Stack trace', - }; - }, - }), - ); - - const response = await request(app) - .post('/graphql') - .send({ - query: '{thrower}', - }); - - expect(response.status).to.equal(200); - expect(JSON.parse(response.text)).to.deep.equal({ - data: null, - errors: [ - { - message: 'Throws!', - locations: [{ line: 1, column: 2 }], - stack: 'Stack trace', - }, - ], - }); - }); - it('handles unsupported HTTP methods', async () => { const app = express(); diff --git a/packages/apollo-server-hapi/src/ApolloServer.test.ts b/packages/apollo-server-hapi/src/ApolloServer.test.ts index 79335e1fddf..f82803502cf 100644 --- a/packages/apollo-server-hapi/src/ApolloServer.test.ts +++ b/packages/apollo-server-hapi/src/ApolloServer.test.ts @@ -212,7 +212,6 @@ describe('apollo-server-hapi', () => { const apolloFetch = createApolloFetch({ uri }).useAfter( (response, next) => { - console.log(response.response.headers); expect( response.response.headers.get('access-control-expose-headers'), ).to.deep.equal( diff --git a/packages/apollo-server-integration-testsuite/package.json b/packages/apollo-server-integration-testsuite/package.json index 3cf63aff439..9ea6df58555 100644 --- a/packages/apollo-server-integration-testsuite/package.json +++ b/packages/apollo-server-integration-testsuite/package.json @@ -26,11 +26,15 @@ "apollo-server-core": "^2.0.0-rc.5" }, "devDependencies": { + "@types/body-parser": "1.17.0", + "apollo-engine-reporting-protobuf": "0.0.0-beta.7", "apollo-fetch": "^0.7.0", "apollo-link": "^1.2.2", "apollo-link-http": "^1.5.4", "apollo-link-persisted-queries": "^0.2.1", "apollo-server-env": "^2.0.0-rc.3", + "body-parser": "^1.18.3", + "graphql-extensions": "^0.1.0-beta.15", "graphql-subscriptions": "^0.5.8", "graphql-tag": "^2.9.2", "js-sha256": "^0.9.0", diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index 387223ad8b5..d39c5bab691 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -5,6 +5,10 @@ import * as http from 'http'; import * as net from 'net'; import 'mocha'; import { sha256 } from 'js-sha256'; +import express = require('express'); +import bodyParser = require('body-parser'); + +import { Trace } from 'apollo-engine-reporting-protobuf'; import { GraphQLSchema, @@ -33,6 +37,7 @@ import { Config, ApolloServerBase, } from 'apollo-server-core'; +import { GraphQLExtension, GraphQLResponse } from 'graphql-extensions'; export function createServerInfo( server: AS, @@ -153,11 +158,20 @@ export function testApolloServer( expect(introspectionResult.data, 'data should not exist').not.to .exist; expect(introspectionResult.errors, 'errors should exist').to.exist; + expect(introspectionResult.errors[0].message).to.match( + /introspection/, + ); + expect(formatError.callCount).to.equal( + introspectionResult.errors.length, + ); const result = await apolloFetch({ query: TEST_STRING_QUERY }); expect(result.data, 'data should not exist').not.to.exist; expect(result.errors, 'errors should exist').to.exist; - expect(formatError.called).true; + expect(result.errors[0].message).to.match(/Not allowed/); + expect(formatError.callCount).to.equal( + introspectionResult.errors.length + result.errors.length, + ); }); it('allows introspection by default', async () => { @@ -322,16 +336,197 @@ export function testApolloServer( }); expect(introspectionResult.data, 'data should not exist').not.to.exist; expect(introspectionResult.errors, 'errors should exist').to.exist; + expect(formatError.calledOnce).true; + expect(throwError.calledOnce).true; const result = await apolloFetch({ query: TEST_STRING_QUERY }); expect(result.data, 'data should not exist').not.to.exist; expect(result.errors, 'errors should exist').to.exist; - expect(formatError.called).true; - expect(throwError.called).true; + expect(formatError.calledTwice).true; + expect(throwError.calledTwice).true; }); }); describe('lifecycle', () => { + async function startEngineServer({ port, check }) { + const engine = express(); + engine.use((req, _res, next) => { + // body parser requires a content-type + req.headers['content-type'] = 'text/plain'; + next(); + }); + engine.use( + bodyParser.raw({ + inflate: true, + type: '*/*', + }), + ); + engine.use(check); + return await engine.listen(port); + } + + it('validation > engine > extensions > formatError', async () => { + return new Promise(async (resolve, reject) => { + const nodeEnv = process.env.NODE_ENV; + delete process.env.NODE_ENV; + + let listener = await startEngineServer({ + port: 10101, + check: (req, res) => { + const trace = JSON.stringify(Trace.decode(req.body)); + try { + expect(trace).to.match(/nope/); + expect(trace).not.to.match(/masked/); + } catch (e) { + reject(e); + } + res.end(); + listener.close(resolve); + }, + }); + + const throwError = stub().callsFake(() => { + throw new Error('nope'); + }); + + const validationRule = stub().callsFake(() => { + expect( + formatError.notCalled, + 'formatError should be called after validation', + ).true; + expect( + extension.notCalled, + 'extension should be called after validation', + ).true; + return true; + }); + const extension = stub(); + + const formatError = stub().callsFake(error => { + expect(error instanceof Error).true; + expect( + extension.calledOnce, + 'extension should be called before formatError', + ).true; + expect( + validationRule.calledOnce, + 'validationRules should be called before formatError', + ).true; + + error.message = 'masked'; + return error; + }); + + class Extension extends GraphQLExtension { + willSendResponse(o: { graphqlResponse: GraphQLResponse }) { + expect(o.graphqlResponse.errors.length).to.equal(1); + expect( + formatError.notCalled, + 'formatError should be called after extensions', + ).true; + expect( + validationRule.calledOnce, + 'validationRules should be called before extensions', + ).true; + extension(); + } + } + + const { url: uri } = await createApolloServer({ + typeDefs: gql` + type Query { + error: String + } + `, + resolvers: { + Query: { + error: () => { + throwError(); + }, + }, + }, + validationRules: [validationRule], + extensions: [() => new Extension()], + engine: { + endpointUrl: 'http://localhost:10101', + apiKey: 'fake', + maxUncompressedReportSize: 1, + }, + formatError, + debug: true, + }); + + const apolloFetch = createApolloFetch({ uri }); + + const result = await apolloFetch({ + query: `{error}`, + }); + expect(result.data).to.deep.equal({ + error: null, + }); + expect(result.errors, 'errors should exist').to.exist; + expect(result.errors[0].message).to.equal('masked'); + expect(formatError.calledOnce).true; + expect(throwError.calledOnce).true; + + process.env.NODE_ENV = nodeEnv; + }); + }); + + it('errors thrown in extensions call formatError and are wrapped', async () => { + const extension = stub().callsFake(() => { + throw new Error('nope'); + }); + + const formatError = stub().callsFake(error => { + expect(error instanceof Error).true; + expect( + extension.calledOnce, + 'extension should be called before formatError', + ).true; + + error.message = 'masked'; + return error; + }); + + class Extension extends GraphQLExtension { + willSendResponse(_o: { graphqlResponse: GraphQLResponse }) { + expect( + formatError.notCalled, + 'formatError should be called after extensions', + ).true; + extension(); + } + } + + const { url: uri } = await createApolloServer({ + typeDefs: gql` + type Query { + error: String + } + `, + resolvers: { + Query: { + error: () => {}, + }, + }, + extensions: [() => new Extension()], + formatError, + debug: true, + }); + + const apolloFetch = createApolloFetch({ uri }); + + const result = await apolloFetch({ + query: `{error}`, + }); + expect(result.data, 'data should not exist').to.not.exist; + expect(result.errors, 'errors should exist').to.exist; + expect(result.errors[0].message).to.equal('masked'); + expect(result.errors[0].message).to.equal('masked'); + expect(formatError.calledOnce).true; + }); + it('defers context eval with thunk until after options creation', async () => { const uniqueContext = { key: 'major' }; const typeDefs = gql` diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts index b368d9cb578..0a4c5bae809 100644 --- a/packages/apollo-server-integration-testsuite/src/index.ts +++ b/packages/apollo-server-integration-testsuite/src/index.ts @@ -13,6 +13,7 @@ import { GraphQLInt, GraphQLError, GraphQLNonNull, + GraphQLScalarType, introspectionQuery, BREAK, } from 'graphql'; @@ -24,6 +25,61 @@ import gql from 'graphql-tag'; export * from './ApolloServer'; +const QueryRootType = new GraphQLObjectType({ + name: 'QueryRoot', + fields: { + test: { + type: GraphQLString, + args: { + who: { + type: GraphQLString, + }, + }, + resolve: (_, args) => 'Hello ' + (args['who'] || 'World'), + }, + thrower: { + type: new GraphQLNonNull(GraphQLString), + resolve: () => { + throw new Error('Throws!'); + }, + }, + custom: { + type: GraphQLString, + args: { + foo: { + type: new GraphQLScalarType({ + name: 'Foo', + serialize: v => v, + parseValue: () => { + throw new Error('Something bad happened'); + }, + parseLiteral: () => { + throw new Error('Something bad happened'); + }, + }), + }, + }, + }, + context: { + type: GraphQLString, + resolve: (_obj, _args, context) => context, + }, + }, +}); + +const TestSchema = new GraphQLSchema({ + query: QueryRootType, + mutation: new GraphQLObjectType({ + name: 'MutationRoot', + fields: { + writeTest: { + type: QueryRootType, + resolve: () => ({}), + }, + }, + }), +}); + const personType = new GraphQLObjectType({ name: 'PersonType', fields: { @@ -855,6 +911,66 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => { }); }); + it('allows for custom error formatting to sanitize', async () => { + app = await createApp({ + graphqlOptions: { + schema: TestSchema, + formatError(error) { + return { message: 'Custom error format: ' + error.message }; + }, + }, + }); + + const response = await request(app) + .post('/graphql') + .send({ + query: '{thrower}', + }); + + expect(response.status).to.equal(200); + expect(JSON.parse(response.text)).to.deep.equal({ + data: null, + errors: [ + { + message: 'Custom error format: Throws!', + }, + ], + }); + }); + + it('allows for custom error formatting to elaborate', async () => { + app = await createApp({ + graphqlOptions: { + schema: TestSchema, + formatError(error) { + return { + message: error.message, + locations: error.locations, + stack: 'Stack trace', + }; + }, + }, + }); + + const response = await request(app) + .post('/graphql') + .send({ + query: '{thrower}', + }); + + expect(response.status).to.equal(200); + expect(JSON.parse(response.text)).to.deep.equal({ + data: null, + errors: [ + { + message: 'Throws!', + locations: [{ line: 1, column: 2 }], + stack: 'Stack trace', + }, + ], + }); + }); + it('sends internal server error when formatError fails', async () => { app = await createApp({ graphqlOptions: { diff --git a/packages/graphql-extensions/src/index.ts b/packages/graphql-extensions/src/index.ts index fd5647704ec..e09457768db 100644 --- a/packages/graphql-extensions/src/index.ts +++ b/packages/graphql-extensions/src/index.ts @@ -47,7 +47,9 @@ export class GraphQLExtension { executionArgs: ExecutionArgs; }): EndHandler | void; - public willSendResponse?(o: { graphqlResponse: GraphQLResponse }): void; + public willSendResponse?(o: { + graphqlResponse: GraphQLResponse; + }): void | { graphqlResponse: GraphQLResponse }; public willResolveField?( source: any, @@ -100,12 +102,20 @@ export class GraphQLExtensionStack { ); } - public willSendResponse(o: { graphqlResponse: GraphQLResponse }): void { - this.extensions.forEach(extension => { + public willSendResponse(o: { + graphqlResponse: GraphQLResponse; + }): { graphqlResponse: GraphQLResponse } { + let reference = o; + // Reverse the array, since this is functions as an end handler + [...this.extensions].reverse().forEach(extension => { if (extension.willSendResponse) { - extension.willSendResponse(o); + const result = extension.willSendResponse(reference); + if (result) { + reference = result; + } } }); + return reference; } public willResolveField(