From 1aeb21500deb3eed9b3c413be20d6f7f985b1c6a Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 2 Jun 2022 17:13:41 -0700 Subject: [PATCH] Make top-level errors consistent and observable (#6514) Previously, errors that weren't specially cased as part of the "request pipeline" were not observable by plugins. (In some but not all cases, they were visible to formatError hooks.) Some "HTTP-level" errors were handled by returning text/plain responses, whereas many other errors were handled by returning GraphQL-style JSON responses (using the formatError hook). This change makes things more consistent by returning all errors as JSON responses (using the formatError hook). In addition, for better observability, several new top-level plugin APIs are introduced that are called when various kinds of errors occur. Specifically: - New plugin hook `startupDidFail`. This is called once if startup fails (eg, if a serverWillStart plugin throws or if gateway fails to load the schema). (If this hook throws, its error is logged.) Future calls to executeHTTPGraphQLRequest (if the server was started in the background) now return a formatted JSON error instead of throwing. - New plugin hook `contextCreationDidFail`. (If this hook throws, its error is logged.) As before, these errors are sent as formatted JSON errors. - New plugin hook `invalidRequestWasReceived`. This is called for all the errors that occur before being able to construct a GraphQLRequest from an HTTPGraphQLRequest; this includes things like the top-level fields being of the wrong type, missing body, CSRF failure, etc). These errors are now sent as formatted JSON instead of as text/plain. - New plugin hook `unexpectedErrorProcessingRequest`. This is called if `processGraphQLRequest` throws (typically because a plugin hook threw). We are making the assumption that this means there is a programming error, so we are making the behavior change that the thrown error will be logged and "Internal server error" will be sent in the HTTP response. - Fix grammar of "GET supports only query operation" and don't return it if the operation is unknown. Fixes #6140. Part of #6053. --- packages/server/src/ApolloServer.ts | 215 +++++++++++----- .../server/src/__tests__/ApolloServer.test.ts | 66 +++-- .../integration/apolloServerTests.ts | 135 ++++++++-- .../__tests__/integration/httpServerTests.ts | 84 ++++-- packages/server/src/errors.ts | 17 +- packages/server/src/externalTypes/plugins.ts | 12 + packages/server/src/httpBatching.ts | 8 +- packages/server/src/index.ts | 2 - packages/server/src/preventCsrf.ts | 5 +- packages/server/src/requestPipeline.ts | 10 +- packages/server/src/runHttpQuery.ts | 242 +++++++----------- 11 files changed, 497 insertions(+), 299 deletions(-) diff --git a/packages/server/src/ApolloServer.ts b/packages/server/src/ApolloServer.ts index 1b1ab66a02f..4d35f6e15d7 100644 --- a/packages/server/src/ApolloServer.ts +++ b/packages/server/src/ApolloServer.ts @@ -21,7 +21,7 @@ import Negotiator from 'negotiator'; import * as uuid from 'uuid'; import { newCachePolicy } from './cachePolicy'; import { ApolloConfig, determineApolloConfig } from './config'; -import { formatApolloErrors } from './errors'; +import { BadRequestError, ensureError, formatApolloErrors } from './errors'; import type { ApolloServerPlugin, BaseContext, @@ -35,6 +35,7 @@ import type { LandingPage, PluginDefinition, } from './externalTypes'; +import type { HTTPGraphQLHead } from './externalTypes/http'; import { runPotentiallyBatchedHttpQuery } from './httpBatching'; import { InternalPluginId, pluginIsInternal } from './internalPlugin'; import { @@ -52,6 +53,7 @@ import { } from './preventCsrf'; import { APQ_CACHE_PREFIX, processGraphQLRequest } from './requestPipeline'; import { + badMethodErrorMessage, cloneObject, HeaderMap, newHTTPGraphQLHead, @@ -489,10 +491,24 @@ export class ApolloServer { toDispose, toDisposeLast, }; - } catch (error) { + } catch (maybeError: unknown) { + const error = ensureError(maybeError); + + try { + await Promise.all( + this.internals.plugins.map(async (plugin) => + plugin.startupDidFail?.({ error }), + ), + ); + } catch (pluginError) { + this.internals.logger.error( + `startupDidFail hook threw: ${pluginError}`, + ); + } + this.internals.state = { phase: 'failed to start', - error: error as Error, + error: error, }; throw error; } finally { @@ -959,7 +975,6 @@ export class ApolloServer { } // TODO(AS4): Make sure we like the name of this function. - // TODO(AS4): Should we make this into a function that never throws? public async executeHTTPGraphQLRequest({ httpGraphQLRequest, context, @@ -967,71 +982,123 @@ export class ApolloServer { httpGraphQLRequest: HTTPGraphQLRequest; context: ContextThunk; }): Promise { - // TODO(AS4): This throws if we started in the background and there was an - // error, but instead of throwing it should return an appropriate error - // response. Note that we can make the serverIsStartedInBackground case of - // the `rejected load promise is thrown by server.start` test tighter once - // we fix this. - const runningServerState = await this._ensureStarted(); + try { + let runningServerState; + try { + runningServerState = await this._ensureStarted(); + } catch (error: unknown) { + // This is typically either the masked error from when background startup + // failed, or related to invoking this function before startup or + // during/after shutdown (due to lack of draining). + return this.errorResponse(error); + } - if ( - runningServerState.landingPage && - this.prefersHTML(httpGraphQLRequest) - ) { - return { - headers: new HeaderMap([['content-type', 'text/html']]), - completeBody: runningServerState.landingPage.html, - bodyChunks: null, - }; - } + if ( + runningServerState.landingPage && + this.prefersHTML(httpGraphQLRequest) + ) { + return { + headers: new HeaderMap([['content-type', 'text/html']]), + completeBody: runningServerState.landingPage.html, + bodyChunks: null, + }; + } + + // If enabled, check to ensure that this request was preflighted before doing + // anything real (such as running the context function). + if (this.internals.csrfPreventionRequestHeaders) { + preventCsrf( + httpGraphQLRequest.headers, + this.internals.csrfPreventionRequestHeaders, + ); + } + + let contextValue: TContext; + try { + contextValue = await context(); + } catch (maybeError: unknown) { + const error = ensureError(maybeError); + try { + await Promise.all( + this.internals.plugins.map(async (plugin) => + plugin.contextCreationDidFail?.({ + error, + }), + ), + ); + } catch (pluginError) { + this.internals.logger.error( + `contextCreationDidFail hook threw: ${pluginError}`, + ); + } + + error.message = `Context creation failed: ${error.message}`; + // If we explicitly provide an error code that isn't + // INTERNAL_SERVER_ERROR, we'll treat it as a client error. + const statusCode = + error instanceof GraphQLError && + error.extensions.code && + error.extensions.code !== 'INTERNAL_SERVER_ERROR' + ? 400 + : 500; + return this.errorResponse(error, newHTTPGraphQLHead(statusCode)); + } - // If enabled, check to ensure that this request was preflighted before doing - // anything real (such as running the context function). - if (this.internals.csrfPreventionRequestHeaders) { - // TODO(AS4): We introduced an error handling bug when we ported this to - // version-4: this throws HttpQueryError but we don't treat that specially - // here. - preventCsrf( - httpGraphQLRequest.headers, - this.internals.csrfPreventionRequestHeaders, + return await runPotentiallyBatchedHttpQuery( + httpGraphQLRequest, + contextValue, + runningServerState.schemaManager.getSchemaDerivedData(), + this.internals, ); + } catch (maybeError_: unknown) { + const maybeError = maybeError_; // fixes inference because catch vars are not const + if (maybeError instanceof BadRequestError) { + try { + await Promise.all( + this.internals.plugins.map(async (plugin) => + plugin.invalidRequestWasReceived?.({ error: maybeError }), + ), + ); + } catch (pluginError) { + this.internals.logger.error( + `invalidRequestWasReceived hook threw: ${pluginError}`, + ); + } + return this.errorResponse( + maybeError, + // Quite hacky, but beats putting more stuff on GraphQLError + // subclasses, maybe? + maybeError.message === badMethodErrorMessage + ? { + statusCode: 405, + headers: new HeaderMap([['allow', 'GET, POST']]), + } + : newHTTPGraphQLHead(400), + ); + } + return this.errorResponse(maybeError); } + } - let contextValue: TContext; - try { - contextValue = await context(); - } catch (e: any) { - // XXX `any` isn't ideal, but this is the easiest thing for now, without - // introducing a strong `instanceof GraphQLError` requirement. - e.message = `Context creation failed: ${e.message}`; - // For errors that are not internal, such as authentication, we - // should provide a 400 response - const statusCode = - e.extensions && - e.extensions.code && - e.extensions.code !== 'INTERNAL_SERVER_ERROR' - ? 400 - : 500; - return { - statusCode, - headers: new HeaderMap([['content-type', 'application/json']]), - completeBody: prettyJSONStringify({ - errors: formatApolloErrors([e as Error], { - includeStackTracesInErrorResponses: - this.internals.includeStackTracesInErrorResponses, - formatError: this.internals.formatError, - }), + private errorResponse( + error: unknown, + httpGraphQLHead: HTTPGraphQLHead = newHTTPGraphQLHead(), + ): HTTPGraphQLResponse { + return { + statusCode: httpGraphQLHead.statusCode ?? 500, + headers: new HeaderMap([ + ...httpGraphQLHead.headers, + ['content-type', 'application/json'], + ]), + completeBody: prettyJSONStringify({ + errors: formatApolloErrors([error], { + includeStackTracesInErrorResponses: + this.internals.includeStackTracesInErrorResponses, + formatError: this.internals.formatError, }), - bodyChunks: null, - }; - } - - return await runPotentiallyBatchedHttpQuery( - httpGraphQLRequest, - contextValue, - runningServerState.schemaManager.getSchemaDerivedData(), - this.internals, - ); + }), + bodyChunks: null, + }; } private prefersHTML(request: HTTPGraphQLRequest): boolean { @@ -1146,7 +1213,27 @@ export async function internalExecuteOperation({ metrics: {}, overallCachePolicy: newCachePolicy(), }; - await processGraphQLRequest(schemaDerivedData, internals, requestContext); + + try { + await processGraphQLRequest(schemaDerivedData, internals, requestContext); + } catch (maybeError: unknown) { + // processGraphQLRequest throwing usually means that either there's a bug in + // Apollo Server or some plugin hook threw unexpectedly. + const error = ensureError(maybeError); + // If *these* hooks throw then we'll still get a 500 but won't mask its + // error. + await Promise.all( + internals.plugins.map(async (plugin) => + plugin.unexpectedErrorProcessingRequest?.({ + requestContext, + error, + }), + ), + ); + // Mask unexpected error externally. + internals.logger.error(`Unexpected error processing request: ${error}`); + throw new Error('Internal server error'); + } return requestContext.response; } diff --git a/packages/server/src/__tests__/ApolloServer.test.ts b/packages/server/src/__tests__/ApolloServer.test.ts index 75cbe8a27b9..eeed5c7948b 100644 --- a/packages/server/src/__tests__/ApolloServer.test.ts +++ b/packages/server/src/__tests__/ApolloServer.test.ts @@ -134,16 +134,23 @@ const failToStartPlugin: ApolloServerPlugin = { }; describe('ApolloServer start', () => { - const redactedMessage = - 'This data graph is missing a valid configuration. More details may be available in the server logs.'; - it('start throws on startup error', async () => { + const startupDidFail = jest.fn(); const server = new ApolloServer({ typeDefs, resolvers, - plugins: [failToStartPlugin], + plugins: [failToStartPlugin, { startupDidFail }], }); await expect(server.start()).rejects.toThrow('nope'); + expect(startupDidFail.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + Object { + "error": [Error: nope], + }, + ], + ] + `); }); it('stop throws on stop error', async () => { @@ -176,7 +183,7 @@ describe('ApolloServer start', () => { // We care more specifically about this in the background start case because // integrations shouldn't call executeHTTPGraphQLRequest if a "foreground" // start fails. - it('executeHTTPGraphQLRequest throws redacted message if background start fails', async () => { + it('executeHTTPGraphQLRequest returns redacted error if background start fails', async () => { const error = jest.fn(); const logger = { debug: jest.fn(), @@ -191,21 +198,44 @@ describe('ApolloServer start', () => { plugins: [failToStartPlugin], logger, }); + server.startInBackgroundHandlingStartupErrorsByLoggingAndFailingAllRequests(); - for (const _ in [1, 2]) { - await expect( - server.executeHTTPGraphQLRequest({ - httpGraphQLRequest: { - method: 'POST', - headers: new HeaderMap([['content-type', 'application-json']]), - body: JSON.stringify({ query: '{__typename}' }), - searchParams: {}, - }, - context: async () => ({}), - }), - ).rejects.toThrow(redactedMessage); - } + const request = { + httpGraphQLRequest: { + method: 'POST', + headers: new HeaderMap([['content-type', 'application-json']]), + body: JSON.stringify({ query: '{__typename}' }), + searchParams: {}, + }, + context: async () => ({}), + }; + + expect(await server.executeHTTPGraphQLRequest(request)) + .toMatchInlineSnapshot(` + Object { + "bodyChunks": null, + "completeBody": "{\\"errors\\":[{\\"message\\":\\"This data graph is missing a valid configuration. More details may be available in the server logs.\\",\\"extensions\\":{\\"code\\":\\"INTERNAL_SERVER_ERROR\\"}}]} + ", + "headers": Map { + "content-type" => "application/json", + }, + "statusCode": 500, + } + `); + + expect(await server.executeHTTPGraphQLRequest(request)) + .toMatchInlineSnapshot(` + Object { + "bodyChunks": null, + "completeBody": "{\\"errors\\":[{\\"message\\":\\"This data graph is missing a valid configuration. More details may be available in the server logs.\\",\\"extensions\\":{\\"code\\":\\"INTERNAL_SERVER_ERROR\\"}}]} + ", + "headers": Map { + "content-type" => "application/json", + }, + "statusCode": 500, + } + `); // Three times: once for the actual background _start call, twice for the // two operations. diff --git a/packages/server/src/__tests__/integration/apolloServerTests.ts b/packages/server/src/__tests__/integration/apolloServerTests.ts index 7379cd29ab2..036a6fd6cd4 100644 --- a/packages/server/src/__tests__/integration/apolloServerTests.ts +++ b/packages/server/src/__tests__/integration/apolloServerTests.ts @@ -58,10 +58,20 @@ import type { CreateServerForIntegrationTestsResult, } from '.'; import type { SchemaLoadOrUpdateCallback } from '../../types'; +import type { Logger } from '@apollo/utils.logger'; const quietLogger = loglevel.getLogger('quiet'); quietLogger.setLevel(loglevel.levels.WARN); +function mockLogger(): Logger { + return { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; +} + const INTROSPECTION_QUERY = ` { __schema { @@ -480,13 +490,7 @@ export function defineIntegrationTestSuiteApolloServerTests( // startInBackgroundHandlingStartupErrorsByLoggingAndFailingAllRequests) // but actual operations will fail, like the function name says. // (But the error it throws should be masked.) - const error = jest.fn(); - const logger = { - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error, - }; + const logger = mockLogger(); const url = await createServerAndGetUrl({ gateway, logger }); // We don't need to call stop() on the server since it fails to start. serverToCleanUp = null; @@ -503,11 +507,13 @@ export function defineIntegrationTestSuiteApolloServerTests( // The error is logged once immediately when startup fails, and // again during the request. - expect(error).toHaveBeenCalledTimes(2); - expect(error.mock.calls[0][0]).toBe( + expect(logger.error).toHaveBeenCalledTimes(2); + expect(logger.error).toHaveBeenNthCalledWith( + 1, `An error occurred during Apollo Server startup. All GraphQL requests will now fail. The startup error was: ${loadError.message}`, ); - expect(error.mock.calls[1][0]).toBe( + expect(logger.error).toHaveBeenNthCalledWith( + 2, `An error occurred during Apollo Server startup. All GraphQL requests will now fail. The startup error was: ${loadError.message}`, ); } else { @@ -766,6 +772,8 @@ export function defineIntegrationTestSuiteApolloServerTests( describe('formatError', () => { it('wraps thrown error from validation rules', async () => { + const logger = mockLogger(); + const throwError = jest.fn(() => { throw new Error('nope'); }); @@ -775,28 +783,63 @@ export function defineIntegrationTestSuiteApolloServerTests( return error; }); - const uri = await createServerAndGetUrl({ + const url = await createServerAndGetUrl({ schema, validationRules: [throwError], introspection: true, formatError, + logger, }); - const apolloFetch = createApolloFetch({ uri }); - - const introspectionResult = await apolloFetch({ - query: INTROSPECTION_QUERY, - }); - expect(introspectionResult.data).toBeUndefined(); - expect(introspectionResult.errors).toBeDefined(); - expect(formatError).toHaveBeenCalledTimes(1); - expect(throwError).toHaveBeenCalledTimes(1); + { + const res = await request(url) + .post('/') + .send({ query: INTROSPECTION_QUERY }); + expect(res.status).toBe(500); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + }, + "message": "Internal server error", + }, + ], + } + `); + expect(formatError).toHaveBeenCalledTimes(1); + expect(throwError).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenLastCalledWith( + 'Unexpected error processing request: Error: nope', + ); + } - const result = await apolloFetch({ query: TEST_STRING_QUERY }); - expect(result.data).toBeUndefined(); - expect(result.errors).toBeDefined(); - expect(formatError).toHaveBeenCalledTimes(2); - expect(throwError).toHaveBeenCalledTimes(2); + { + const res = await request(url) + .post('/') + .send({ query: TEST_STRING_QUERY }); + expect(res.status).toBe(500); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + }, + "message": "Internal server error", + }, + ], + } + `); + expect(formatError).toHaveBeenCalledTimes(2); + expect(throwError).toHaveBeenCalledTimes(2); + expect(logger.error).toHaveBeenCalledTimes(2); + expect(logger.error).toHaveBeenLastCalledWith( + 'Unexpected error processing request: Error: nope', + ); + } }); it('works with errors similar to GraphQL errors, such as yup', async () => { @@ -1439,7 +1482,9 @@ export function defineIntegrationTestSuiteApolloServerTests( throw pluginError; }); const formatError = jest.fn((formattedError, error) => { - expect(error).toEqual(pluginError); + // Errors thrown by plugins are generally replaced with "Internal + // server error" and logged. + expect(error.message).toBe('Internal server error'); // extension should be called before formatError expect(pluginCalled).toHaveBeenCalledTimes(1); @@ -1448,6 +1493,8 @@ export function defineIntegrationTestSuiteApolloServerTests( message: 'masked', }; }); + const logger = mockLogger(); + const unexpectedErrorProcessingRequest = jest.fn(); const uri = await createServerAndGetUrl({ typeDefs: gql` type Query { @@ -1459,6 +1506,7 @@ export function defineIntegrationTestSuiteApolloServerTests( fieldWhichWillError: () => {}, }, }, + logger, plugins: [ { async requestDidStart() { @@ -1470,6 +1518,7 @@ export function defineIntegrationTestSuiteApolloServerTests( }, }; }, + unexpectedErrorProcessingRequest, }, ], formatError, @@ -1483,6 +1532,14 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(result.errors).toBeDefined(); expect(result.errors[0].message).toEqual('masked'); expect(formatError).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + 'Unexpected error processing request: Error: nope', + ); + expect(unexpectedErrorProcessingRequest).toHaveBeenCalledTimes(1); + expect( + unexpectedErrorProcessingRequest.mock.calls[0][0].error, + ).toMatchInlineSnapshot(`[Error: nope]`); }); describe('context field', () => { @@ -1665,12 +1722,14 @@ export function defineIntegrationTestSuiteApolloServerTests( }, }, }; + const contextCreationDidFail = jest.fn(); const uri = await createServerAndGetUrl( { typeDefs, resolvers, stopOnTerminationSignals: false, nodeEnv: '', + plugins: [{ contextCreationDidFail }], }, { context: async () => { @@ -1690,6 +1749,16 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(e.extensions).toBeDefined(); expect(e.extensions.code).toEqual('UNAUTHENTICATED'); expect(e.extensions.exception.stacktrace).toBeDefined(); + + expect(contextCreationDidFail.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + Object { + "error": [AuthenticationError: Context creation failed: valid result], + }, + ], + ] + `); }); }); @@ -2658,6 +2727,8 @@ export function defineIntegrationTestSuiteApolloServerTests( }); describe('CSRF prevention', () => { + const invalidRequestErrors: Error[] = []; + async function makeServer( csrfPrevention?: ApolloServerOptions['csrfPrevention'], ): Promise { @@ -2666,6 +2737,13 @@ export function defineIntegrationTestSuiteApolloServerTests( typeDefs: 'type Query { x: ID }', resolvers: { Query: { x: () => 'foo' } }, csrfPrevention, + plugins: [ + { + async invalidRequestWasReceived({ error }) { + invalidRequestErrors.push(error); + }, + }, + ], }) ).url; } @@ -2675,11 +2753,16 @@ export function defineIntegrationTestSuiteApolloServerTests( function succeeds(res: Response) { expect(res.status).toBe(200); expect(res.body).toEqual(response); + expect(invalidRequestErrors).toHaveLength(0); } function blocked(res: Response) { expect(res.status).toBe(400); expect(res.text).toMatch(/This operation has been blocked/); + expect(invalidRequestErrors).toHaveLength(1); + expect(invalidRequestErrors.pop()?.message).toMatch( + /This operation has been blocked/, + ); } it('default', async () => { diff --git a/packages/server/src/__tests__/integration/httpServerTests.ts b/packages/server/src/__tests__/integration/httpServerTests.ts index a79164f5f22..a50886ad829 100644 --- a/packages/server/src/__tests__/integration/httpServerTests.ts +++ b/packages/server/src/__tests__/integration/httpServerTests.ts @@ -217,7 +217,7 @@ export function defineIntegrationTestSuiteHttpServerTests( serverIsStartedInBackground?: boolean; } = {}, ) { - describe('httpServerLevelTests.ts', () => { + describe('httpServerTests.ts', () => { let didEncounterErrors: jest.MockedFunction< NonNullable['didEncounterErrors']> >; @@ -250,6 +250,7 @@ export function defineIntegrationTestSuiteHttpServerTests( afterEach(stopServer); describe('graphqlHTTP', () => { + // In AS3 we did a 405 with `allow: GET, POST` instead; it was mild it('rejects the request if the method is not POST or GET', async () => { const app = await createApp(); const req = request(app) @@ -413,16 +414,26 @@ export function defineIntegrationTestSuiteHttpServerTests( await req.then((res) => { expect(res.status).toEqual(405); expect(res.headers['allow']).toEqual('POST'); - expect((res.error as HTTPError).text).toMatch( - 'GET supports only query operation', - ); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "BAD_REQUEST", + }, + "message": "GET requests only support query operations, not mutation operations", + }, + ], + } + `); }); expect(didEncounterErrors).toBeCalledWith( expect.objectContaining({ errors: expect.arrayContaining([ expect.objectContaining({ - message: 'GET supports only query operation', + message: + 'GET requests only support query operations, not mutation operations', }), ]), }), @@ -459,16 +470,26 @@ export function defineIntegrationTestSuiteHttpServerTests( await req.then((res) => { expect(res.status).toEqual(405); expect(res.headers['allow']).toEqual('POST'); - expect((res.error as HTTPError).text).toMatch( - 'GET supports only query operation', - ); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "BAD_REQUEST", + }, + "message": "GET requests only support query operations, not mutation operations", + }, + ], + } + `); }); expect(didEncounterErrors).toBeCalledWith( expect.objectContaining({ errors: expect.arrayContaining([ expect.objectContaining({ - message: 'GET supports only query operation', + message: + 'GET requests only support query operations, not mutation operations', }), ]), }), @@ -789,9 +810,18 @@ export function defineIntegrationTestSuiteHttpServerTests( variables: '{ "echo": "world" }', }); expect(res.status).toEqual(400); - expect((res.error as HTTPError).text).toMatchInlineSnapshot( - `"\`variables\` in a POST body should be provided as an object, not a recursively JSON-encoded string."`, - ); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "BAD_REQUEST", + }, + "message": "\`variables\` in a POST body should be provided as an object, not a recursively JSON-encoded string.", + }, + ], + } + `); }); it('POST does not handle a request with extensions as string', async () => { @@ -801,9 +831,18 @@ export function defineIntegrationTestSuiteHttpServerTests( extensions: '{ "echo": "world" }', }); expect(res.status).toEqual(400); - expect((res.error as HTTPError).text).toMatchInlineSnapshot( - `"\`extensions\` in a POST body should be provided as an object, not a recursively JSON-encoded string."`, - ); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "BAD_REQUEST", + }, + "message": "\`extensions\` in a POST body should be provided as an object, not a recursively JSON-encoded string.", + }, + ], + } + `); }); it('can handle a request with operationName', async () => { @@ -986,9 +1025,18 @@ export function defineIntegrationTestSuiteHttpServerTests( ]); expect(res.status).toEqual(400); - expect((res.error as HTTPError).text).toMatchInlineSnapshot( - `"Operation batching disabled."`, - ); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "BAD_REQUEST", + }, + "message": "Operation batching disabled.", + }, + ], + } + `); }); it('clones batch context', async () => { diff --git a/packages/server/src/errors.ts b/packages/server/src/errors.ts index b89d0340150..7883d049eab 100644 --- a/packages/server/src/errors.ts +++ b/packages/server/src/errors.ts @@ -93,8 +93,10 @@ export class UserInputError extends GraphQLError { // document it, and maybe consider using it for more of the errors in // runHttpQuery instead of just returning text/plain errors. export class BadRequestError extends GraphQLError { - constructor(message: string, extensions?: Record) { - super(message, { extensions: { ...extensions, code: 'BAD_REQUEST' } }); + constructor(message: string, options?: { extensions?: Record }) { + super(message, { + extensions: { ...options?.extensions, code: 'BAD_REQUEST' }, + }); this.name = 'BadRequestError'; } @@ -154,10 +156,7 @@ export function formatApolloErrors( }); function enrichError(maybeError: unknown): GraphQLFormattedError { - const error: Error = - maybeError instanceof Error - ? maybeError - : new GraphQLError('Unexpected error value: ' + String(maybeError)); + const error: Error = ensureError(maybeError); const graphqlError: GraphQLError = error instanceof GraphQLError @@ -194,3 +193,9 @@ export function formatApolloErrors( return { ...graphqlError.toJSON(), extensions }; } } + +export function ensureError(maybeError: unknown): Error { + return maybeError instanceof Error + ? maybeError + : new GraphQLError('Unexpected error value: ' + String(maybeError)); +} diff --git a/packages/server/src/externalTypes/plugins.ts b/packages/server/src/externalTypes/plugins.ts index 9d3daa6af89..5cda2e7a57c 100644 --- a/packages/server/src/externalTypes/plugins.ts +++ b/packages/server/src/externalTypes/plugins.ts @@ -45,6 +45,18 @@ export interface ApolloServerPlugin { requestContext: GraphQLRequestContext, ): Promise | void>; + // TODO(AS4): Document all these. + unexpectedErrorProcessingRequest?({ + requestContext, + error, + }: { + requestContext: GraphQLRequestContext; + error: Error; + }): Promise; + contextCreationDidFail?({ error }: { error: Error }): Promise; + invalidRequestWasReceived?({ error }: { error: Error }): Promise; + startupDidFail?({ error }: { error: Error }): Promise; + // See the similarly named field on ApolloServer for details. Note that it // appears that this only works if it is a *field*, not a *method*, which is // why `requestDidStart` (which takes a TContext wrapped in something) is not diff --git a/packages/server/src/httpBatching.ts b/packages/server/src/httpBatching.ts index bb0b504ae36..11a4bc7953f 100644 --- a/packages/server/src/httpBatching.ts +++ b/packages/server/src/httpBatching.ts @@ -4,7 +4,8 @@ import type { HTTPGraphQLResponse, } from './externalTypes'; import type { ApolloServerInternals, SchemaDerivedData } from './ApolloServer'; -import { HeaderMap, HttpQueryError, runHttpQuery } from './runHttpQuery'; +import { HeaderMap, runHttpQuery } from './runHttpQuery'; +import { BadRequestError } from './errors'; export async function runBatchHttpQuery( batchRequest: Omit & { body: any[] }, @@ -78,8 +79,5 @@ export async function runPotentiallyBatchedHttpQuery< internals, ); } - return new HttpQueryError( - 400, - 'Operation batching disabled.', - ).asHTTPGraphQLResponse(); + throw new BadRequestError('Operation batching disabled.'); } diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index be72984eb84..218ece780bb 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -1,7 +1,5 @@ // TODO(AS4): Evaluate the full exposed API. -export { HttpQueryError } from './runHttpQuery'; - export { SyntaxError, ValidationError, diff --git a/packages/server/src/preventCsrf.ts b/packages/server/src/preventCsrf.ts index 00e4a2f18e9..9139b3eed1b 100644 --- a/packages/server/src/preventCsrf.ts +++ b/packages/server/src/preventCsrf.ts @@ -1,5 +1,5 @@ import MIMEType from 'whatwg-mimetype'; -import { HttpQueryError } from './runHttpQuery'; +import { BadRequestError } from './errors'; // Our recommended set of CSRF prevention headers. Operations that do not // provide a content-type such as `application/json` (in practice, this @@ -87,8 +87,7 @@ export function preventCsrf( return; } - throw new HttpQueryError( - 400, + throw new BadRequestError( `This operation has been blocked as a potential Cross-Site Request Forgery ` + `(CSRF). Please either specify a 'content-type' header (with a type that ` + `is not one of ${NON_PREFLIGHTED_CONTENT_TYPES.join(', ')}) or provide ` + diff --git a/packages/server/src/requestPipeline.ts b/packages/server/src/requestPipeline.ts index ff02e4bf323..27252260dc1 100644 --- a/packages/server/src/requestPipeline.ts +++ b/packages/server/src/requestPipeline.ts @@ -267,9 +267,15 @@ export async function processGraphQLRequest( // it's generally how HTTP requests should work, and additionally it makes us // less vulnerable to mutations running over CSRF, if you turn off our CSRF // prevention feature.) - if (request.http?.method === 'GET' && operation?.operation !== 'query') { + if ( + request.http?.method === 'GET' && + operation?.operation && + operation.operation !== 'query' + ) { return await sendErrorResponse( - new GraphQLError('GET supports only query operation'), + new BadRequestError( + `GET requests only support query operations, not ${operation.operation} operations`, + ), undefined, { statusCode: 405, headers: new HeaderMap([['allow', 'POST']]) }, ); diff --git a/packages/server/src/runHttpQuery.ts b/packages/server/src/runHttpQuery.ts index 8a4c1db8c3d..ad71419eb47 100644 --- a/packages/server/src/runHttpQuery.ts +++ b/packages/server/src/runHttpQuery.ts @@ -1,4 +1,3 @@ -import { formatApolloErrors } from './errors'; import type { BaseContext, GraphQLRequest, @@ -12,6 +11,7 @@ import { } from './ApolloServer'; import type { FormattedExecutionResult } from 'graphql'; import type { HTTPGraphQLHead } from './externalTypes/http'; +import { BadRequestError } from './errors'; // TODO(AS4): keep rethinking whether Map is what we want or if we just // do want to use (our own? somebody else's?) Headers class. @@ -26,41 +26,6 @@ export class HeaderMap extends Map { } } -export class HttpQueryError extends Error { - public statusCode: number; - // TODO(AS4): consider making this a map (or whatever type we settle on - // for headers) - public headers: Map; - - constructor( - statusCode: number, - message: string, - headers?: Map, - ) { - super(message); - this.name = 'HttpQueryError'; - this.statusCode = statusCode; - // This throws if any header names have capital leaders. - this.headers = new HeaderMap(headers ?? []); - } - - // TODO(AS4): Do we really want any text/plain errors, or - // or should we unify error handling so that every single error - // works the same way (JSON + sent through the plugin system)? - asHTTPGraphQLResponse(): HTTPGraphQLResponse { - return { - statusCode: this.statusCode, - // Copy to HeaderMap to ensure lower-case keys. - headers: new HeaderMap([ - ['content-type', 'text/plain'], - ...this.headers.entries(), - ]), - completeBody: this.message, - bodyChunks: null, - }; - } -} - function fieldIfString( o: Record, fieldName: string, @@ -80,14 +45,12 @@ function jsonParsedFieldIfNonEmptyString( try { hopefullyRecord = JSON.parse(o[fieldName]); } catch { - throw new HttpQueryError( - 400, + throw new BadRequestError( `The ${fieldName} search parameter contains invalid JSON.`, ); } if (!isStringRecord(hopefullyRecord)) { - throw new HttpQueryError( - 400, + throw new BadRequestError( `The ${fieldName} search parameter should contain a JSON-encoded object.`, ); } @@ -120,8 +83,7 @@ function ensureQueryIsStringOrMissing(query: any) { } // Check for a common error first. if (query.kind === 'Document') { - throw new HttpQueryError( - 400, + throw new BadRequestError( "GraphQL queries must be strings. It looks like you're sending the " + 'internal graphql-js representation of a parsed query in your ' + 'request instead of a request in the GraphQL query language. You ' + @@ -130,132 +92,102 @@ function ensureQueryIsStringOrMissing(query: any) { 'the internal representation to a string for you.', ); } else { - throw new HttpQueryError(400, 'GraphQL queries must be strings.'); + throw new BadRequestError('GraphQL queries must be strings.'); } } -// This function should not throw. +export const badMethodErrorMessage = + 'Apollo Server supports only GET/POST requests.'; + export async function runHttpQuery( httpRequest: HTTPGraphQLRequest, contextValue: TContext, schemaDerivedData: SchemaDerivedData, internals: ApolloServerInternals, ): Promise { - try { - let graphQLRequest: GraphQLRequest; - - switch (httpRequest.method) { - case 'POST': - // TODO(AS4): If it's an array, some error about enabling batching? - if (!isNonEmptyStringRecord(httpRequest.body)) { - return new HttpQueryError( - 400, - 'POST body missing, invalid Content-Type, or JSON object has no keys.', - ).asHTTPGraphQLResponse(); - } - - ensureQueryIsStringOrMissing(httpRequest.body.query); - - if (typeof httpRequest.body.variables === 'string') { - // TODO(AS4): make sure we note this change in migration - return new HttpQueryError( - 400, - '`variables` in a POST body should be provided as an object, not a recursively JSON-encoded string.', - ).asHTTPGraphQLResponse(); - } - - if (typeof httpRequest.body.extensions === 'string') { - // TODO(AS4): make sure we note this change in migration - return new HttpQueryError( - 400, - '`extensions` in a POST body should be provided as an object, not a recursively JSON-encoded string.', - ).asHTTPGraphQLResponse(); - } - - graphQLRequest = { - query: fieldIfString(httpRequest.body, 'query'), - operationName: fieldIfString(httpRequest.body, 'operationName'), - variables: fieldIfRecord(httpRequest.body, 'variables'), - extensions: fieldIfRecord(httpRequest.body, 'extensions'), - http: httpRequest, - }; - - break; - case 'GET': - if (!isStringRecord(httpRequest.searchParams)) { - return new HttpQueryError( - 400, - 'GET query missing.', - ).asHTTPGraphQLResponse(); - } - - ensureQueryIsStringOrMissing(httpRequest.searchParams.query); - - graphQLRequest = { - query: fieldIfString(httpRequest.searchParams, 'query'), - operationName: fieldIfString( - httpRequest.searchParams, - 'operationName', - ), - variables: jsonParsedFieldIfNonEmptyString( - httpRequest.searchParams, - 'variables', - ), - extensions: jsonParsedFieldIfNonEmptyString( - httpRequest.searchParams, - 'extensions', - ), - http: httpRequest, - }; - - break; - default: - return new HttpQueryError( - 405, - 'Apollo Server supports only GET/POST requests.', - new HeaderMap([['allow', 'GET, POST']]), - ).asHTTPGraphQLResponse(); - } - - const graphQLResponse = await internalExecuteOperation({ - graphQLRequest, - contextValue, - internals, - schemaDerivedData, - }); + let graphQLRequest: GraphQLRequest; + + switch (httpRequest.method) { + case 'POST': + // TODO(AS4): If it's an array, some error about enabling batching? + if (!isNonEmptyStringRecord(httpRequest.body)) { + throw new BadRequestError( + 'POST body missing, invalid Content-Type, or JSON object has no keys.', + ); + } + + ensureQueryIsStringOrMissing(httpRequest.body.query); + + if (typeof httpRequest.body.variables === 'string') { + // TODO(AS4): make sure we note this change in migration + throw new BadRequestError( + '`variables` in a POST body should be provided as an object, not a recursively JSON-encoded string.', + ); + } + + if (typeof httpRequest.body.extensions === 'string') { + // TODO(AS4): make sure we note this change in migration + throw new BadRequestError( + '`extensions` in a POST body should be provided as an object, not a recursively JSON-encoded string.', + ); + } + + graphQLRequest = { + query: fieldIfString(httpRequest.body, 'query'), + operationName: fieldIfString(httpRequest.body, 'operationName'), + variables: fieldIfRecord(httpRequest.body, 'variables'), + extensions: fieldIfRecord(httpRequest.body, 'extensions'), + http: httpRequest, + }; + + break; + case 'GET': + if (!isStringRecord(httpRequest.searchParams)) { + throw new BadRequestError('GET query missing.'); + } + + ensureQueryIsStringOrMissing(httpRequest.searchParams.query); + + graphQLRequest = { + query: fieldIfString(httpRequest.searchParams, 'query'), + operationName: fieldIfString(httpRequest.searchParams, 'operationName'), + variables: jsonParsedFieldIfNonEmptyString( + httpRequest.searchParams, + 'variables', + ), + extensions: jsonParsedFieldIfNonEmptyString( + httpRequest.searchParams, + 'extensions', + ), + http: httpRequest, + }; + + break; + default: + throw new BadRequestError(badMethodErrorMessage); + } - const body = prettyJSONStringify( - orderExecutionResultFields(graphQLResponse.result), - ); + const graphQLResponse = await internalExecuteOperation({ + graphQLRequest, + contextValue, + internals, + schemaDerivedData, + }); - graphQLResponse.http.headers.set( - 'content-length', - Buffer.byteLength(body, 'utf8').toString(), - ); + const body = prettyJSONStringify( + orderExecutionResultFields(graphQLResponse.result), + ); - return { - ...graphQLResponse.http, - completeBody: body, - bodyChunks: null, - }; - } catch (error) { - if (error instanceof HttpQueryError) { - return error.asHTTPGraphQLResponse(); - } + graphQLResponse.http.headers.set( + 'content-length', + Buffer.byteLength(body, 'utf8').toString(), + ); - return { - statusCode: 500, - headers: new HeaderMap([['content-type', 'application/json']]), - completeBody: prettyJSONStringify({ - errors: formatApolloErrors([error as Error], { - includeStackTracesInErrorResponses: - internals.includeStackTracesInErrorResponses, - formatError: internals.formatError, - }), - }), - bodyChunks: null, - }; - } + return { + ...graphQLResponse.http, + completeBody: body, + bodyChunks: null, + }; } function orderExecutionResultFields(