diff --git a/.changeset/purple-paws-yell.md b/.changeset/purple-paws-yell.md new file mode 100644 index 00000000000..86ed8997dc8 --- /dev/null +++ b/.changeset/purple-paws-yell.md @@ -0,0 +1,18 @@ +--- +'@apollo/server': minor +--- + +Introduce new opt-in configuration option to mitigate v4 status code regression + +Apollo Server v4 accidentally started responding to requests with an invalid `variables` object with a 200 status code, where v3 previously responded with a 400. In order to not break current behavior (potentially breaking users who have creatively worked around this issue) and offer a mitigation, we've added the following configuration option which we recommend for all users. + +```ts +new ApolloServer({ + // ... + status400ForVariableCoercionErrors: true, +}); +``` + +Specifically, this regression affects cases where _input variable coercion_ fails. Variables of an incorrect type (i.e. `String` instead of `Int`) or unexpectedly `null` are examples that fail variable coercion. Additionally, missing or incorrect fields on input objects as well as custom scalars that throw during validation will also fail variable coercion. For more specifics on variable coercion, see the "Input Coercion" sections in the [GraphQL spec](https://spec.graphql.org/June2018/#sec-Scalars). + +This will become the default behavior in Apollo Server v5 and the configuration option will be ignored / no longer needed. diff --git a/docs/source/api/apollo-server.mdx b/docs/source/api/apollo-server.mdx index d0d65be1455..80ee4848edf 100644 --- a/docs/source/api/apollo-server.mdx +++ b/docs/source/api/apollo-server.mdx @@ -475,6 +475,23 @@ If this is set to any string value, use that value instead of the environment va + + + +###### `status400ForVariableCoercionErrors` + +`boolean` + + + + +**Set this option to `true`.** It mitigates a regression introduced in Apollo Server v4 where the server returns a 200 status code (instead of 400) when a client query provides invalid variables. [Learn more.](../migration/#known-regressions) + +Apollo Server v5 will _always_ behave as if this option is `true` (and will ignore any provided value). + + + + diff --git a/docs/source/data/errors.mdx b/docs/source/data/errors.mdx index 58afeaeb90d..f7df300ab25 100644 --- a/docs/source/data/errors.mdx +++ b/docs/source/data/errors.mdx @@ -5,6 +5,8 @@ description: Making errors actionable on the client and server import TopLevelAwait from "../shared/top-level-await.mdx" +> Apollo Server v4 introduced a regression where providing invalid variables yields a 200 status code instead of 400. To mitigate this regression, provide the `status400ForVariableCoercionErrors: true` option to your `ApolloServer` constructor. For more information, see the [migration guide](../migration/#known-regressions). + Whenever Apollo Server encounters errors while processing a GraphQL operation, its response to the client includes an `errors` array containing each error that occurred. Each error in the array has an `extensions` field that provides additional useful information, including an error `code` and (while in development mode) a `stacktrace`. diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index fc00337b83b..de6b893d424 100644 --- a/docs/source/migration.mdx +++ b/docs/source/migration.mdx @@ -344,6 +344,29 @@ You can also import each plugin's associated TypeScript types (e.g., `ApolloServ Once you've updated your imports, you can remove your project's dependency on `apollo-server-core`. +## Known regressions + +### Appropriate 400 status codes + +Apollo Server v4 responds to an invalid `variables` object with a 200 status code, whereas v3 responds appropriately with a 400 status code. This regression was introduced in [PR #6502](https://github.com/apollographql/apollo-server/pull/6502) and brought to our attention in [Issue #7462](https://github.com/apollographql/apollo-server/issues/7462). + +Specifically, this regression affects cases where _input variable coercion_ fails. Variables of an incorrect type (i.e. `String` instead of `Int`) or unexpectedly `null` are examples that fail variable coercion. Additionally, missing or incorrect fields on input objects as well as custom scalars that throw during validation will also fail variable coercion. For additional specifics on variable coercion, see the "Input Coercion" sections in the [GraphQL spec](https://spec.graphql.org/June2018/#sec-Scalars). + +We recommend mitigating this regression unless you've already modified your application to work around it. To do so, add the `status400ForVariableCoercionErrors: true` option to your `ApolloServer` constructor: + + + +```ts +new ApolloServer({ + // ... + status400ForVariableCoercionErrors: true, +}) +``` + + + +This option will no longer be needed (and will be ignored) in Apollo Server v5. + ## Bumped dependencies ### Node.js diff --git a/packages/server/src/ApolloServer.ts b/packages/server/src/ApolloServer.ts index 64ab795d685..c77b53b1ad5 100644 --- a/packages/server/src/ApolloServer.ts +++ b/packages/server/src/ApolloServer.ts @@ -174,7 +174,9 @@ export interface ApolloServerInternals { rootValue?: ((parsedQuery: DocumentNode) => unknown) | unknown; validationRules: Array; fieldResolver?: GraphQLFieldResolver; - + // TODO(AS5): remove OR warn + ignore with this option set, ignore option and + // flip default behavior. + status400ForVariableCoercionErrors?: boolean; __testing_incrementalExecutionResults?: GraphQLExperimentalIncrementalExecutionResults; } @@ -325,6 +327,8 @@ export class ApolloServer { ? null : config.csrfPrevention.requestHeaders ?? recommendedCsrfPreventionRequestHeaders, + status400ForVariableCoercionErrors: + config.status400ForVariableCoercionErrors ?? false, __testing_incrementalExecutionResults: config.__testing_incrementalExecutionResults, }; diff --git a/packages/server/src/__tests__/ApolloServer.test.ts b/packages/server/src/__tests__/ApolloServer.test.ts index dd113979e77..0656e902644 100644 --- a/packages/server/src/__tests__/ApolloServer.test.ts +++ b/packages/server/src/__tests__/ApolloServer.test.ts @@ -23,6 +23,17 @@ const typeDefs = gql` hello: String error: Boolean contextFoo: String + needsStringArg(aString: String): String + needsCompoundArg(aCompound: CompoundInput): String + } + + input CompoundInput { + compound: NestedInput! + } + + input NestedInput { + nested1: String! + nested2: String! } `; @@ -378,6 +389,61 @@ describe('ApolloServer executeOperation', () => { await server.stop(); }); + // TODO(AS5): expect an update here when default flips + it.each([ + { status400ForVariableCoercionErrors: false, expectedStatus: undefined }, + { status400ForVariableCoercionErrors: true, expectedStatus: 400 }, + ])( + 'variable coercion errors', + async ({ status400ForVariableCoercionErrors, expectedStatus }) => { + const server = new ApolloServer({ + typeDefs, + resolvers, + status400ForVariableCoercionErrors, + }); + await server.start(); + + const { body, http } = await server.executeOperation({ + query: 'query NeedsArg($arg: String) { needsStringArg(aString: $arg) }', + variables: { arg: 1 }, + }); + const result = singleResult(body); + expect(result.errors?.[0].extensions?.code).toBe('BAD_USER_INPUT'); + expect(http.status).toBe(expectedStatus); + await server.stop(); + }, + ); + + // CompoundInput is { compound: { nested1: String!, nested2: String! }! } + // absence, null, and non-string values will all cause coercion errors + it.each([ + { arg: { compound: { nested1: 'abc' } } }, + { arg: { compound: { nested1: 'abc', nested2: null } } }, + { arg: { compound: { nested1: 'abc', nested2: 123 } } }, + {}, + null, + undefined, + ])('variable coercion errors, additional examples: %s', async (variables) => { + const server = new ApolloServer({ + typeDefs, + resolvers, + status400ForVariableCoercionErrors: true, + }); + await server.start(); + + const { body, http } = await server.executeOperation({ + query: `#graphql + query NeedsArg($arg: CompoundInput!) { needsCompoundArg(aCompound: $arg) } + `, + // @ts-expect-error for `null` case + variables, + }); + const result = singleResult(body); + expect(result.errors?.[0].extensions?.code).toBe('BAD_USER_INPUT'); + expect(http.status).toBe(400); + await server.stop(); + }); + it('passes its second argument as context object', async () => { const server = new ApolloServer({ typeDefs, diff --git a/packages/server/src/externalTypes/constructor.ts b/packages/server/src/externalTypes/constructor.ts index 3f012f214a1..7ab4addf8ef 100644 --- a/packages/server/src/externalTypes/constructor.ts +++ b/packages/server/src/externalTypes/constructor.ts @@ -102,6 +102,15 @@ interface ApolloServerOptionsBase { // parsing the schema. parseOptions?: ParseOptions; + // TODO(AS5): remove OR warn + ignore with this option set, ignore option and + // flip default behavior. Default false. This opt-in configuration fixes a + // regression introduced in v4. In v3, Apollo Server would correctly respond + // to a request with invalid `variables` with a 400 status code. AS4 responds + // with a 200 status code by default. We recommend setting this to `true` + // unless you've explicitly worked around this regression already (and maybe + // consider undoing the workaround). + status400ForVariableCoercionErrors?: boolean; + // For testing only. __testing_incrementalExecutionResults?: GraphQLExperimentalIncrementalExecutionResults; } diff --git a/packages/server/src/requestPipeline.ts b/packages/server/src/requestPipeline.ts index 663e08a37d4..fbd9014e634 100644 --- a/packages/server/src/requestPipeline.ts +++ b/packages/server/src/requestPipeline.ts @@ -474,6 +474,19 @@ export async function processGraphQLRequest( const { formattedErrors, httpFromErrors } = resultErrors ? formatErrors(resultErrors) : { formattedErrors: undefined, httpFromErrors: newHTTPGraphQLHead() }; + + // TODO(AS5) This becomes the default behavior and the + // `status400ForVariableCoercionErrors` configuration option is removed / + // ignored. + if ( + internals.status400ForVariableCoercionErrors && + resultErrors?.length && + result.data === undefined && + !httpFromErrors.status + ) { + httpFromErrors.status = 400; + } + mergeHTTPGraphQLHead(requestContext.response.http, httpFromErrors); if ('singleResult' in fullResult) {