From 551a5dde4f5b081caf69b57dda344de053008b19 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 6 Apr 2021 23:59:14 +0000 Subject: [PATCH 1/4] apollo-server-core: use UserInputError for variable coercion errors This particular error can be trivially triggered by clients, so it doesn't make sense to use `INTERNAL_SERVER_ERROR` for it. It seems like a good fit for `BAD_USER_INPUT`, which previously was only used if you explicitly throw a `UserInputError` in your app. Fixes #3498. --- .../apollo-server-core/src/requestPipeline.ts | 29 ++++++++++++++++--- packages/apollo-server-errors/src/index.ts | 6 +++- .../src/ApolloServer.ts | 21 ++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index d1a43dfdb9b..55da2cc086b 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -31,6 +31,7 @@ import { PersistedQueryNotSupportedError, PersistedQueryNotFoundError, formatApolloErrors, + UserInputError, } from 'apollo-server-errors'; import { GraphQLRequest, @@ -451,16 +452,36 @@ export async function processGraphQLRequest( requestContext as GraphQLRequestContextExecutionDidStart, ); - if (result.errors) { - await didEncounterErrors(result.errors); + // The first thing that execution does is coerce the request's variables + // to the types declared in the operation, which can lead to errors if + // they are of the wrong type. We change any such errors into + // UserInputError so that their code doesn't end up being + // INTERNAL_SERVER_ERROR, since these are client errors. + const resultErrors = result.errors?.map((e) => { + if ( + e.nodes?.length === 1 && + e.nodes[0].kind === 'VariableDefinition' && + e.message.startsWith( + `Variable "$${e.nodes[0].variable.name.value}" got invalid value `, + ) + ) { + return fromGraphQLError(e, { + errorConstructor: (message) => new UserInputError(message), + }); + } + return e; + }); + + if (resultErrors) { + await didEncounterErrors(resultErrors); } response = { ...result, - errors: result.errors ? formatErrors(result.errors) : undefined, + errors: resultErrors ? formatErrors(resultErrors) : undefined, }; - executionDispatcher.reverseInvokeHookSync("executionDidEnd"); + executionDispatcher.reverseInvokeHookSync('executionDidEnd'); } catch (executionError) { executionDispatcher.reverseInvokeHookSync("executionDidEnd", executionError); return await sendErrorResponse(executionError); diff --git a/packages/apollo-server-errors/src/index.ts b/packages/apollo-server-errors/src/index.ts index 8f2fb3ecb4f..702cafb1d64 100644 --- a/packages/apollo-server-errors/src/index.ts +++ b/packages/apollo-server-errors/src/index.ts @@ -134,12 +134,16 @@ export function toApolloError( export interface ErrorOptions { code?: string; + // Sometimes it's more convenient to pass a function than a class name. + errorConstructor?: (message: string) => ApolloError; errorClass?: typeof ApolloError; } export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) { const copy: ApolloError = - options && options.errorClass + options && options.errorConstructor + ? options.errorConstructor(error.message) + : options && options.errorClass ? new options.errorClass(error.message) : new ApolloError(error.message); diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index 706af759511..e7fa2a03c6e 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -348,6 +348,27 @@ export function testApolloServer( }); }); + it('variable coercion errors', async () => { + const { url: uri } = await createApolloServer({ + typeDefs: gql` + type Query { + hello(x: String): String + } + `, + }); + + const apolloFetch = createApolloFetch({ uri }); + + const result = await apolloFetch({ + query: `query ($x:String) {hello(x:$x)}`, + variables: { x: 2 }, + }); + expect(result.data).toBeUndefined(); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toMatch(/got invalid value 2; Expected type String/); + expect(result.errors[0].extensions.code).toBe('BAD_USER_INPUT'); + }); + describe('schema creation', () => { it('accepts typeDefs and resolvers', async () => { const typeDefs = gql` From 5cf5f8bb744b62e573d0a63ab884800788f63314 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 7 Apr 2021 00:09:54 +0000 Subject: [PATCH 2/4] figure out how to use the new constraint --- packages/apollo-server-core/src/requestPipeline.ts | 2 +- packages/apollo-server-errors/src/index.ts | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index 55da2cc086b..4c33d39f1b6 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -466,7 +466,7 @@ export async function processGraphQLRequest( ) ) { return fromGraphQLError(e, { - errorConstructor: (message) => new UserInputError(message), + errorClass: UserInputError, }); } return e; diff --git a/packages/apollo-server-errors/src/index.ts b/packages/apollo-server-errors/src/index.ts index 702cafb1d64..d88bbf03995 100644 --- a/packages/apollo-server-errors/src/index.ts +++ b/packages/apollo-server-errors/src/index.ts @@ -134,16 +134,12 @@ export function toApolloError( export interface ErrorOptions { code?: string; - // Sometimes it's more convenient to pass a function than a class name. - errorConstructor?: (message: string) => ApolloError; - errorClass?: typeof ApolloError; + errorClass?: new (message: string) => ApolloError; } export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) { const copy: ApolloError = - options && options.errorConstructor - ? options.errorConstructor(error.message) - : options && options.errorClass + options && options.errorClass ? new options.errorClass(error.message) : new ApolloError(error.message); From a8eac984b5cb894bb882701540747cc96c54babb Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 7 Apr 2021 07:58:51 -0700 Subject: [PATCH 3/4] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d57a2a020b7..63a45ca4b65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-server-cache-redis`: New `BaseRedisCache` class which takes an `ioredis`-compatible Redis client as an argument. The existing classes `RedisCache` and `RedisClusterCache` (which pass their arguments to `ioredis` constructors) are now implemented in terms of this class. This allows you to use any of the `ioredis` constructor forms rather than just the ones recognized by our classes. This also fixes a long-standing bug where the Redis cache implementations returned a number from `delete()`; it now returns a number, matching what the `KeyValueCache` interface and the TypeScript types expect. [PR #5034](https://github.com/apollographql/apollo-server/pull/5034) [PR #5088](https://github.com/apollographql/apollo-server/pull/5088) [Issue #4870](https://github.com/apollographql/apollo-server/issues/4870) [Issue #5006](https://github.com/apollographql/apollo-server/issues/5006) - `apollo-server-core`: Fix type for `formatResponse` function. It never is called with a `null` argument, and is allowed to return `null`. [Issue #5009](https://github.com/apollographql/apollo-server/issues/5009) [PR #5089](https://github.com/apollographql/apollo-server/pull/5089) - `apollo-server-lambda`: Fix regression in v2.21.2 where thrown errors were replaced by throwing the JS Error class itself. [PR #5085](https://github.com/apollographql/apollo-server/pull/5085) +- `apollo-server-core`: If a client sends a variable of the wrong type, this is now reported as an error with an `extensions.code` of `BAD_USER_INPUT` rather than `INTERNAL_SERVER_ERROR`. [PR #5091](https://github.com/apollographql/apollo-server/pull/5091) [Issue #3498](https://github.com/apollographql/apollo-server/issues/3498) ## v2.22.2 From 490337508dda2de82535d9cdf6d01700e154b994 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 8 Apr 2021 08:17:39 -0700 Subject: [PATCH 4/4] feedback --- packages/apollo-server-core/src/requestPipeline.ts | 3 ++- packages/apollo-server-errors/src/index.ts | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index 4c33d39f1b6..d448b4d505a 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -10,6 +10,7 @@ import { validate as graphqlValidate, parse as graphqlParse, execute as graphqlExecute, + Kind, } from 'graphql'; import { GraphQLExtension, @@ -460,7 +461,7 @@ export async function processGraphQLRequest( const resultErrors = result.errors?.map((e) => { if ( e.nodes?.length === 1 && - e.nodes[0].kind === 'VariableDefinition' && + e.nodes[0].kind === Kind.VARIABLE_DEFINITION && e.message.startsWith( `Variable "$${e.nodes[0].variable.name.value}" got invalid value `, ) diff --git a/packages/apollo-server-errors/src/index.ts b/packages/apollo-server-errors/src/index.ts index d88bbf03995..6eeea005bd4 100644 --- a/packages/apollo-server-errors/src/index.ts +++ b/packages/apollo-server-errors/src/index.ts @@ -134,6 +134,8 @@ export function toApolloError( export interface ErrorOptions { code?: string; + // This declaration means it takes any "class" that has a constructor that + // takes a single string, and should be invoked via the `new` operator. errorClass?: new (message: string) => ApolloError; }