fix: Introduce status code regression mitigation (#7465)

Apollo Server v4 introduced a regression with respect to invalid
`variables` and http status codes. AS4 incorrectly started responding
with a 200 status code, where AS3 would respond with a 400 when the
provided variables object failed variable coercion (during `graphql-js`
`execute`).

Providing the following config to your AS4 constructor options will
opt-in to the regression mitigation:
```ts
new ApolloServer({
  // ...
  status400WithErrorsAndNoData: true,
})
```

Fixes #7462
Related discussion #7460

---------

Co-authored-by: Stephen Barlow <stephen@apollographql.com>
Co-authored-by: David Glasser <glasser@apollographql.com>

.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.

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
 </td>
 </tr>
 
+<tr>
+<td>
+
+###### `status400ForVariableCoercionErrors`
+
+`boolean`
+
+</td>
+<td>
+
+**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).
+
+</td>
+</tr>
+
 </tbody>
 </table>
 

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).
+
 <!-- cSpell:ignore typenam -->
 
 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`.

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:
+
+<MultiCodeBlock>
+
+```ts
+new ApolloServer({
+  // ...
+  status400ForVariableCoercionErrors: true,
+})
+```
+
+</MultiCodeBlock>
+
+This option will no longer be needed (and will be ignored) in Apollo Server v5.
+
 ## Bumped dependencies
 
 ### Node.js

packages/server/src/ApolloServer.ts

@@ -174,7 +174,9 @@ export interface ApolloServerInternals<TContext extends BaseContext> {
   rootValue?: ((parsedQuery: DocumentNode) => unknown) | unknown;
   validationRules: Array<ValidationRule>;
   fieldResolver?: GraphQLFieldResolver<any, TContext>;
-
+  // 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<in out TContext extends BaseContext = BaseContext> {
           ? null
           : config.csrfPrevention.requestHeaders ??
             recommendedCsrfPreventionRequestHeaders,
+      status400ForVariableCoercionErrors:
+        config.status400ForVariableCoercionErrors ?? false,
       __testing_incrementalExecutionResults:
         config.__testing_incrementalExecutionResults,
     };

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,

packages/server/src/externalTypes/constructor.ts

@@ -102,6 +102,15 @@ interface ApolloServerOptionsBase<TContext extends BaseContext> {
   // 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;
 }

packages/server/src/requestPipeline.ts

@@ -474,6 +474,19 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
       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) {