Stricter JSON body validation; run graphql-over-HTTP test suite (#7171)

This PR adds the spec audit suite from `graphql-http` (designed to
validate compatibility with the "GraphQL over HTTP" specification) to
the integration test suite. It expects all required (MUST) audits to
pass, and for all optional (MAY/SHOULD) audits to pass that we haven't
explicitly excepted.

All required audits already passed. Failing optional audits fell into
three categories:

- Ignoring `operationName`, `variables`, and `extensions` when provided
with incorrect types instead of returning a 400 error. The SHOULD here
seemed reasonable and so this PR changes our POST handler to return 400s
if incorrect types are provided here. As described in the changeset,
this is backwards incompatible but we are comfortable making this choice
today.
- Returning 400 errors for various error conditions like GraphQL parse
errors, for the original `application/json` response type. The one
aspect of the GoH spec that is prescriptive rather than descriptive is
the invention of the `application/graphql-response+json` response MIME
type; the theory is that you can't really "trust" that a non-2xx
response with MIME type `application/json` is actually generated by the
GraphQL server rather than by some other proxy, so it has you use this
new type along with 4xx for these errors, and then SHOULDs that these
errors come with 200 for the original MIME type. The main reason that
this is a SHOULD is because many servers like Apollo Server already
return 400s in this case, and we're not interested in changing that. So
we ignore these particular errors.
- There's a sorta buggy audit that has opinions about how bad JSON
errors should look. See
graphql/graphql-http#25

Note that we deliberately make a strict dependency on a single version
of graphql-http, so that the tests run by a given version of
`@apollo/server-integration-test-suite` are well defined.

Fixes #7158.

Co-authored-by: Denis Badurina <badurinadenis@gmail.com>

.changeset/good-pets-begin.md

@@ -0,0 +1,10 @@
+---
+'@apollo/server-integration-testsuite': minor
+'@apollo/server': minor
+---
+
+If a POST body contains a non-string `operationName` or a non-object `variables` or `extensions`, fail with status code 400 instead of ignoring the field.
+
+In addition to being a reasonable idea, this provides more compliance with the "GraphQL over HTTP" spec.
+
+This is a backwards incompatible change, but we are still early in the Apollo Server 4 adoption cycle and this is in line with the change already made in Apollo Server 4 to reject requests providing `variables` or `extensions` as strings. If this causes major problems for users who have already upgraded to Apollo Server 4 in production, we can consider reverting or partially reverting this change.

.changeset/tidy-eggs-turn.md

@@ -0,0 +1,5 @@
+---
+'@apollo/server-integration-testsuite': patch
+---
+
+The integration test suite now incorporates the `graphql-http` package's audit suite for the "GraphQL over HTTP" specification.

docs/source/migration.mdx

@@ -1070,6 +1070,8 @@ Whereas this query would be invalid:
 }
 ```
 
+(Moreover, Apollo Server 4 responds with a 400 status code if `variables` and `extensions` are provided in a `POST` body with any type other than object, such as array, boolean, or null. Similarly, it responds with a 400 status code if `operationName` is provided in a `POST` body with any type other than string.)
+
 
 ## Changed features
 

packages/integration-testsuite/package.json

@@ -36,6 +36,7 @@
     "@josephg/resolvable": "^1.0.1",
     "body-parser": "^1.20.0",
     "express": "^4.18.1",
+    "graphql-http": "1.8.0",
     "graphql-tag": "^2.12.6",
     "loglevel": "^1.8.0",
     "node-fetch": "^2.6.7",

packages/integration-testsuite/src/httpSpecTests.ts

@@ -0,0 +1,84 @@
+import type {
+  CreateServerForIntegrationTests,
+  CreateServerForIntegrationTestsResult,
+} from './index.js';
+import { afterAll, beforeAll, describe, test } from '@jest/globals';
+import { serverAudits } from 'graphql-http';
+import fetch from 'node-fetch';
+
+export function defineIntegrationTestSuiteHttpSpecTests(
+  createServer: CreateServerForIntegrationTests,
+) {
+  describe('httpSpecTests.ts', () => {
+    let createServerResult: CreateServerForIntegrationTestsResult;
+
+    beforeAll(async () => {
+      createServerResult = await createServer({
+        // Any schema will do (the tests just run `{__typename}`).
+        typeDefs: 'type Query { x: ID }',
+        // The test doesn't know we should send apollo-require-preflight along
+        // with GETs. We could override `fetchFn` to add it but this seems simple enough.
+        csrfPrevention: false,
+      });
+    });
+
+    afterAll(async () => {
+      await createServerResult.server.stop();
+      await createServerResult.extraCleanup?.();
+    });
+
+    for (const audit of serverAudits({
+      url: () => createServerResult.url,
+      fetchFn: fetch,
+    })) {
+      test(audit.name, async () => {
+        const result = await audit.fn();
+
+        if (result.status === 'ok') {
+          return;
+        }
+        if (result.status === 'error') {
+          throw new Error(result.reason);
+        }
+
+        if (result.status !== 'warn') {
+          throw new Error(`unknown status ${result.status}`);
+        }
+
+        // We failed an optional audit. That's OK, but let's make sure it's
+        // one of the ones we expect to fail!
+
+        // The spec has a bunch of optional suggestions which say that you
+        // should use 200 rather than 400 for various errors unless opting in to
+        // the new application/graphql-response+json response type. That's based
+        // on the theory that "400 + application/json" might come from some
+        // random proxy layer rather than an actual GraphQL processor and so it
+        // shouldn't be relied on. (It *does* expect you to use 400 for these
+        // errors when returning `application/graphql-response+json`, and we
+        // pass those tests.) But Apollo Server has used non-200 status codes
+        // for a long time, and in fact a major reason these are merely SHOULDs
+        // in the spec is so that AS can pass without backwards-incompatible
+        // changes here. So we ignore these particular SHOULD failures.
+        if (
+          audit.name.startsWith('SHOULD use 200 status code') &&
+          audit.name.endsWith('when accepting application/json') &&
+          result.reason === 'Status code 400 is not 200'
+        ) {
+          return;
+        }
+
+        // This is a bit weird: this issue is not actually that we include the 'data'
+        // entry, but that JSON parse errors aren't delivered as JSON responses at all.
+        // See https://github.com/graphql/graphql-http/issues/25
+        if (
+          audit.name ===
+          'SHOULD not contain the data entry on JSON parsing failure when accepting application/graphql-response+json'
+        ) {
+          return;
+        }
+
+        throw new Error(result.reason);
+      });
+    }
+  });
+}

packages/integration-testsuite/src/index.ts

@@ -7,6 +7,7 @@ import type {
 import { describe } from '@jest/globals';
 import { defineIntegrationTestSuiteApolloServerTests } from './apolloServerTests.js';
 import { defineIntegrationTestSuiteHttpServerTests } from './httpServerTests.js';
+import { defineIntegrationTestSuiteHttpSpecTests } from './httpSpecTests.js';
 
 export interface CreateServerForIntegrationTestsResult {
   server: ApolloServer<BaseContext>;
@@ -33,5 +34,6 @@ export function defineIntegrationTestSuite(
   describe('integration tests', () => {
     defineIntegrationTestSuiteApolloServerTests(createServer, options);
     defineIntegrationTestSuiteHttpServerTests(createServer, options);
+    defineIntegrationTestSuiteHttpSpecTests(createServer);
   });
 }

packages/server/src/runHttpQuery.ts

@@ -153,6 +153,33 @@ export async function runHttpQuery<TContext extends BaseContext>({
         );
       }
 
+      if (
+        'extensions' in httpRequest.body &&
+        !isStringRecord(httpRequest.body.extensions)
+      ) {
+        throw new BadRequestError(
+          '`extensions` in a POST body must be an object if provided.',
+        );
+      }
+
+      if (
+        'variables' in httpRequest.body &&
+        !isStringRecord(httpRequest.body.variables)
+      ) {
+        throw new BadRequestError(
+          '`variables` in a POST body must be an object if provided.',
+        );
+      }
+
+      if (
+        'operationName' in httpRequest.body &&
+        typeof httpRequest.body.operationName !== 'string'
+      ) {
+        throw new BadRequestError(
+          '`operationName` in a POST body must be a string if provided.',
+        );
+      }
+
       graphQLRequest = {
         query: fieldIfString(httpRequest.body, 'query'),
         operationName: fieldIfString(httpRequest.body, 'operationName'),