From 37b3b7fb57ea105f40776166c9532536fd3f053d Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 21 Nov 2022 13:59:48 -0800 Subject: [PATCH] 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 https://github.com/graphql/graphql-http/issues/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 --- .changeset/good-pets-begin.md | 10 +++ .changeset/tidy-eggs-turn.md | 5 ++ docs/source/migration.mdx | 2 + package-lock.json | 19 +++++ packages/integration-testsuite/package.json | 1 + .../src/httpSpecTests.ts | 84 +++++++++++++++++++ packages/integration-testsuite/src/index.ts | 2 + packages/server/src/runHttpQuery.ts | 27 ++++++ 8 files changed, 150 insertions(+) create mode 100644 .changeset/good-pets-begin.md create mode 100644 .changeset/tidy-eggs-turn.md create mode 100644 packages/integration-testsuite/src/httpSpecTests.ts diff --git a/.changeset/good-pets-begin.md b/.changeset/good-pets-begin.md new file mode 100644 index 00000000000..8b48764f2c0 --- /dev/null +++ b/.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. diff --git a/.changeset/tidy-eggs-turn.md b/.changeset/tidy-eggs-turn.md new file mode 100644 index 00000000000..be7ccd50545 --- /dev/null +++ b/.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. diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index f7db4d7e2fb..b7165952a7b 100644 --- a/docs/source/migration.mdx +++ b/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 diff --git a/package-lock.json b/package-lock.json index 9eadab0b79f..f9048bff9df 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7854,6 +7854,17 @@ "graphql": "^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0 || ^16.0.0" } }, + "node_modules/graphql-http": { + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/graphql-http/-/graphql-http-1.8.0.tgz", + "integrity": "sha512-8nZJW5zybaKMa6pPXgZKW2Jy5pescyguhCwaF0eBLCmsErNbwT940ShHSZRTEseplAizXdea3CJEBsHUHYUlCw==", + "engines": { + "node": ">=12" + }, + "peerDependencies": { + "graphql": ">=0.11 <=16" + } + }, "node_modules/graphql-request": { "version": "4.3.0", "resolved": "https://registry.npmjs.org/graphql-request/-/graphql-request-4.3.0.tgz", @@ -13034,6 +13045,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", @@ -13400,6 +13412,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", @@ -19269,6 +19282,12 @@ "tslib": "^2.4.0" } }, + "graphql-http": { + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/graphql-http/-/graphql-http-1.8.0.tgz", + "integrity": "sha512-8nZJW5zybaKMa6pPXgZKW2Jy5pescyguhCwaF0eBLCmsErNbwT940ShHSZRTEseplAizXdea3CJEBsHUHYUlCw==", + "requires": {} + }, "graphql-request": { "version": "4.3.0", "resolved": "https://registry.npmjs.org/graphql-request/-/graphql-request-4.3.0.tgz", diff --git a/packages/integration-testsuite/package.json b/packages/integration-testsuite/package.json index 9d2501be338..2cbd203b47f 100644 --- a/packages/integration-testsuite/package.json +++ b/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", diff --git a/packages/integration-testsuite/src/httpSpecTests.ts b/packages/integration-testsuite/src/httpSpecTests.ts new file mode 100644 index 00000000000..e25e4c41821 --- /dev/null +++ b/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); + }); + } + }); +} diff --git a/packages/integration-testsuite/src/index.ts b/packages/integration-testsuite/src/index.ts index bcba9b9299d..b9c8db9bb2f 100644 --- a/packages/integration-testsuite/src/index.ts +++ b/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; @@ -33,5 +34,6 @@ export function defineIntegrationTestSuite( describe('integration tests', () => { defineIntegrationTestSuiteApolloServerTests(createServer, options); defineIntegrationTestSuiteHttpServerTests(createServer, options); + defineIntegrationTestSuiteHttpSpecTests(createServer); }); } diff --git a/packages/server/src/runHttpQuery.ts b/packages/server/src/runHttpQuery.ts index 9086c68ad35..d9cdf2b43f9 100644 --- a/packages/server/src/runHttpQuery.ts +++ b/packages/server/src/runHttpQuery.ts @@ -153,6 +153,33 @@ export async function runHttpQuery({ ); } + 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'),