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'),